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).
But looking at what this is replacing:
> -case "$GIT_TEST_TEE_STARTED, $* " in
> -done,*)
> - # do not redirect again
> - ;;
> -*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
your version is easily an order of magnitude less horrible. ;)
> t/test-lib.sh | 53 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 32 insertions(+), 21 deletions(-)
This looks good to me overall, though...
> +# Parse some options early, taking care to leave $@ intact.
> +for opt
> +do
> + case "$opt" in
> + --tee)
> + tee=t ;;
> + -V|--verbose-log)
> + verbose_log=t ;;
> + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
> + valgrind=memcheck ;;
> + --valgrind=*)
> + valgrind=${opt#--*=} ;;
> + --valgrind-only=*)
> + valgrind_only=${opt#--*=} ;;
> + *)
> + # Other options will be handled later.
> + esac
> +done
> [...]
> +elif test -n "$tee" || test -n "$verbose_log" ||
> + test -n "$valgrind" || test -n "$valgrind_only"
Now that we've nicely moved the parsing up, would it make sense to put
the annotation for "this option implies --tee" with those options?
I.e., set tee=t when we see --verbose-log, which keeps all of the
verbose-log logic together?
> @@ -336,9 +344,12 @@ do
> echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
> fi
> shift ;;
> - -V|--verbose-log)
> - verbose_log=t
> - shift ;;
> + --tee|\
> + -V|--verbose-log|\
> + --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
> + --valgrind=*|\
> + --valgrind-only=*)
> + shift ;; # These options were handled already.
> *)
It's too bad there's not an easy way to selectively remove from the $@
array (which would avoid duplicating this list here).
The best I could come up with is:
-- >8 --
first=t
for i in "$@"; do
test -n "$first" && set --
first=
case "$i" in
--foo)
echo "saw foo" ;;
*)
set -- "$@" "$i" ;;
esac
done
for i in "$@"; do
echo "remainder: $i"
done
-- 8< --
but I won't be surprised if there are portability problems with
assigning $@ in the middle of a loop that iterates over it.
-Peff
