* [PATCH] test-lib: allow short options to be stacked @ 2020-03-21 3:07 Matheus Tavares 2020-03-21 4:53 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Matheus Tavares @ 2020-03-21 3:07 UTC (permalink / raw) To: git; +Cc: gitster, SZEDER Gábor, Johannes Schindelin 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(-) 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 -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 3:07 [PATCH] test-lib: allow short options to be stacked Matheus Tavares @ 2020-03-21 4:53 ` Junio C Hamano 2020-03-21 17:27 ` Matheus Tavares 2020-03-21 6:26 ` Jeff King ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2020-03-21 4:53 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, SZEDER Gábor, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 4:53 ` Junio C Hamano @ 2020-03-21 17:27 ` Matheus Tavares 0 siblings, 0 replies; 16+ messages in thread From: Matheus Tavares @ 2020-03-21 17:27 UTC (permalink / raw) To: gitster; +Cc: Johannes.Schindelin, git, szeder.dev 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 3:07 [PATCH] test-lib: allow short options to be stacked Matheus Tavares 2020-03-21 4:53 ` Junio C Hamano @ 2020-03-21 6:26 ` Jeff King 2020-03-21 18:50 ` Matheus Tavares Bernardino 2020-03-21 18:57 ` Junio C Hamano 2020-03-21 8:55 ` Johannes Sixt 2020-03-21 19:57 ` [PATCH v2] test-lib: allow short options to be bundled Matheus Tavares 3 siblings, 2 replies; 16+ messages in thread From: Jeff King @ 2020-03-21 6:26 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, gitster, SZEDER Gábor, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 6:26 ` Jeff King @ 2020-03-21 18:50 ` Matheus Tavares Bernardino 2020-03-22 6:49 ` Jeff King 2020-03-22 8:14 ` SZEDER Gábor 2020-03-21 18:57 ` Junio C Hamano 1 sibling, 2 replies; 16+ messages in thread From: Matheus Tavares Bernardino @ 2020-03-21 18:50 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, SZEDER Gábor, Johannes Schindelin 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). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 18:50 ` Matheus Tavares Bernardino @ 2020-03-22 6:49 ` Jeff King 2020-03-22 8:14 ` SZEDER Gábor 1 sibling, 0 replies; 16+ messages in thread From: Jeff King @ 2020-03-22 6:49 UTC (permalink / raw) To: Matheus Tavares Bernardino Cc: git, Junio C Hamano, SZEDER Gábor, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 18:50 ` Matheus Tavares Bernardino 2020-03-22 6:49 ` Jeff King @ 2020-03-22 8:14 ` SZEDER Gábor 1 sibling, 0 replies; 16+ messages in thread From: SZEDER Gábor @ 2020-03-22 8:14 UTC (permalink / raw) To: Matheus Tavares Bernardino Cc: Jeff King, git, Junio C Hamano, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 6:26 ` Jeff King 2020-03-21 18:50 ` Matheus Tavares Bernardino @ 2020-03-21 18:57 ` Junio C Hamano 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2020-03-21 18:57 UTC (permalink / raw) To: Jeff King; +Cc: Matheus Tavares, git, SZEDER Gábor, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 3:07 [PATCH] test-lib: allow short options to be stacked Matheus Tavares 2020-03-21 4:53 ` Junio C Hamano 2020-03-21 6:26 ` Jeff King @ 2020-03-21 8:55 ` Johannes Sixt 2020-03-21 18:55 ` Matheus Tavares Bernardino 2020-03-21 19:57 ` [PATCH v2] test-lib: allow short options to be bundled Matheus Tavares 3 siblings, 1 reply; 16+ messages in thread From: Johannes Sixt @ 2020-03-21 8:55 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, gitster, SZEDER Gábor, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 8:55 ` Johannes Sixt @ 2020-03-21 18:55 ` Matheus Tavares Bernardino 2020-03-21 20:11 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Matheus Tavares Bernardino @ 2020-03-21 18:55 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, Junio C Hamano, SZEDER Gábor, Johannes Schindelin 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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] test-lib: allow short options to be stacked 2020-03-21 18:55 ` Matheus Tavares Bernardino @ 2020-03-21 20:11 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2020-03-21 20:11 UTC (permalink / raw) To: Matheus Tavares Bernardino Cc: Johannes Sixt, git, SZEDER Gábor, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] test-lib: allow short options to be bundled 2020-03-21 3:07 [PATCH] test-lib: allow short options to be stacked Matheus Tavares ` (2 preceding siblings ...) 2020-03-21 8:55 ` Johannes Sixt @ 2020-03-21 19:57 ` Matheus Tavares 2020-03-21 20:07 ` Junio C Hamano 2020-03-22 15:58 ` [PATCH v3] " Matheus Tavares 3 siblings, 2 replies; 16+ messages in thread From: Matheus Tavares @ 2020-03-21 19:57 UTC (permalink / raw) To: git; +Cc: gitster, szeder.dev, Johannes.Schindelin, j6t, peff 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 bundled 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') Helped-by: Jeff King <peff@peff.net> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- Changes since v1: - Added a check for bundles containing more than one "option that requires args" (e.g. '-rr'), in which case we error-out. We could interpret '-rr 1 2' as 'run tests 1 _and_ 2', but the unbundled format, '-r 1 -r 2', is not currently interpreted like that (the last just overrides the previous). So, for simplicity, let's only forbid such bundles for now. - Used "$1" instead of "$@" to get the argument in parse_option() - Replaced occurrences of "stacked options" to "bundled options" - Eliminated spawning of extra processes in the bundled options parser - s/below/later/ in the parser loop comment, to make it clearer t/README | 3 ++- t/test-lib.sh | 62 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/t/README b/t/README index 9afd61e3ca..080bc59fc4 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 bundled, 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..dda770ec94 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -78,20 +78,24 @@ then 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" +opt_required_arg= +# $1: option string +# $2: name of the var where the arg will be stored +mark_option_requires_arg () +{ + if test -n "$opt_required_arg" then - eval $store_arg_to=\$opt - store_arg_to= - prev_opt= - continue + echo "error: options that require args cannot be bundled" \ + "together: '$opt_required_arg' and '$1'" >&2 + exit 1 fi + opt_required_arg=$1 + store_arg_to=$2 +} + +parse_option () { + local opt="$1" case "$opt" in -d|--d|--de|--deb|--debu|--debug) @@ -101,7 +105,7 @@ do -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 + mark_option_requires_arg "$opt" run_list ;; --run=*) run_list=${opt#--*=} ;; @@ -185,12 +189,42 @@ do *) 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' later. +for opt +do + if test -n "$store_arg_to" + then + eval $store_arg_to=\$opt + store_arg_to= + opt_required_arg= + continue + fi - prev_opt=$opt + case "$opt" in + --*) + parse_option "$opt" ;; + -?*) + # bundled short options must be fed separately to parse_option + opt=${opt#-} + while test -n "$opt" + do + extra=${opt#?} + this=${opt%$extra} + opt=$extra + parse_option "-$this" + done + ;; + *) + echo "error: unknown test option '$opt'" >&2; exit 1 ;; + esac done if test -n "$store_arg_to" then - echo "error: $prev_opt requires an argument" >&2 + echo "error: $opt_required_arg requires an argument" >&2 exit 1 fi -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] test-lib: allow short options to be bundled 2020-03-21 19:57 ` [PATCH v2] test-lib: allow short options to be bundled Matheus Tavares @ 2020-03-21 20:07 ` Junio C Hamano 2020-03-21 22:42 ` Matheus Tavares Bernardino 2020-03-22 15:58 ` [PATCH v3] " Matheus Tavares 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2020-03-21 20:07 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, szeder.dev, Johannes.Schindelin, j6t, peff Matheus Tavares <matheus.bernardino@usp.br> writes: > - Added a check for bundles containing more than one "option that > requires args" (e.g. '-rr'), in which case we error-out. We could > interpret '-rr 1 2' as 'run tests 1 _and_ 2', but the unbundled > format, '-r 1 -r 2', is not currently interpreted like that (the last > just overrides the previous). So, for simplicity, let's only forbid > such bundles for now. Makes sense. I think this is the best we can do at this moment. > +opt_required_arg= > +# $1: option string > +# $2: name of the var where the arg will be stored > +mark_option_requires_arg () > +{ "{" on the same line, just like you did for parse_option below. > + if test -n "$opt_required_arg" > then > + echo "error: options that require args cannot be bundled" \ > + "together: '$opt_required_arg' and '$1'" >&2 > + exit 1 > fi > + opt_required_arg=$1 > + store_arg_to=$2 > +} > + > +parse_option () { > + local opt="$1" > ... > + case "$opt" in > + --*) > + parse_option "$opt" ;; I think J6t's suggestion to the previous round still has merit here. > + -?*) > + # bundled short options must be fed separately to parse_option > + opt=${opt#-} > + while test -n "$opt" > + do > + extra=${opt#?} Take the rest of the string after stripping the first one in $extra ... > + this=${opt%$extra} ... and then strip that tail part from the end, which would give the first letter in $this. > + opt=$extra And the next round will use the remainder after taking $this out of the bundled options from the front. Makes sense. > + parse_option "-$this" > + done Thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] test-lib: allow short options to be bundled 2020-03-21 20:07 ` Junio C Hamano @ 2020-03-21 22:42 ` Matheus Tavares Bernardino 0 siblings, 0 replies; 16+ messages in thread From: Matheus Tavares Bernardino @ 2020-03-21 22:42 UTC (permalink / raw) To: Junio C Hamano Cc: git, SZEDER Gábor, Johannes Schindelin, Johannes Sixt, Jeff King On Sat, Mar 21, 2020 at 5:07 PM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares <matheus.bernardino@usp.br> writes: > > > +opt_required_arg= > > +# $1: option string > > +# $2: name of the var where the arg will be stored > > +mark_option_requires_arg () > > +{ > > "{" on the same line, just like you did for parse_option below. Thanks. Will fix. > > + if test -n "$opt_required_arg" > > then > > + echo "error: options that require args cannot be bundled" \ > > + "together: '$opt_required_arg' and '$1'" >&2 > > + exit 1 > > fi > > + opt_required_arg=$1 > > + store_arg_to=$2 > > +} > > + > > +parse_option () { > > + local opt="$1" > > ... > > + case "$opt" in > > + --*) > > + parse_option "$opt" ;; > > I think J6t's suggestion to the previous round still has merit here. Yeah, I agree with your last reply that it makes the codeflow clearer. I'll wait a bit to see if anyone else has other comments about this iteration and then send v3 with both changes. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] test-lib: allow short options to be bundled 2020-03-21 19:57 ` [PATCH v2] test-lib: allow short options to be bundled Matheus Tavares 2020-03-21 20:07 ` Junio C Hamano @ 2020-03-22 15:58 ` Matheus Tavares 2020-03-23 20:18 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Matheus Tavares @ 2020-03-22 15:58 UTC (permalink / raw) To: gitster; +Cc: git, Johannes.Schindelin, j6t, peff, szeder.dev 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 bundled 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') Note: Short options that require an argument can also be used in a bundle, in any position. So, for example, '-r 5 -x', '-xr 5' and '-rx 5' are all valid and equivalent. A special case would be having a bundle with more than one of such options. To keep things simple, this case is not allowed for now. This shouldn't be a major limitation, though, as the only short option that requires an argument today is '-r'. And concatenating '-r's as in '-rr 5 6' would probably not be very practical: its unbundled format would be '-r 5 -r 6', for which test-lib currently considers only the last argument. Therefore, if '-rr 5 6' were to be allowed, it would have the same effect as just typing '-r 6'. Note: the test-lib currently doesn't support '-r5', as an alternative for '-r 5', so the former is not supported in bundles as well. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- Changes since v2: - Fixed style in mark_option_requires_arg(), moving "{" to the same line - In the main options parser loop, joined long options and unbundled short options in the same case arm, as suggested by J6t. This provides a clearer codeflow. - Summarized, in the commit message, the approach chosen on bundling options that require args. Since this was an important point in the last rounds, I thought it would be good to include our decisions (and justification) in the commit body. t/README | 3 ++- t/test-lib.sh | 61 +++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 49 insertions(+), 15 deletions(-) diff --git a/t/README b/t/README index 9afd61e3ca..080bc59fc4 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 bundled, 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..a6178e9fac 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -78,20 +78,23 @@ then 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" +opt_required_arg= +# $1: option string +# $2: name of the var where the arg will be stored +mark_option_requires_arg () { + if test -n "$opt_required_arg" then - eval $store_arg_to=\$opt - store_arg_to= - prev_opt= - continue + echo "error: options that require args cannot be bundled" \ + "together: '$opt_required_arg' and '$1'" >&2 + exit 1 fi + opt_required_arg=$1 + store_arg_to=$2 +} + +parse_option () { + local opt="$1" case "$opt" in -d|--d|--de|--deb|--debu|--debug) @@ -101,7 +104,7 @@ do -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 + mark_option_requires_arg "$opt" run_list ;; --run=*) run_list=${opt#--*=} ;; @@ -185,12 +188,42 @@ do *) 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' later. +for opt +do + if test -n "$store_arg_to" + then + eval $store_arg_to=\$opt + store_arg_to= + opt_required_arg= + continue + fi - prev_opt=$opt + case "$opt" in + --*|-?) + parse_option "$opt" ;; + -?*) + # bundled short options must be fed separately to parse_option + opt=${opt#-} + while test -n "$opt" + do + extra=${opt#?} + this=${opt%$extra} + opt=$extra + parse_option "-$this" + done + ;; + *) + echo "error: unknown test option '$opt'" >&2; exit 1 ;; + esac done if test -n "$store_arg_to" then - echo "error: $prev_opt requires an argument" >&2 + echo "error: $opt_required_arg requires an argument" >&2 exit 1 fi -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3] test-lib: allow short options to be bundled 2020-03-22 15:58 ` [PATCH v3] " Matheus Tavares @ 2020-03-23 20:18 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2020-03-23 20:18 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, Johannes.Schindelin, j6t, peff, szeder.dev Matheus Tavares <matheus.bernardino@usp.br> writes: > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > --- > > Changes since v2: Queued. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-03-23 20:18 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-21 3:07 [PATCH] test-lib: allow short options to be stacked Matheus Tavares 2020-03-21 4:53 ` Junio C Hamano 2020-03-21 17:27 ` Matheus Tavares 2020-03-21 6:26 ` Jeff King 2020-03-21 18:50 ` Matheus Tavares Bernardino 2020-03-22 6:49 ` Jeff King 2020-03-22 8:14 ` SZEDER Gábor 2020-03-21 18:57 ` Junio C Hamano 2020-03-21 8:55 ` Johannes Sixt 2020-03-21 18:55 ` Matheus Tavares Bernardino 2020-03-21 20:11 ` Junio C Hamano 2020-03-21 19:57 ` [PATCH v2] test-lib: allow short options to be bundled Matheus Tavares 2020-03-21 20:07 ` Junio C Hamano 2020-03-21 22:42 ` Matheus Tavares Bernardino 2020-03-22 15:58 ` [PATCH v3] " Matheus Tavares 2020-03-23 20:18 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).