| Message ID | 7a6a8197dcd58e8690892d03cb904dd1eec5d7c1.1584818457.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] test-lib: allow short options to be bundled | expand |
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
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.
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
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(-)