Re: [PATCHv2 1/2] attr: map builtin userdiff drivers to well-known extensions
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:40
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. [...]
quoted hunk ↗ jump to hunk
--- 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: :) [...]
quoted hunk ↗ jump to hunk
--- 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