[PATCH] test-lib: allow short options to be stacked

[PATCH] test-lib: allow short options to be stacked

[Matheus Tavares]

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

Re: [PATCH] test-lib: allow short options to be stacked

[Junio C Hamano]

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.

Re: [PATCH] test-lib: allow short options to be stacked

[Jeff King]

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

Re: [PATCH] test-lib: allow short options to be stacked

[Johannes Sixt]

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

Re: [PATCH] test-lib: allow short options to be stacked

[Matheus Tavares]

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.

Re: [PATCH] test-lib: allow short options to be stacked

[Matheus Tavares Bernardino]

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).

Re: [PATCH] test-lib: allow short options to be stacked

[Matheus Tavares Bernardino]

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/

Re: [PATCH] test-lib: allow short options to be stacked

[Junio C Hamano]

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.

[PATCH v2] test-lib: allow short options to be bundled

[Matheus Tavares]

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

Re: [PATCH v2] test-lib: allow short options to be bundled

[Junio C Hamano]

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

Re: [PATCH] test-lib: allow short options to be stacked

[Junio C Hamano]

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.

Re: [PATCH v2] test-lib: allow short options to be bundled

[Matheus Tavares Bernardino]

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.

Re: [PATCH] test-lib: allow short options to be stacked

[Jeff King]

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

Re: [PATCH] test-lib: allow short options to be stacked

[SZEDER Gábor]

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.

[PATCH v3] test-lib: allow short options to be bundled

[Matheus Tavares]

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

Re: [PATCH v3] test-lib: allow short options to be bundled

[Junio C Hamano]

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.