[pacman-dev] [PATCH] scripts/library: rewrite parse_options

21 Jul 2011

In addition to being what I feel is a cleaner and faster implementation,
we avoid the use of eval by normalizing option arguments into a global
array which is then set after a successful call to parse_options.

This trims out the idea of having multiple arguments to a single option,
making our parsing algorithm a little more sane. We never took advantage of
this in makepkg (for the one option that feasibly supports it), and I
think we've overlooked a much simpler solution in pacman-key. Since
actions are limited to 1 at a time the leftover positional parameters
become the keys or keyfiles which are acted upon.

Also added is a new test directory test/scripts with a harness for
parse_options, run as part of make check.

Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
Thoughts? I know Allan wasn't quite sold on this, as the downside is that we
sort of pigeonhole ourselves into using the non-optional parameter as arguments
to our action. This has zero effect on makepkg.

I've also thought to add --option=arg syntax parsing, but I'm not sure we need
this.

 Makefile.am                        |    6 +-
 configure.ac                       |    1 +
 scripts/library/parse_options.sh   |  195 +++++++++++++++++-------------------
 scripts/makepkg.sh.in              |   13 +--
 scripts/pacman-key.sh.in           |   59 ++++++------
 test/scripts/Makefile.am           |    9 ++
 test/scripts/parse_options_test.sh |  103 +++++++++++++++++++
 7 files changed, 242 insertions(+), 144 deletions(-)
 rewrite scripts/library/parse_options.sh (89%)
 create mode 100644 test/scripts/Makefile.am
 create mode 100755 test/scripts/parse_options_test.sh

diff --git a/Makefile.am b/Makefile.am
index 286e6ae..f88b6d1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,4 +1,4 @@
-SUBDIRS = lib/libalpm src/util src/pacman scripts etc test/pacman test/util contrib
+SUBDIRS = lib/libalpm src/util src/pacman scripts etc test/pacman test/scripts test/util contrib
 if WANT_DOC
 SUBDIRS += doc
 endif
@@ -21,12 +21,14 @@ dist_pkgdata_DATA = \
 	proto/ChangeLog.proto
 
 # run the pactest test suite and vercmp tests
