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'