[pacman-dev] [PATCH] scripts/library: rewrite parse_options
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
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner <d@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@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
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this... Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package. I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added. But my main issue is that we lose the ability to specify multiple arguments with optional parameters. We do not use this anywhere at the moment, so it is not a big loss but that is not to say we will never use it in the future. Basically, I am not sure what the actual advantage of rewriting this is... Allan
On Thu, Aug 18, 2011 at 08:57:55AM +1000, Allan McRae wrote:
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this...
Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package.
And, imo, this introduces bizzare unexpected behavior. With the "standard" getopt{,_long}, passing something such as: --pkg foo bar I'd expect bar to be completely unrelated to the flag. Not the case here, and this behavior isn't really documented clearly at all. We don't even properly handle arguments with whitespace anymore. Not such a big deal for makepkg, but I think it's reasonable that pacman-key might some day need to import a key from a file with a space in the name.
I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added.
We can (should?) follow pacman here and separate multiple arguments by commas. I'm going to channel Dan and call out "consistency" here.
But my main issue is that we lose the ability to specify multiple arguments with optional parameters. We do not use this anywhere at the moment, so it is not a big loss but that is not to say we will never use it in the future.
My problem with this is that you're then forced, in a calling application, to change the behavior of your parsing of normalized options. It's a little strange.
Basically, I am not sure what the actual advantage of rewriting this is...
Faster, cleaner, safer (no eval!!), and more standard. That's what I'm selling here. d
On 18/08/11 09:34, Dave Reisner wrote:
On Thu, Aug 18, 2011 at 08:57:55AM +1000, Allan McRae wrote:
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this...
Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package.
And, imo, this introduces bizzare unexpected behavior. With the "standard" getopt{,_long}, passing something such as:
--pkg foo bar
I'd expect bar to be completely unrelated to the flag. Not the case here, and this behavior isn't really documented clearly at all. We don't even properly handle arguments with whitespace anymore. Not such a big deal for makepkg, but I think it's reasonable that pacman-key might some day need to import a key from a file with a space in the name.
I thought the whitespace issue was fixed but testing now I see it is not. That is definitely something that needs addressed.
I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added.
We can (should?) follow pacman here and separate multiple arguments by commas. I'm going to channel Dan and call out "consistency" here.
You mean like "pacman -S pkg1,pkg2"? Fairly sure we do not do that! So I call consistency... Where do we use commas in pacman options? And it is going to be just the --pkg option in makepkg that requires such quoting and commas as far as I can tell. All pacman-key options will still take a non-comma separated list. Allan
On Wed, Aug 17, 2011 at 7:22 PM, Allan McRae <allan@archlinux.org> wrote:
On 18/08/11 09:34, Dave Reisner wrote:
On Thu, Aug 18, 2011 at 08:57:55AM +1000, Allan McRae wrote:
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this...
Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package.
And, imo, this introduces bizzare unexpected behavior. With the "standard" getopt{,_long}, passing something such as:
--pkg foo bar
I'd expect bar to be completely unrelated to the flag. Not the case here, and this behavior isn't really documented clearly at all. We don't even properly handle arguments with whitespace anymore. Not such a big deal for makepkg, but I think it's reasonable that pacman-key might some day need to import a key from a file with a space in the name.
I thought the whitespace issue was fixed but testing now I see it is not. That is definitely something that needs addressed.
I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added.
We can (should?) follow pacman here and separate multiple arguments by commas. I'm going to channel Dan and call out "consistency" here.
You mean like "pacman -S pkg1,pkg2"? Fairly sure we do not do that! So I call consistency... Where do we use commas in pacman options?
pacman -Syu --ignorepkg foo,bar.baz
On Thu, Aug 18, 2011 at 10:22:18AM +1000, Allan McRae wrote:
On 18/08/11 09:34, Dave Reisner wrote:
On Thu, Aug 18, 2011 at 08:57:55AM +1000, Allan McRae wrote:
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this...
Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package.
And, imo, this introduces bizzare unexpected behavior. With the "standard" getopt{,_long}, passing something such as:
--pkg foo bar
I'd expect bar to be completely unrelated to the flag. Not the case here, and this behavior isn't really documented clearly at all. We don't even properly handle arguments with whitespace anymore. Not such a big deal for makepkg, but I think it's reasonable that pacman-key might some day need to import a key from a file with a space in the name.
I thought the whitespace issue was fixed but testing now I see it is not. That is definitely something that needs addressed.
I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added.
We can (should?) follow pacman here and separate multiple arguments by commas. I'm going to channel Dan and call out "consistency" here.
You mean like "pacman -S pkg1,pkg2"? Fairly sure we do not do that! So I call consistency... Where do we use commas in pacman options?
No I mean like pacman -Syu --ignore pkg1,pkg2 But, that's a good point about the "flexibility" of the non-option parameters. Just like I'm proposing for pacman-key, we generalize the term for pacman and call them "targets". They could be a repo, a file, a sync/local package... They don't belong to any particular option, but they're acted upon by the singular specified action. d
On 18/08/11 10:26, Dave Reisner wrote:
On Thu, Aug 18, 2011 at 10:22:18AM +1000, Allan McRae wrote:
On 18/08/11 09:34, Dave Reisner wrote:
On Thu, Aug 18, 2011 at 08:57:55AM +1000, Allan McRae wrote:
On 18/08/11 08:27, Dan McGee wrote:
On Thu, Jul 21, 2011 at 3:40 PM, Dave Reisner<d@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@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.
Well, I was deferring to you as Dave is right that I was never sold on this...
Unless I am missing something, this does have a minor effect on makepkg. In git "makepkg --pkg foo bar" builds only foo and bar from a split package.
And, imo, this introduces bizzare unexpected behavior. With the "standard" getopt{,_long}, passing something such as:
--pkg foo bar
I'd expect bar to be completely unrelated to the flag. Not the case here, and this behavior isn't really documented clearly at all. We don't even properly handle arguments with whitespace anymore. Not such a big deal for makepkg, but I think it's reasonable that pacman-key might some day need to import a key from a file with a space in the name.
I thought the whitespace issue was fixed but testing now I see it is not. That is definitely something that needs addressed.
I guess with this patch we would have to quote the package names in some way (like is needed on the current maint release). So we would need to revert changes made to the makepkg documentation when the multiple arguments stuff was added.
We can (should?) follow pacman here and separate multiple arguments by commas. I'm going to channel Dan and call out "consistency" here.
You mean like "pacman -S pkg1,pkg2"? Fairly sure we do not do that! So I call consistency... Where do we use commas in pacman options?
No I mean like pacman -Syu --ignore pkg1,pkg2
But, that's a good point about the "flexibility" of the non-option parameters. Just like I'm proposing for pacman-key, we generalize the term for pacman and call them "targets". They could be a repo, a file, a sync/local package... They don't belong to any particular option, but they're acted upon by the singular specified action.
I'll just add a final note that we are losing the ability to ever do anything like "pacman-key --add key1 key2 --delete key3" (at least with that syntax). You might say that is a good thing, but there is a long standing bug report for universal transactions in pacman (the original 4.0 target!) allowing us to do the analogous "pacman -S pkg1 pkg2 -R pkg3". I'm happy taking away the ability to do one under the argument of consistency as long as the other is taken off the table too. Allan
participants (3)
-
Allan McRae
-
Dan McGee
-
Dave Reisner