[pacman-dev] [PATCH] scripts/library: rewrite parse_options
Dan McGee
dpmcgee at gmail.com
Wed Aug 17 18:27:40 EDT 2011
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner <d at falconindy.com> wrote:
> 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 at 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.
Allan, was deferring to you on 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
>
>
>
More information about the pacman-dev
mailing list