From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 27 Sep 2010 18:23:35 GMT
Subject: Re: [PATCH v3 1/3] tests: Prepare --textconv tests for correctly-failing conversion program
Message-ID: <7vsk0vyriw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <66d4603c7e21561557f612690d6196e7ae0b38f9.1285351816.git.kirr@landau.phys.spbu.ru>

Kirill Smelkov <kirr@landau.phys.spbu.ru> writes:

> 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.

> +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.