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

From: Jeff King <peff@peff.net>
Date: Sep 28, 2010, 15:58 UTC
Message-ID: <20100928155845.GA10271@sigill.intra.peff.net>
In-Reply-To: <7vlj6lubg1.fsf@alter.siamese.dyndns.org>

On Tue, Sep 28, 2010 at 08:36:46AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> >> >  cat >helper <<'EOF'
> >> >  #!/bin/sh
> >> > -sed 's/^/converted: /' "$@" >helper.out
> >> > +grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> >>  ...
> > ...
> > I too think (1) is right. It was just that originally there was $@
> > (which I now understand was wrong).
> 
> Well, the original's use of "$@" is perfectly fine; it would do the right
> thing with one argument, of course, but it would do the right thing with
> more than one, too.  On the other hand, your use inside "echo" is not.

Moreover, the use of "grep" is wrong. Giving it two files, one of which has "^bin: " and one of which doesn't, will silently accept the latter. If it's going to handle multiple files, it must be a for-loop (or you could invert "grep -qv", but I think that might be getting too clever to remain readable).

Which is why I suggested just dropping the t4042 bit, which is the only part that actually needs to handle multiple arguments. The other ones can just switch to using "$1". The helper script is simple enough that there is no need for them to share the same code.

-Peff