On Tue, Dec 11, 2018 at 01:42:45PM +0100, SZEDER Gábor wrote:
> > 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.
It certainly looks better to me. It also makes sense to me to validate
the options before forking/logging, though I suppose one could argue the
opposite.
I wonder why we didn't do it this way in the beginning (i.e., why the
tee bits were all handled separately before the parsing phase). I guess
just because we have to pass the options down to the sub-process.
> 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
OK, so this is set for the unstuck options, which then pick up the
option in the next loop iteration. That's perhaps less gross than my
"re-build the options with set --" trick.
A simple variable set is enough for "-r". In theory we could make this:
if test -n "$handle_unstuck_arg"
then
eval "$handle_unstuck_arg \$1"
fi
...
-r)
handle_unstuck_arg=handle_opt_r ;;
and handle_opt_r() could do whatever it wants. But I don't really
foresee us adding a lot of new options (in fact, given that this is just
the internal tests, I am tempted to say that we should just make it
"-r<arg>" for the sake of simplicity and consistency. But maybe somebody
would be annoyed. I have never used "-r" ever myself).
-Peff
