Re: [PATCH v3] convert filter: supply path to external driver

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 21, 2010, 18:19 UTC
Message-ID: <20101221181924.GB25812@burratino>
In-Reply-To: <20101221134403.GA10401@honk.padd.com>

Hi,

Pete Wyckoff wrote:

> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh

Nitpicks (some silly, some not):

Show 10 quoted lines

> @@ -93,4 +93,51 @@ test_expect_success expanded_in_repo '
>  	cmp expanded-keywords expected-output
>  '
>  
> +cat <<EOF >argc.sh
> +#!$SHELL_PATH
> +echo argc: \$# "\$@"
> +echo argc running >&2
> +EOF
> +chmod +x argc.sh

You can embed this in a test_expect_success stanza (like the next one or the earlier "setup") like so:

	cat <<-EOF >argc.sh &&
	#!$SHELL_PATH
	...
	EOF
	chmod +x argc.sh &&

This way if the "chmod" fails on some platform the test would catch that.

Show 5 quoted lines

> +
> +#
> +# The use of %f in a filter definition is expanded to the path to
> +# the filename being smudged or cleaned.  It must be shell escaped.
> +#

I'd even prefer to see this comment inside the test_expect_success assertion so it can be printed when running the test with "-v". But I suppose consistency with the other test in the script suggests otherwise.

[...]

> +    echo some test text > test
> +    cat test > $norm &&
> +    cat test > "$spec" &&

Missing && after "> test". Probably best to remove the space after > (just for consistency[1]). Also, please use tabs to indent.

[...]

> +    # make sure argc.sh counted the right number of args
> +    echo "argc: 1 $norm" > res &&
> +    cmp res $norm &&

test_cmp?  (for nicer output)  See t/README.

[...]

Show 10 quoted lines

> +    # %f with other args
> +    git config filter.argc.smudge "./argc.sh %f --myword" &&
> +    rm $norm "$spec" &&
> +    git checkout -- $norm "$spec" &&
> +
> +    # make sure argc.sh counted the right number of args
> +    echo "argc: 2 $norm --myword" > res &&
> +    cmp res $norm &&
> +    echo "argc: 2 $spec --myword" > res &&
> +    cmp res "$spec" &&

Probably would be clearer if this were a separate test assertion.

> +    :
> +'
> +
>  test_done

Thanks for the tests. I haven't looked at the substance, alas, but hope that helps nonetheless.

Jonathan

[1] Trumped up justification for the "no space after >" style: if I always include a space after, I would be tempted to use

	noisy_command > /dev/null 2> &1

But that does not work because >& is recognized as a single token.