On Tue, Sep 28, 2010 at 04:07:22PM +0400, Kirill Smelkov wrote:
> On Mon, Sep 27, 2010 at 11:23:35AM -0700, Junio C Hamano wrote:
> > Kirill Smelkov <kirr@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Kirill Smelkov <kirr@xxxxxxxxxx>
> ---
> 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
Please ignore this - changing so breaks textconv cache tests in this
file.
I'll come up with updated patch.
Sorry for the noise...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html