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

> On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:
> > Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:
> 
> 
> > > +sed 's/^bin:/converted:/' "$@" >helper.out
> > 
> > Minor nit: this is inconsistent with the check done with grep above that
> > insists that the colon is followed by a SP.
> 
> Yes, you are right. I'll amend it.
> 
> > > diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
> > > index 91f8198..7668099 100755
> > > --- a/t/t4042-diff-textconv-caching.sh
> > > +++ b/t/t4042-diff-textconv-caching.sh
> > > @@ -5,18 +5,19 @@ test_description='test textconv caching'
> > >  
> > >  cat >helper <<'EOF'
> > >  #!/bin/sh
> > > -sed 's/^/converted: /' "$@" >helper.out
> > > +grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> > 
> > You are not feeding arguments you think you are to the above "echo":
> > 
> >         $ cat >/var/tmp/j.sh <<\EOF
> >         #!/bin/sh
> >         e () {
> >                 i=0
> >                 for s
> >                 do
> >                         i=$(( $i + 1 ))
> >                         echo "$i: $s"
> >                 done
> >         }
> >         f () {
> >                 e "E: $@ is not binary"
> >         }
> >         f 1 "2 3" 4
> >         EOF
> >         $ sh /var/tmp/j.sh
> >         1: E: 1
> >         2: 2 3
> >         3: 4 is not binary
> > 
> > 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)
> 
> ---- 8< ----
> 
> From: Kirill Smelkov <kirr@mns.spb.ru>
> Date: Tue, 28 Sep 2010 15:34:48 +0400
> Subject: [PATCH 4/4] t4042,t8006,t8007: Textconv converter should take only one argument
> 
> Textconv helper in this tests was incorrectly referencing $@, which goes
> agains textconv "spec". I quote Documentation/gitattributes.txt
> 
> """ The `textconv` config option is used to define a program for
>     performing such a conversion. The program should take a single
>     argument, the name of a file to convert, and produce the
>     resulting text on stdout. """
> 
> So correct textconv helpers to use $1 instead.
> 
> Noticed-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kirill Smelkov <kirr@mns.spb.ru>
> ---
>  t/t4042-diff-textconv-caching.sh |    4 ++--
>  t/t8006-blame-textconv.sh        |    4 ++--
>  t/t8007-cat-file-textconv.sh     |    4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
> index 68fee12..6aaa10b 100755
> --- a/t/t4042-diff-textconv-caching.sh
> +++ b/t/t4042-diff-textconv-caching.sh
> @@ -5,8 +5,8 @@ test_description='test textconv caching'
>  
>  cat >helper <<'EOF'
>  #!/bin/sh
> -grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
> -sed 's/^bin: /converted: /' "$@" >helper.out
> +grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
> +sed 's/^bin: /converted: /' "$1" >helper.out
>  cat helper.out
>  EOF
>  chmod +x helper