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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Pete Wyckoff wrote:

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

Nitpicks (some silly, some not):

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

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

[...]
> +    # %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.