| Message ID | 48c28683412e3e0803d4c7189a6d66daddcdc580.1584759277.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | test-lib: allow short options to be stacked | expand |
Matheus Tavares <matheus.bernardino@usp.br> writes: > +parse_option () { > + local opt="$@" I do not think there is any context in which var="$@" makes sense in shell script (var="$*" is understandable, though). Did you mean opt=$1 here? > +# Parse options while taking care to leave $@ intact, so we will still > +# have all the original command line options when executing the test > +# script again for '--tee' and '--verbose-log' below. The phrase "below" no longer makes much sense after moving lines around, does it? > +store_arg_to= > +prev_opt= > +for opt > +do > + if test -n "$store_arg_to" > + then > + eval $store_arg_to=\$opt > + store_arg_to= > + prev_opt= > + continue > + fi > + > + case "$opt" in > + --*) > + parse_option "$opt" ;; > + -?*) > + # stacked short options must be fed separately to parse_option Are you calling concatenated short options, e.g. "-abc", as "stacked"? It sounds like a very unusual phrasing, at least to me. > + for c in $(echo "${opt#-}" | sed 's/./& /g') > + do > + parse_option "-$c" > + done > + ;; > + *) > + echo "error: unknown test option '$opt'" >&2; exit 1 ;; > + esac > > prev_opt=$opt > done I am personally not very enthused (the line counts vs benefit does not feel so great), but as long as it works correctly and maintainable, I won't complain too much.
On Sat, Mar 21, 2020 at 12:07:05AM -0300, Matheus Tavares wrote: > When debugging a test (or a set of tests), it's common to execute it > with some combination of short options, such as: > > $ ./txxx-testname.sh -d -x -i > > In cases like this, CLIs usually allow the short options to be stacked > in a single argument, for convenience and agility. Let's add this > feature to test-lib, allowing the above command to be run as: > > $ ./txxx-testname.sh -dxi > (or any other permutation, e.g. '-ixd') Yay. I've grown accustomed to the lack of this feature in the test scripts, but I'll be happy to have it. :) Most getopt implementations I've seen call this "bundling" rather than "stacking" (I don't care too much either way, but Junio mentioned being confused at the name). > + case "$opt" in > + --*) > + parse_option "$opt" ;; > + -?*) > + # stacked short options must be fed separately to parse_option > + for c in $(echo "${opt#-}" | sed 's/./& /g') > + do > + parse_option "-$c" > + done I wondered if we could do this without the extra process. This works: opt=${opt#-} while test -n "$opt" do extra=${opt#?} this=${opt%$extra} opt=$extra parse_option "-$this" done It's a little convoluted. I'm not sure if saving a process per unbundled short option is worth it. What happens to bundled short options with arguments? I think "-r" is the only one. We don't allow "stuck" short options like "-r5", so we don't have to worry about feeding non-option bits to parse_option(). It looks like we'd only examine $store_arg_to outside of the short-option loop, so we'd treat: ./t1234-foo.sh -vrix 5 the same as: ./t1234-foo.sh -v -r 5 -i -x which seems reasonable. But: ./t1234-foo.sh -rr 5 6 would get garbled. We'd come out of "-rr" parsing with $store_arg_to set, but only grab the first argument. I think I could live with that, considering this is just the test scripts. Fixing it would require store_arg_to becoming a space-separated list. -Peff
Am 21.03.20 um 04:07 schrieb Matheus Tavares: > + > +# Parse options while taking care to leave $@ intact, so we will still > +# have all the original command line options when executing the test > +# script again for '--tee' and '--verbose-log' below. > +store_arg_to= > +prev_opt= > +for opt > +do > + if test -n "$store_arg_to" > + then > + eval $store_arg_to=\$opt > + store_arg_to= > + prev_opt= > + continue > + fi > + > + case "$opt" in > + --*) Can we please have a case arm for -? here: --* | -?) so that we do not pay the price of many sub-processes when options are _not_ stacked? > + parse_option "$opt" ;; > + -?*) > + # stacked short options must be fed separately to parse_option > + for c in $(echo "${opt#-}" | sed 's/./& /g') > + do > + parse_option "-$c" > + done > + ;; > + *) > + echo "error: unknown test option '$opt'" >&2; exit 1 ;; > + esac > > prev_opt=$opt > done > -- Hannes
On Sat, Mar 21, 2020 at 1:53 AM Junio C Hamano <gitster@pobox.com> wrote: > Matheus Tavares <matheus.bernardino@usp.br> writes: > > > +parse_option () { > > + local opt="$@" > > I do not think there is any context in which var="$@" makes sense in > shell script (var="$*" is understandable, though). > > Did you mean opt=$1 here? Right, it should be $1. Thanks. > > +# Parse options while taking care to leave $@ intact, so we will still > > +# have all the original command line options when executing the test > > +# script again for '--tee' and '--verbose-log' below. > > The phrase "below" no longer makes much sense after moving lines > around, does it? Oh, I thought "below" referred to the later usage of $@ (when --tee or --verbose-log are set). I.e. not the parsing section we moved up, but this one: elif test -n "$tee" then ... (GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1; Maybe change "below" for "later in the code"? > > +store_arg_to= > > +prev_opt= > > +for opt > > +do > > + if test -n "$store_arg_to" > > + then > > + eval $store_arg_to=\$opt > > + store_arg_to= > > + prev_opt= > > + continue > > + fi > > + > > + case "$opt" in > > + --*) > > + parse_option "$opt" ;; > > + -?*) > > + # stacked short options must be fed separately to parse_option > > Are you calling concatenated short options, e.g. "-abc", as > "stacked"? It sounds like a very unusual phrasing, at least to me. Yeah, I wasn't really sure about the naming for this. Thanks, "concatenated" (or "bundled", as Peff suggested in another reply) does sound better.
On Sat, Mar 21, 2020 at 3:26 AM Jeff King <peff@peff.net> wrote: > > On Sat, Mar 21, 2020 at 12:07:05AM -0300, Matheus Tavares wrote: > > > In cases like this, CLIs usually allow the short options to be stacked > > in a single argument, for convenience and agility. Let's add this > > feature to test-lib, allowing the above command to be run as: > > Most getopt implementations I've seen call this "bundling" rather than > "stacking" (I don't care too much either way, but Junio mentioned being > confused at the name). Yeah, "stacking" wasn't the best word choice. I will replace it by "bundling" then, thanks. > > > + case "$opt" in > > + --*) > > + parse_option "$opt" ;; > > + -?*) > > + # stacked short options must be fed separately to parse_option > > + for c in $(echo "${opt#-}" | sed 's/./& /g') > > + do > > + parse_option "-$c" > > + done > > I wondered if we could do this without the extra process. This works: > > opt=${opt#-} > while test -n "$opt" > do > extra=${opt#?} > this=${opt%$extra} > opt=$extra > parse_option "-$this" > done > > It's a little convoluted. I'm not sure if saving a process per unbundled > short option is worth it. I quite liked this alternative with builtins. It's a little more verbose, but it remains very clear. > What happens to bundled short options with arguments? I think "-r" is > the only one. We don't allow "stuck" short options like "-r5", so we > don't have to worry about feeding non-option bits to parse_option(). It > looks like we'd only examine $store_arg_to outside of the short-option > loop, so we'd treat: > > ./t1234-foo.sh -vrix 5 > > the same as: > > ./t1234-foo.sh -v -r 5 -i -x Yes. I just thought, though, that if another "short option with arguments" gets added in the future, the bundle would not work correctly. I don't think we need to be prepared for such a scenario now, but ... > which seems reasonable. But: > > ./t1234-foo.sh -rr 5 6 > > would get garbled. ... we could prohibit using more than one "short option with arguments" in the same bundle. This would not only solve the problem for "-rr 5 6"[1] but also for the scenario of future new options. And it's quite simple to implement, we just have to check whether $store_arg_to is set before setting it to another value. I'll try that for v2. [1]: Someone that used '-rr 5 6' might have wanted the script to run *both* tests 5 and 6. But I don't think we need to support that now, since '-r 5 -r 6' doesn't do that as well (instead, the last value overrides all previous ones).
On Sat, Mar 21, 2020 at 5:55 AM Johannes Sixt <j6t@kdbg.org> wrote: > > Am 21.03.20 um 04:07 schrieb Matheus Tavares: > > > > +for opt > > +do > > + if test -n "$store_arg_to" > > + then > > + eval $store_arg_to=\$opt > > + store_arg_to= > > + prev_opt= > > + continue > > + fi > > + > > + case "$opt" in > > + --*) > > Can we please have a case arm for -? here: > > --* | -?) > > so that we do not pay the price of many sub-processes when options are > _not_ stacked? Makes sense, thanks. However, using Peff's suggestion[1] for the character iteration already eliminates the need for extra processes. So it will cover this case as well. [1]: https://lore.kernel.org/git/20200321062611.GA1441446@coredump.intra.peff.net/
Jeff King <peff@peff.net> writes: > I wondered if we could do this without the extra process. This works: > > opt=${opt#-} > while test -n "$opt" > do > extra=${opt#?} > this=${opt%$extra} > opt=$extra > parse_option "-$this" > done > > It's a little convoluted. I'm not sure if saving a process per unbundled > short option is worth it. I was wondering if I should suggest something similar to the above when I wrote my response. Yours actually does not look as bad as what mine would have been ;-) > What happens to bundled short options with arguments? I think "-r" is > the only one. We don't allow "stuck" short options like "-r5", so we > don't have to worry about feeding non-option bits to parse_option(). It > looks like we'd only examine $store_arg_to outside of the short-option > loop, so we'd treat: > > ./t1234-foo.sh -vrix 5 > > the same as: > > ./t1234-foo.sh -v -r 5 -i -x > > which seems reasonable. But: > > ./t1234-foo.sh -rr 5 6 > > would get garbled. And also we declare we will never add any option that takes an argument with this patch? ... Ah, no, it is just store_arg_to is taking a short-cut assuming the current lack of bundled options. OK, so yeah, I am not sure if this half-way "solution" is worth it. It is just the test scripts, sure, that we have this strange limitation that "-rr 5 6" is not parsed correctly (i.e. "do not use the bundled form if -r is involved" is something developers can live with), but then it is just the test scripts so "do not use the bundled form at al" is not too bad, either. It is just one less thing for developers to remember, compared to having to remember "you can, but except for this special case". So, I dunno. Thanks.
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > Makes sense, thanks. However, using Peff's suggestion[1] for the > character iteration already eliminates the need for extra processes. Even without an extra process, having to strip "-", assign an empty string to $extra, and then strip that empty string from the tail to come up with a single letter in $this, all are consuming cycles. Even though these wasted cycles are now much smaller, having an arm that specifically catches unbundled case and avoid doing anything extra makes the codeflow clear, I would think.
On Sat, Mar 21, 2020 at 03:50:55PM -0300, Matheus Tavares Bernardino wrote: > > which seems reasonable. But: > > > > ./t1234-foo.sh -rr 5 6 > > > > would get garbled. > > ... we could prohibit using more than one "short option with > arguments" in the same bundle. This would not only solve the problem > for "-rr 5 6"[1] but also for the scenario of future new options. And > it's quite simple to implement, we just have to check whether > $store_arg_to is set before setting it to another value. I'll try that > for v2. Yeah, I'd be perfectly happy with that. This bundling is a new format that did not exist before, so we are not taking away anything you could previously do. As long as we don't produce a wrong or confusing result (and instead say "don't do that; we don't support it", anybody else is free to come along later and make it work. :) > [1]: Someone that used '-rr 5 6' might have wanted the script to run > *both* tests 5 and 6. But I don't think we need to support that now, > since '-r 5 -r 6' doesn't do that as well (instead, the last value > overrides all previous ones). Heh, that's what I assumed "-r 5 -r 6" would do, but I guess it goes to show that I do not use that option very much. :) -Peff
On Sat, Mar 21, 2020 at 03:50:55PM -0300, Matheus Tavares Bernardino wrote: > [1]: Someone that used '-rr 5 6' might have wanted the script to run > *both* tests 5 and 6. But I don't think we need to support that now, > since '-r 5 -r 6' doesn't do that as well (instead, the last value > overrides all previous ones). Well, that '-r 5 -r 6' should be written as '-r 5,6', but it shouldn't be terribly difficult to concatenate the args of multiple '-r' options with a comma, I suppose. But '-rr 5 6' just looks wrong.
diff --git a/t/README b/t/README index 9afd61e3ca..c3cf8f617b 100644 --- a/t/README +++ b/t/README @@ -69,7 +69,8 @@ You can also run each test individually from command line, like this: You can pass --verbose (or -v), --debug (or -d), and --immediate (or -i) command line argument to the test, or by setting GIT_TEST_OPTS -appropriately before running "make". +appropriately before running "make". Short options can be stacked, i.e. +'-d -v' is the same as '-dv'. -v:: --verbose:: diff --git a/t/test-lib.sh b/t/test-lib.sh index 0ea1e5a05e..14363010d2 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -72,119 +72,137 @@ then if test -n "$GIT_TEST_INSTALLED" then echo >&2 "error: there is no working Git at '$GIT_TEST_INSTALLED'" else echo >&2 'error: you do not seem to have built git yet.' fi exit 1 fi -# Parse options while taking care to leave $@ intact, so we will still -# have all the original command line options when executing the test -# script again for '--tee' and '--verbose-log' below. -store_arg_to= -prev_opt= -for opt -do - if test -n "$store_arg_to" - then - eval $store_arg_to=\$opt - store_arg_to= - prev_opt= - continue - fi +parse_option () { + local opt="$@" case "$opt" in -d|--d|--de|--deb|--debu|--debug) debug=t ;; -i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate) immediate=t ;; -l|--l|--lo|--lon|--long|--long-|--long-t|--long-te|--long-tes|--long-test|--long-tests) GIT_TEST_LONG=t; export GIT_TEST_LONG ;; -r) store_arg_to=run_list ;; --run=*) run_list=${opt#--*=} ;; -h|--h|--he|--hel|--help) help=t ;; -v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose) verbose=t ;; --verbose-only=*) verbose_only=${opt#--*=} ;; -q|--q|--qu|--qui|--quie|--quiet) # Ignore --quiet under a TAP::Harness. Saying how many tests # passed without the ok/not ok details is always an error. test -z "$HARNESS_ACTIVE" && quiet=t ;; --with-dashes) with_dashes=t ;; --no-bin-wrappers) no_bin_wrappers=t ;; --no-color) color= ;; --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind) valgrind=memcheck tee=t ;; --valgrind=*) valgrind=${opt#--*=} tee=t ;; --valgrind-only=*) valgrind_only=${opt#--*=} tee=t ;; --tee) tee=t ;; --root=*) root=${opt#--*=} ;; --chain-lint) GIT_TEST_CHAIN_LINT=1 ;; --no-chain-lint) GIT_TEST_CHAIN_LINT=0 ;; -x) trace=t ;; -V|--verbose-log) verbose_log=t tee=t ;; --write-junit-xml) write_junit_xml=t ;; --stress) stress=t ;; --stress=*) echo "error: --stress does not accept an argument: '$opt'" >&2 echo "did you mean --stress-jobs=${opt#*=} or --stress-limit=${opt#*=}?" >&2 exit 1 ;; --stress-jobs=*) stress=t; stress=${opt#--*=} case "$stress" in *[!0-9]*|0*|"") echo "error: --stress-jobs=<N> requires the number of jobs to run" >&2 exit 1 ;; *) # Good. ;; esac ;; --stress-limit=*) stress=t; stress_limit=${opt#--*=} case "$stress_limit" in *[!0-9]*|0*|"") echo "error: --stress-limit=<N> requires the number of repetitions" >&2 exit 1 ;; *) # Good. ;; esac ;; *) echo "error: unknown test option '$opt'" >&2; exit 1 ;; esac +} + +# Parse options while taking care to leave $@ intact, so we will still +# have all the original command line options when executing the test +# script again for '--tee' and '--verbose-log' below. +store_arg_to= +prev_opt= +for opt +do + if test -n "$store_arg_to" + then + eval $store_arg_to=\$opt + store_arg_to= + prev_opt= + continue + fi + + case "$opt" in + --*) + parse_option "$opt" ;; + -?*) + # stacked short options must be fed separately to parse_option + for c in $(echo "${opt#-}" | sed 's/./& /g') + do + parse_option "-$c" + done + ;; + *) + echo "error: unknown test option '$opt'" >&2; exit 1 ;; + esac prev_opt=$opt done
When debugging a test (or a set of tests), it's common to execute it with some combination of short options, such as: $ ./txxx-testname.sh -d -x -i In cases like this, CLIs usually allow the short options to be stacked in a single argument, for convenience and agility. Let's add this feature to test-lib, allowing the above command to be run as: $ ./txxx-testname.sh -dxi (or any other permutation, e.g. '-ixd') Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- t/README | 3 ++- t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 15 deletions(-)