[pacman-dev] [PATCH] libmakepkg: fix empty arguments in parseopts
Previously parseopts checked if there was an argument by checking that the string was non-empty, resulting in empty arguments being incorrectly considered non-existent. This change makes parseopts check if arguments exist at all, rather than checking that they are non-empty Signed-off-by: Ethan Sommer <e5ten.arch@gmail.com> --- scripts/libmakepkg/util/parseopts.sh.in | 8 ++++---- test/scripts/parseopts_test.sh | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/scripts/libmakepkg/util/parseopts.sh.in b/scripts/libmakepkg/util/parseopts.sh.in index c056cb1e..8fb862d8 100644 --- a/scripts/libmakepkg/util/parseopts.sh.in +++ b/scripts/libmakepkg/util/parseopts.sh.in @@ -102,7 +102,7 @@ parseopts() { 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 + elif (( i == ${#1} - 1 && $# > 1 )); then OPTRET+=("$2") shift break @@ -121,7 +121,7 @@ parseopts() { case $? in 0) # parse failure - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then printf "${0##*/}: $(gettext "option '%s' does not allow an argument")\n" "--$opt" >&2 OPTRET=(--) return 1 @@ -132,10 +132,10 @@ parseopts() { ;; 1) # --longopt=optarg - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then OPTRET+=("--$opt" "$optarg") # --longopt optarg - elif [[ $2 ]]; then + elif (( $# > 1 )); then OPTRET+=("--$opt" "$2" ) shift # parse failure diff --git a/test/scripts/parseopts_test.sh b/test/scripts/parseopts_test.sh index 9674c6a6..1d76f1ad 100755 --- a/test/scripts/parseopts_test.sh +++ b/test/scripts/parseopts_test.sh @@ -31,7 +31,7 @@ tap_parse() { unset OPTRET } -tap_plan 50 +tap_plan 52 # usage: tap_parse <expected result> <token count> test-params... # a failed tap_parse will match only the end of options marker '--' @@ -111,4 +111,7 @@ tap_parse '--force --' 2 --force # exact match on possible stem (opt has optarg) tap_parse '--clean foo --' 3 --clean=foo +# all possible ways to specify empty optargs +tap_parse '--key --pkg -p --' 7 --key '' --pkg='' -p '' + tap_finish -- 2.23.0
On 25/10/19 11:48 am, Ethan Sommer wrote:
Previously parseopts checked if there was an argument by checking that the string was non-empty, resulting in empty arguments being incorrectly considered non-existent. This change makes parseopts check if arguments exist at all, rather than checking that they are non-empty
Signed-off-by: Ethan Sommer <e5ten.arch@gmail.com>
When parsing options, is there any case where an empty argument is functionally different from a missing one? I.e. what is the justification for considering empty arguments?
--- scripts/libmakepkg/util/parseopts.sh.in | 8 ++++---- test/scripts/parseopts_test.sh | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/libmakepkg/util/parseopts.sh.in b/scripts/libmakepkg/util/parseopts.sh.in index c056cb1e..8fb862d8 100644 --- a/scripts/libmakepkg/util/parseopts.sh.in +++ b/scripts/libmakepkg/util/parseopts.sh.in @@ -102,7 +102,7 @@ parseopts() { 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 + elif (( i == ${#1} - 1 && $# > 1 )); then OPTRET+=("$2") shift break @@ -121,7 +121,7 @@ parseopts() { case $? in 0) # parse failure - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then printf "${0##*/}: $(gettext "option '%s' does not allow an argument")\n" "--$opt" >&2 OPTRET=(--) return 1 @@ -132,10 +132,10 @@ parseopts() { ;; 1) # --longopt=optarg - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then OPTRET+=("--$opt" "$optarg") # --longopt optarg - elif [[ $2 ]]; then + elif (( $# > 1 )); then OPTRET+=("--$opt" "$2" ) shift # parse failure diff --git a/test/scripts/parseopts_test.sh b/test/scripts/parseopts_test.sh index 9674c6a6..1d76f1ad 100755 --- a/test/scripts/parseopts_test.sh +++ b/test/scripts/parseopts_test.sh @@ -31,7 +31,7 @@ tap_parse() { unset OPTRET }
-tap_plan 50 +tap_plan 52
# usage: tap_parse <expected result> <token count> test-params... # a failed tap_parse will match only the end of options marker '--' @@ -111,4 +111,7 @@ tap_parse '--force --' 2 --force # exact match on possible stem (opt has optarg) tap_parse '--clean foo --' 3 --clean=foo
+# all possible ways to specify empty optargs +tap_parse '--key --pkg -p --' 7 --key '' --pkg='' -p '' + tap_finish
When parsing options, is there any case where an empty argument is functionally different from a missing one?
I.e. what is the justification for considering empty arguments? makepkg --config='' --clean ==> ERROR: --clean not found. Aborting...
I think that this isn't correct and makes it less clear than it should be why the error is happening, i.e. the option parsing is incorrect when the options passed should be completely valid to the parser, and only the program itself should consider them incorrect.
On 25/10/19 11:48 am, Ethan Sommer wrote:
Previously parseopts checked if there was an argument by checking that the string was non-empty, resulting in empty arguments being incorrectly considered non-existent. This change makes parseopts check if arguments exist at all, rather than checking that they are non-empty
Signed-off-by: Ethan Sommer <e5ten.arch@gmail.com>
I'm pushing the optional argument patch to master. Can you rebase this on top of it. Thanks, Allan
--- scripts/libmakepkg/util/parseopts.sh.in | 8 ++++---- test/scripts/parseopts_test.sh | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/scripts/libmakepkg/util/parseopts.sh.in b/scripts/libmakepkg/util/parseopts.sh.in index c056cb1e..8fb862d8 100644 --- a/scripts/libmakepkg/util/parseopts.sh.in +++ b/scripts/libmakepkg/util/parseopts.sh.in @@ -102,7 +102,7 @@ parseopts() { 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 + elif (( i == ${#1} - 1 && $# > 1 )); then OPTRET+=("$2") shift break @@ -121,7 +121,7 @@ parseopts() { case $? in 0) # parse failure - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then printf "${0##*/}: $(gettext "option '%s' does not allow an argument")\n" "--$opt" >&2 OPTRET=(--) return 1 @@ -132,10 +132,10 @@ parseopts() { ;; 1) # --longopt=optarg - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then OPTRET+=("--$opt" "$optarg") # --longopt optarg - elif [[ $2 ]]; then + elif (( $# > 1 )); then OPTRET+=("--$opt" "$2" ) shift # parse failure diff --git a/test/scripts/parseopts_test.sh b/test/scripts/parseopts_test.sh index 9674c6a6..1d76f1ad 100755 --- a/test/scripts/parseopts_test.sh +++ b/test/scripts/parseopts_test.sh @@ -31,7 +31,7 @@ tap_parse() { unset OPTRET }
-tap_plan 50 +tap_plan 52
# usage: tap_parse <expected result> <token count> test-params... # a failed tap_parse will match only the end of options marker '--' @@ -111,4 +111,7 @@ tap_parse '--force --' 2 --force # exact match on possible stem (opt has optarg) tap_parse '--clean foo --' 3 --clean=foo
+# all possible ways to specify empty optargs +tap_parse '--key --pkg -p --' 7 --key '' --pkg='' -p '' + tap_finish
Previously parseopts checked if there was an argument by checking that the string was non-empty, resulting in empty arguments being incorrectly considered non-existent. This change makes parseopts check if arguments exist at all, rather than checking that they are non-empty Signed-off-by: Ethan Sommer <e5ten.arch@gmail.com> --- scripts/libmakepkg/util/parseopts.sh.in | 8 ++++---- test/scripts/parseopts_test.sh | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/scripts/libmakepkg/util/parseopts.sh.in b/scripts/libmakepkg/util/parseopts.sh.in index 38ff7f62..698f2acb 100644 --- a/scripts/libmakepkg/util/parseopts.sh.in +++ b/scripts/libmakepkg/util/parseopts.sh.in @@ -103,7 +103,7 @@ parseopts() { OPTRET+=("-$opt" "${1:i+1}") break # if we're at the end, grab the the next positional, if it exists - elif (( i == ${#1} - 1 )) && [[ $2 ]]; then + elif (( i == ${#1} - 1 && $# > 1 )); then OPTRET+=("-$opt" "$2") shift break @@ -144,7 +144,7 @@ parseopts() { case $? in 0) # parse failure - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then printf "${0##*/}: $(gettext "option '%s' does not allow an argument")\n" "--$opt" >&2 OPTRET=(--) return 1 @@ -155,10 +155,10 @@ parseopts() { ;; 1) # --longopt=optarg - if [[ $optarg ]]; then + if [[ $1 = *=* ]]; then OPTRET+=("--$opt" "$optarg") # --longopt optarg - elif [[ $2 ]]; then + elif (( $# > 1 )); then OPTRET+=("--$opt" "$2" ) shift # parse failure diff --git a/test/scripts/parseopts_test.sh b/test/scripts/parseopts_test.sh index 8f1ea1f3..3d706be8 100755 --- a/test/scripts/parseopts_test.sh +++ b/test/scripts/parseopts_test.sh @@ -31,7 +31,7 @@ tap_parse() { unset OPTRET } -tap_plan 54 +tap_plan 56 # usage: tap_parse <expected result> <token count> test-params... # a failed tap_parse will match only the end of options marker '--' @@ -117,4 +117,7 @@ tap_parse '--opt= --opt=foo --opt --' 4 --opt= --opt=foo --opt # short opt with and without optional arg, and non-option arg tap_parse '-b=foo -A -b -- foo' 5 -bfoo -Ab foo +# all possible ways to specify empty optargs +tap_parse '--key --pkg -p --' 7 --key '' --pkg='' -p '' + tap_finish -- 2.24.0
On 5/11/19 2:36 pm, Ethan Sommer wrote:
Previously parseopts checked if there was an argument by checking that the string was non-empty, resulting in empty arguments being incorrectly considered non-existent. This change makes parseopts check if arguments exist at all, rather than checking that they are non-empty
Signed-off-by: Ethan Sommer <e5ten.arch@gmail.com> ---
Thanks! Pushed. A
participants (2)
-
Allan McRae
-
Ethan Sommer