Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program

From: Jeff King <peff@peff.net>
Date: Sep 28, 2010, 13:23 UTC
Message-ID: <20100928132356.GA5829@sigill.intra.peff.net>
In-Reply-To: <20100928120722.GA29525@landau.phys.spbu.ru>

On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:

Show 16 quoted lines

> > Granted, echo is forgiving and will concatenate the arguments it gets with
> > a space in between, but you would either want to either:
> > 
> >  (1) make it more explicit that helper gets only one argument, by saying
> >      "$1" instead of "$@", in all places in the helper script; or
> > 
> >  (2) if you are planning to make 'helper' capable of handling multiple
> >      input files, show the error message for the ones that are not binary
> >      (you would probably need a loop for that).
> > 
> > I think (1) would be sufficient in this case.
> 
> I too think (1) is right. It was just that originally there was $@
> (which I now understand was wrong). So ok to apply the following patch
> on top of this series? (I assume it's ok, and will repost the whole
> thing)

No, "helper" is supposed to be able to take multiple arguments, at least in t4042. See the "changing textconv invalidates cache" test. The extra argument comes from the user, not from git.

>  t/t4042-diff-textconv-caching.sh |    4 ++--

Why are we touching t4042 at all in this series? We are not actually adding any tests to it, AFAICT.

>  t/t8006-blame-textconv.sh        |    4 ++--
>  t/t8007-cat-file-textconv.sh     |    4 ++--

These ones simply copied the helper script, and could be switched to use $1.

-Peff