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
---
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