Re: [PATCHv2 1/2] attr: map builtin userdiff drivers to well-known extensions

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 19, 2011, 18:07 UTC
Message-ID: <20111219180733.GA12200@elie.hsd1.il.comcast.net>
In-Reply-To: <20111219154938.GA19829@sigill.intra.peff.net>

Hi,

Two quick thoughts:

Jeff King wrote:

> The C mappings are still here, but see the next patch.

This is adding a regression in order to remove it. I guess it's harmless, but I don't see the point.

[...]

Show 17 quoted lines

> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -90,4 +90,17 @@ test_expect_success 'diff --no-index with binary creation' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'binary files are not considered text by file extension' '
> +	echo Q | q_to_nul >binary.c &&
> +	git add binary.c &&
> +	cat >expect <<-\EOF &&
> +	diff --git a/binary.c b/binary.c
> +	new file mode 100644
> +	index 0000000..1f2a4f5
> +	Binary files /dev/null and b/binary.c differ
> +	EOF
> +	git diff --cached binary.c >actual &&
> +	test_cmp expect actual

Re the idea of this test: very good idea.

Re the mechanics: I would have been happier to see

	echo Q | q_to_nul >binary.c &&
	git add binary.c &&
	git diff --cached binary.c >diff &&
	grep Binary files diff

since that would avoid hard-coding some assumptions:

 - the blob name of binary.c
 - that [diff] mnemonicprefix defaults to false (I'd like to see the
   default change to true)
 - that [core] abbrev defaults to 7 (it probably won't change, but
   it's a distracting detail, and if we were starting over 8 might be
   a better default)

A bonus comment: :)

[...]

Show 12 quoted lines

> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -124,7 +124,9 @@ do
>  done
>  
>  test_expect_success 'default behaviour' '
> -	rm -f .gitattributes &&
> +	cat >.gitattributes <<-\EOF &&
> +	*.java diff=default
> +	EOF
>  	test_expect_funcname "public class Beer\$"
>  '

	echo "*.java diff=default" >.gitattributes

would do the same with two lines fewer. :)

Thanks for working on this.  I owe you a beer.

Jonathan