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
diff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh
index a8c311f..dbf623b 100755
--- a/t/t8006-blame-textconv.sh
+++ b/t/t8006-blame-textconv.sh
@@ -9,8 +9,8 @@ find_blame() {
cat >helper <<'EOF'
#!/bin/sh
-grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$@"
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: /converted: /' "$1"
EOF
chmod +x helper
diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh
index 5e2e0d2..78a0085 100755
--- a/t/t8007-cat-file-textconv.sh
+++ b/t/t8007-cat-file-textconv.sh
@@ -5,8 +5,8 @@ test_description='git cat-file textconv support'
cat >helper <<'EOF'
#!/bin/sh
-grep -q '^bin: ' "$@" || { echo "E: $@ is not \"binary\" file" 1>&2; exit 1; }
-sed 's/^bin: /converted: /' "$@"
+grep -q '^bin: ' "$1" || { echo "E: $1 is not \"binary\" file" 1>&2; exit 1; }
+sed 's/^bin: /converted: /' "$1"
EOF
chmod +x helper
--
1.7.3.19.g3fe0a
--
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