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

On Mon, Dec 19, 2011 at 12:07:33PM -0600, Jonathan Nieder wrote:

quoted

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.

It's purely an attempt to help somebody reading "git log" later
understand what happened. Maybe a comment in the commit message is more
appropriate.

quoted

+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

Yeah, I think that's fine, and I'll squash it in to my local version.

It does miss one problem, though (that is also present in my original):
using "binary.c" is no longer a good name, since the next patch will
revert the "*.c" bits. :)

quoted

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

Yup. I was following the style of the test directly below, which sets
both java and perl drivers. But the "default" test that needed updating
only checks the java case.

Will squash.

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

You're welcome. :)

-Peff