-check-local: test/pacman test/util src/pacman src/util
+check-local: test/pacman test/scripts test/util src/pacman src/util
 	$(PYTHON) $(top_srcdir)/test/pacman/pactest.py --debug=1 \
 		--test $(top_srcdir)/test/pacman/tests/*.py \
 		-p $(top_builddir)/src/pacman/pacman
 	$(SH) $(top_srcdir)/test/util/vercmptest.sh \
 		$(top_builddir)/src/util/vercmp
+	$(BASH_SHELL) $(top_srcdir)/test/scripts/parse_options_test.sh \
+		$(top_builddir)/scripts/library/parse_options.sh
 
 # create the pacman DB and cache directories upon install
 install-data-local:
diff --git a/configure.ac b/configure.ac
index 6ad5be5..a553176 100644
--- a/configure.ac
+++ b/configure.ac
@@ -381,6 +381,7 @@ doc/Makefile
 etc/Makefile
 test/pacman/Makefile
 test/pacman/tests/Makefile
+test/scripts/Makefile
 test/util/Makefile
 contrib/Makefile
 Makefile
diff --git a/scripts/library/parse_options.sh b/scripts/library/parse_options.sh
dissimilarity index 89%
index 48fd42c..215059f 100644
--- a/scripts/library/parse_options.sh
+++ b/scripts/library/parse_options.sh
@@ -1,105 +1,90 @@
-# getopt like parser
-parse_options() {
-	local short_options=$1; shift;
-	local long_options=$1; shift;
-	local ret=0;
-	local unused_options=""
-	local i
-
-	while [[ -n $1 ]]; do
-		if [[ ${1:0:2} = '--' ]]; then
-			if [[ -n ${1:2} ]]; then
-				local match=""
-				for i in ${long_options//,/ }; do
-					if [[ ${1:2} = ${i//:} ]]; then
-						match=$i
-						break
-					fi
-				done
-				if [[ -n $match ]]; then
-					local needsargument=0
-
-					[[ ${match} = ${1:2}: ]] && needsargument=1
-					[[ ${match} = ${1:2}:: && -n $2 && ${2:0:1} != "-" ]] && needsargument=1
-
-					if (( ! needsargument )); then
-						printf ' %s' "$1"
-					else
-						if [[ -n $2 ]]; then
-							printf ' %s ' "$1"
-							shift
-							printf "'%q" "$1"
-							while [[ -n $2 && ${2:0:1} != "-" ]]; do
-								shift
-								printf " %q" "$1"
-							done
-							printf "'"
-						else
-							printf "@SCRIPTNAME@: $(gettext "option %s requires an argument\n")" "'$1'" >&2
-							ret=1
-						fi
-					fi
-				else
-					echo "@SCRIPTNAME@: $(gettext "unrecognized option") '$1'" >&2
-					ret=1
-				fi
-			else
-				shift
-				break
-			fi
-		elif [[ ${1:0:1} = '-' ]]; then
-			for ((i=1; i<${#1}; i++)); do
-				if [[ $short_options =~ ${1:i:1} ]]; then
-					local needsargument=0
-
-					[[ $short_options =~ ${1:i:1}: && ! $short_options =~ ${1:i:1}:: ]] && needsargument=1
-					[[ $short_options =~ ${1:i:1}:: && \
-						( -n ${1:$i+1} || ( -n $2 && ${2:0:1} != "-" ) ) ]] && needsargument=1
-
-					if (( ! needsargument )); then
-						printf ' -%s' "${1:i:1}"
-					else
-						if [[ -n ${1:$i+1} ]]; then
-							printf ' -%s ' "${1:i:1}"
-							printf "'%q" "${1:$i+1}"
-							while [[ -n $2 && ${2:0:1} != "-" ]]; do
-								shift
-								printf " %q" "$1"
-							done
-							printf "'"
-						else
-							if [[ -n $2 ]]; then
-								printf ' -%s ' "${1:i:1}"
-								shift
-								printf "'%q" "$1"
-								while [[ -n $2 && ${2:0:1} != "-" ]]; do
-									shift
-									printf " %q" "$1"
-								done
-								printf "'"
-
-							else
-								printf "@SCRIPTNAME@: $(gettext "option %s requires an argument\n")" "'-${1:i:1}'" >&2
-								ret=1
-							fi
-						fi
-						break
-					fi
-				else
-					echo "@SCRIPTNAME@: $(gettext "unrecognized option") '-${1:i:1}'" >&2
-					ret=1
-				fi
-			done
-		else
-			unused_options="${unused_options} '$1'"
-		fi
-		shift
-	done
-
-	printf " --"
-	[[ $unused_options ]] && printf ' %s' "${unused_options[@]}"
-	[[ $1 ]] && printf " '%s'" "$@"
-	printf "\n"
-
-	return $ret
-}\ No newline at end of file
+# getopt-like parser
+parse_options () {
+	local opt= i= shortopts=$1
+	local -a longopts param
+
+	# split the longopt array on commas
+	IFS=, read -r -a longopts <<< "$2"
+	shift 2
+
+	while (( $# )); do
+		case $1 in
+			--) # explicit end of options
+				shift
+				break
+				;;
+			-[!-]*) # short option
+				for (( i = 1; i < ${#1}; i++ )); do
+					opt=${1:i:1}
+
+					# option doesn't exist
+					if [[ $shortopts != *$opt* ]]; then
+						echo "@SCRIPTNAME@: $(gettext "unrecognized option") '-$opt'" >&2
+						OPTRET=(--)
+						return 1
+					fi
+
+					OPTRET+=("-$opt")
+					# option requires optarg
+					if [[ $shortopts = *$opt:* ]]; then
+						# if we're not at the end of the option chunk, the rest is the optarg
+						if (( i < ${#1} - 1 )); then
+							OPTRET+=("${1:i+1}")
+							break
+
+						# if we're at the end, grab the the next positional, if it exists
+						elif (( i == ${#1} - 1 )) && [[ $2 ]]; then
+							OPTRET+=("$2")
+							shift
+							break
+
+						# parse failure
+						else
+							printf "@SCRIPTNAME@: $(gettext "option %s requires an argument\n")" "'-$opt'" >&2
+							OPTRET=(--)
+							return 1
+						fi
+					fi
+
+				done
+				;;
+			--?*) # long option
+				opt=${1#--}
+				OPTRET+=("$1")
+
+				for longopt in "${longopts[@]}"; do
+					if [[ $longopt =~ ^($opt)(:?)$ ]]; then
+						# option requires optarg
+						if [[ -n ${BASH_REMATCH[2]} ]]; then
+							if [[ -z $2 ]]; then
+								printf "@SCRIPTNAME@: $(gettext "option %s requires an argument\n")" "'--$opt'" >&2
+								OPTRET=(--)
+								return 1
+							fi
+							OPTRET+=("$2")
+							shift 2
+							continue 2
+
+						# simple longopt
+						elif [[ -n ${BASH_REMATCH[1]} ]]; then
+							shift
+							continue 2
+						fi
+					fi
+				done
+				echo "@SCRIPTNAME@: $(gettext "unrecognized option") '--$opt'" >&2
+				OPTRET=(--)
+				return 1
+				;;
+			*) # non-option arg encountered, add it as a parameter
+				param+=("$1")
+				;;
+		esac
+		shift
+	done
+
+	# add end-of-opt terminator and any leftover positional parameters
+	OPTRET+=('--' "${param[@]}" "$@")
+
+	return 0
+}
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index a4e7156..b6ab2ee 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1755,14 +1755,14 @@ OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
 OPT_LONG+=",repackage,skipinteg,sign,source,syncdeps,version,config:"
 # Pacman Options
 OPT_LONG+=",noconfirm,noprogressbar"
-if ! OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@")"; then
+if ! parse_options "$OPT_SHORT" "$OPT_LONG" "$@"; then
 	echo; usage; exit 1 # E_INVALID_OPTION;
 fi
-eval set -- "$OPT_TEMP"
-unset OPT_SHORT OPT_LONG OPT_TEMP
+set -- "${OPTRET[@]}"
+unset OPT_SHORT OPT_LONG OPTRET
 
-while true; do
-	case "$1" in
+while (( $# )); do
+	case $1 in
 		# Pacman Options
 		--noconfirm)      PACMAN_OPTS+=" --noconfirm" ;;
 		--noprogressbar)  PACMAN_OPTS+=" --noprogressbar" ;;
@@ -1801,8 +1801,7 @@ while true; do
 		-h|--help)        usage; exit 0 ;; # E_OK
 		-V|--version)     version; exit 0 ;; # E_OK
 
-		--)               OPT_IND=0; shift; break;;
-		*)                usage; exit 1 ;; # E_INVALID_OPTION
+		--)               shift; break;;
 	esac
 	shift
 done
diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
index 5cb5bd5..d0fb0ef 100644
--- a/scripts/pacman-key.sh.in
+++ b/scripts/pacman-key.sh.in
@@ -255,16 +255,16 @@ reload_keyring() {
 }
 
 receive_keys() {
-	if [[ -z ${KEYIDS[@]} ]]; then
+	if [[ -z $1 ]]; then
 		error "$(gettext "You need to specify the keyserver and at least one key identifier")"
 		exit 1
 	fi
-	${GPG_PACMAN} --keyserver "$KEYSERVER" --recv-keys "${KEYIDS[@]}"
+	${GPG_PACMAN} --keyserver "$KEYSERVER" --recv-keys "$@"
 }
 
 edit_keys() {
 	local errors=0;
-	for key in ${KEYIDS[@]}; do
+	for key; do
 		# Verify if the key exists in pacman's keyring
 		if ! ${GPG_PACMAN} --list-keys "$key" &>/dev/null; then
 			error "$(gettext "The key identified by %s does not exist")" "$key"
@@ -273,7 +273,7 @@ edit_keys() {
 	done
 	(( errors )) && exit 1;
 
-	for key in ${KEYIDS[@]}; do
+	for key; do
 		${GPG_PACMAN} --edit-key "$key"
 	done
 }
@@ -285,32 +285,33 @@ if ! type gettext &>/dev/null; then
 	}
 fi
 
-OPT_SHORT="a::d:e:f::hlr:uv:V"
-OPT_LONG="add::,config:,delete:,edit-key:,export::,finger::,gpgdir:"
+declare -a OPTRET
+OPT_SHORT="ade:fhlr:uv:V"
+OPT_LONG="add,config:,delete,edit-key,export,finger,gpgdir:"
 OPT_LONG+=",help,init,list,receive:,reload,updatedb,verify:,version"
-if ! OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@")"; then
+if ! parse_options "$OPT_SHORT" "$OPT_LONG" "$@"; then
 	echo; usage; exit 1 # E_INVALID_OPTION;
 fi
-eval set -- "$OPT_TEMP"
-unset OPT_SHORT OPT_LONG OPT_TEMP
+set -- "${OPTRET[@]}"
+unset OPT_SHORT OPT_LONG OPTRET
 
-if [[ $1 == "--" ]]; then
-	usage;
-	exit 0;
+if [[ -z $1 ]]; then
+	usage
+	exit 0
 fi
 
-while true; do
-	case "$1" in
-		-a|--add)         ADD=1; [[ -n $2 && ${2:0:1} != "-" ]] && shift && KEYFILES=($1) ;;
+while (( $# )); do
+	case $1 in
+		-a|--add)         ADD=1 ;;
 		--config)         shift; CONFIG=$1 ;;
-		-d|--delete)      DELETE=1; shift; KEYIDS=($1) ;;
-		--edit-key)       EDITKEY=1; shift; KEYIDS=($1) ;;
-		-e|--export)      EXPORT=1; [[ -n $2 && ${2:0:1} != "-" ]] && shift && KEYIDS=($1) ;;
-		-f|--finger)      FINGER=1; [[ -n $2 && ${2:0:1} != "-" ]] && shift && KEYIDS=($1) ;;
+		-d|--delete)      DELETE=1 ;;
+		--edit-key)       EDITKEY=1 ;;
+		-e|--export)      EXPORT=1 ;;
+		-f|--finger)      FINGER=1 ;;
 		--gpgdir)         shift; PACMAN_KEYRING_DIR=$1 ;;
 		--init)           INIT=1 ;;
 		-l|--list)        LIST=1 ;;
-		-r|--receive)     RECEIVE=1; shift; KEYSERVER=$1; KEYIDS=("${@:1}") ;;
+		-r|--receive)     RECEIVE=1; shift; KEYSERVER=$1 ;;
 		--reload)         RELOAD=1 ;;
 		-u|--updatedb)    UPDATEDB=1 ;;
 		-v|--verify)      VERIFY=1; shift; SIGNATURE=$1 ;;
@@ -318,13 +319,11 @@ while true; do
 		-h|--help)        usage; exit 0 ;;
 		-V|--version)     version; exit 0 ;;
 
-		--)               OPT_IND=0; shift; break;;
-		*)                usage; exit 1 ;;
+		--)               shift; break ;;
 	esac
 	shift
 done
 
-
 if ! type -p gpg >/dev/null; then
     error "$(gettext "Cannot find the %s binary required for all %s operations.")" "gpg" "pacman-key"
 	exit 1
@@ -364,14 +363,14 @@ esac
 
 (( ! INIT )) && check_keyring
 
-(( ADD )) && ${GPG_PACMAN} --quiet --batch --import "${KEYFILES[@]}"
-(( DELETE )) && ${GPG_PACMAN} --quiet --batch --delete-key --yes "${KEYIDS[@]}"
-(( EDITKEY )) && edit_keys
-(( EXPORT )) && ${GPG_PACMAN} --armor --export "${KEYIDS[@]}"
-(( FINGER )) && ${GPG_PACMAN} --batch --fingerprint "${KEYIDS[@]}"
+(( ADD )) && ${GPG_PACMAN} --quiet --batch --import "$@"
+(( DELETE )) && ${GPG_PACMAN} --quiet --batch --delete-key --yes "$@"
+(( EDITKEY )) && edit_keys "$@"
+(( EXPORT )) && ${GPG_PACMAN} --armor --export "$@"
+(( FINGER )) && ${GPG_PACMAN} --batch --fingerprint "$@"
 (( INIT )) && initialize
-(( LIST )) && ${GPG_PACMAN} --batch --list-sigs "${KEYIDS[@]}"
-(( RECEIVE )) && receive_keys
+(( LIST )) && ${GPG_PACMAN} --batch --list-sigs "$@"
+(( RECEIVE )) && receive_keys "$@"
 (( RELOAD )) && reload_keyring
 (( UPDATEDB )) && ${GPG_PACMAN} --batch --check-trustdb
 (( VERIFY )) && ${GPG_PACMAN} --verify $SIGNATURE
diff --git a/test/scripts/Makefile.am b/test/scripts/Makefile.am
new file mode 100644
index 0000000..caa9ab1
--- /dev/null
+++ b/test/scripts/Makefile.am
@@ -0,0 +1,9 @@
+check_SCRIPTS = \
+	parse_options_test.sh
+
+noinst_SCRIPTS = $(check_SCRIPTS)
+
+EXTRA_DIST = \
+	$(check_SCRIPTS)
+
+# vim:set ts=2 sw=2 noet:
diff --git a/test/scripts/parse_options_test.sh b/test/scripts/parse_options_test.sh
new file mode 100755
index 0000000..85981e1
--- /dev/null
+++ b/test/scripts/parse_options_test.sh
@@ -0,0 +1,103 @@
+#!/bin/bash
+
+declare -i testcount=0 pass=0 fail=0
+
+# source the library function
+if [[ -z $1 || ! -f $1 ]]; then
+  printf "error: path to parse_option library not provided or does not exist\n"
+  exit 1
+fi
+. "$1"
+
+if ! type -t parse_options >/dev/null; then
+  printf 'parse_options function not found\n'
+  exit 1
+fi
+
+# borrow opts from makepkg
+OPT_SHORT="AcdefFghiLmop:rRsV"
+OPT_LONG="allsource,asroot,ignorearch,check,clean,nodeps"
+OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver"
+OPT_LONG+=",install,key:,log,nocolor,nobuild,nocheck,nosign,pkg:,rmdeps"
+OPT_LONG+=",repackage,skipinteg,sign,source,syncdeps,version,config:"
+OPT_LONG+=",noconfirm,noprogressbar"
+
+parse() {
+  local result=$1 tokencount=$2; shift 2
+
+  (( ++testcount ))
+  #printf '[TEST %2s]: ' "$testcount"
+  parse_options "$OPT_SHORT" "$OPT_LONG" "$@" 2>/dev/null
+  test_result "$result" "$tokencount" "$*" "${OPTRET[@]}"
+  unset OPTRET
+}
+
+test_result() {
+  local result=$1 tokencount=$2 input=$3; shift 3
+
+  if { [[ $result = "$*" ]] || [[ $2 = NULL && -z $1 ]]; } && (( tokencount == $# )); then
+    (( ++pass ))
+  else
+    printf '[TEST %3s]: FAIL\n' "$testcount"
+    printf '      input: %s\n' "$input"
+    printf '     output: %s (%s tokens)\n' "$*" "$#"
+    printf '   expected: %s (%s tokens)\n' "$result" "$tokencount"
+    echo
+    (( ++fail ))
+  fi
+}
+
+summarize() {
+  if (( !fail )); then
+    printf 'All %s tests successful\n' "$testcount"
+    exit 0
+  else
+    printf '%s of %s tests failed\n' "$fail" "$testcount"
+    exit 1
+  fi
+}
+trap 'summarize' EXIT
+
+printf 'Beginning parse_options tests\n'
+
+# usage: parse <expected result> <token count> test-params...
+# a failed parse will match only the end of options marker '--'
+
+# no options
+parse '--' 1
+
+# short options
+parse '-s -r --' 3 -s -r
+
+# short options, no spaces
+parse '-s -r --' 3 -sr
+
+# short opt missing an opt arg
+parse '--' 1 -s -p
+
+# short opt with an opt arg
+parse '-p PKGBUILD -L --' 4 -p PKGBUILD -L
+
+# short opt with an opt arg, no space
+parse '-p PKGBUILD --' 3 -pPKGBUILD
+
+# valid shortopts as a long opt
+parse '--' 1 --sir
+
+# long opt with missing optarg
+parse '--' 1 -sr --pkg
+
+# long opt with optarg
+parse '--pkg foo --' 3 --pkg foo
+
+# explicit end of options with options after
+parse '-s -r -- foo bar baz' 6 -s -r -- foo bar baz
+
+# non-option parameters mixed in with options
+parse '-s -r -- foo baz' 5 -s foo baz -r
+
+# optarg with whitespace
+parse '-p foo bar -s --' 4 -p'foo bar' -s
+
+# non-option parameter with whitespace
+parse '-i -- foo bar' 3 -i 'foo bar'
-- 
1.7.6

    