Re: [PATCH v2 2/7] test-lib: parse some --options earlier

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 11, 2018, 12:42 UTC
Message-ID: <20181211124245.GT30222@szeder.dev>
In-Reply-To: <20181211110919.GC8452@sigill.intra.peff.net>

On Tue, Dec 11, 2018 at 06:09:19AM -0500, Jeff King wrote:

Show 27 quoted lines

> On Sun, Dec 09, 2018 at 11:56:23PM +0100, SZEDER Gábor wrote:
> 
> > 'test-lib.sh' looks for the presence of certain options like '--tee'
> > and '--verbose-log', so it can execute the test script again to save
> > its standard output and error.  This happens way before the actual
> > option parsing loop, and the condition looking for these options looks
> > a bit odd, too.  This patch series will add two more options to look
> > out for, and, in addition, will have to extract these options' stuck
> > arguments (i.e. '--opt=arg') as well.
> > 
> > Add a proper option parsing loop to check these select options early
> > in 'test-lib.sh', making this early option checking more readable and
> > keeping those later changes in this series simpler.  Use a 'for opt in
> > "$@"' loop to iterate over the options to preserve "$@" intact, so
> > options like '--verbose-log' can execute the test script again with
> > all the original options.
> > 
> > As an alternative, we could parse all options early, but there are
> > options that do require an _unstuck_ argument, which is tricky to
> > handle properly in such a for loop, and the resulting complexity is,
> > in my opinion, worse than having this extra, partial option parsing
> > loop.
> 
> In general, I'm not wild about having multiple option-parsing loops that
> skip the normal left-to-right parsing, since it introduces funny corner
> cases (like "-foo --bar" which should be the same as "--foo=--bar"
> instead thinking that "--bar" was passed as an option).

Yeah, that's already an "issue" in the current implementation as well, though there are no such options that require options as argument.

Show 7 quoted lines

> But looking at what this is replacing:
> 
> > -case "$GIT_TEST_TEE_STARTED, $* " in
> > -done,*)
> > -	# do not redirect again
> > -	;;
> > -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)

Anyway, I had another crack at turning the current option parsing loop into a for loop keeping $@ intact, and the results don't look all that bad this time. Note that this diff below only does the while -> for conversion, but leaves the loop where it is, so the changes are easily visible. The important bits are the conditions at the beginning of the loop and after the loop, and the handling of '-r'; the rest is mostly s/shift// and sort-of s/$1/$opt/.

Thoughts?  Is it better than two loops?  I think it's better.

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a3f7930a3..efdb6be3c8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -264,58 +264,65 @@ test "x$TERM" != "xdumb" && (
 	) &&
 	color=t
 
-while test "$#" -ne 0
+store_arg_to=
+prev_opt=
+for opt
 do
-	case "$1" in
+	if test -n "$store_arg_to"
+	then
+		eval $store_arg_to=\$opt
+		store_arg_to=
+		prev_opt=
+		continue
+	fi
+	case "$opt" in
 	-d|--d|--de|--deb|--debu|--debug)
-		debug=t; shift ;;
+		debug=t ;;
 	-i|--i|--im|--imm|--imme|--immed|--immedi|--immedia|--immediat|--immediate)
-		immediate=t; shift ;;
+		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; shift ;;
+		GIT_TEST_LONG=t; export GIT_TEST_LONG ;;
 	-r)
-		shift; test "$#" -ne 0 || {
-			echo 'error: -r requires an argument' >&2;
-			exit 1;
-		}
-		run_list=$1; shift ;;
+		store_arg_to=run_list
+		prev_opt=-r
+		;;
 	--run=*)
-		run_list=${1#--*=}; shift ;;
+		run_list=${opt#--*=} ;;
 	-h|--h|--he|--hel|--help)
-		help=t; shift ;;
+		help=t ;;
 	-v|--v|--ve|--ver|--verb|--verbo|--verbos|--verbose)
-		verbose=t; shift ;;
+		verbose=t ;;
 	--verbose-only=*)
-		verbose_only=${1#--*=}
-		shift ;;
+		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; shift ;;
+		test -z "$HARNESS_ACTIVE" && quiet=t ;;
 	--with-dashes)
-		with_dashes=t; shift ;;
+		with_dashes=t ;;
 	--no-color)
-		color=; shift ;;
+		color= ;;
 	--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
 		valgrind=memcheck
-		shift ;;
+		;;
 	--valgrind=*)
-		valgrind=${1#--*=}
-		shift ;;
+		valgrind=${opt#--*=}
+		;;
 	--valgrind-only=*)
-		valgrind_only=${1#--*=}
-		shift ;;
+		valgrind_only=${opt#--*=}
+		;;
 	--tee)
-		shift ;; # was handled already
+		;; # was handled already
 	--root=*)
-		root=${1#--*=}
-		shift ;;
+		root=${opt#--*=}
+		;;
 	--chain-lint)
 		GIT_TEST_CHAIN_LINT=1
-		shift ;;
+		;;
 	--no-chain-lint)
 		GIT_TEST_CHAIN_LINT=0
-		shift ;;
+		;;
 	-x)
 		# Some test scripts can't be reliably traced  with '-x',
 		# unless the test is run with a Bash version supporting
@@ -335,15 +342,21 @@ do
 		else
 			echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
 		fi
-		shift ;;
+		;;
 	-V|--verbose-log)
 		verbose_log=t
-		shift ;;
+		;;
 	*)
-		echo "error: unknown test option '$1'" >&2; exit 1 ;;
+		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
+	exit 1
+fi
+
 if test -n "$valgrind_only"
 then
 	test -z "$valgrind" && valgrind=memcheck