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
