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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Dec 30, 2018, 19:04 UTC
Message-ID: <20181230190419.GB6120@szeder.dev>
In-Reply-To: <20181217214436.GB14251@sigill.intra.peff.net>

On Mon, Dec 17, 2018 at 04:44:36PM -0500, Jeff King wrote:

> On Tue, Dec 11, 2018 at 01:42:45PM +0100, SZEDER Gábor wrote:

Show 39 quoted lines

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

Yeah, I would refrain from making it too general and fancy with a callback function for now, when there is only a single option that could use it.

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

I didn't even know what '-r' does...

And I agree that changing it to '-r<arg>' would be the best, but this patch series is about adding '--stress', so changing how '-r' gets its mandatory argument (and potentially annoying someone) is beyond the scope, I would say.