[PATCH 1/2] diff: fix handling of binary rewrite diffs

From: Jeff King <peff@peff.net>
Date: Dec 9, 2008, 08:12 UTC
Message-ID: <20081209081227.GA19626@coredump.intra.peff.net>

The current emit_rewrite_diff code always writes a text patch without checking whether the content is binary. This means that if you end up with a rewrite diff for a binary file, you get lots of raw binary goo in your patch.

Instead, if we have binary files, then let's just skip emit_rewrite_diff altogether. We will already have shown the "dissimilarity index" line, so it is really about the diff contents. If binary diffs are turned off, the "Binary files a/file and b/file differ" message should be the same in either case. If we do have binary patches turned on, there isn't much point in making a less-efficient binary patch that does a total rewrite; no human is going to read it, and since binary patches don't apply with any fuzz anyway, the result of application should be the same.

Signed-off-by: Jeff King <peff@peff.net>
---
I couldn't think of a good reason to want a different format of binary
patch for a rewrite versus a regular diff, but others may be able to.
In that case, you have to mimic the binary codepath somewhat in
emit_rewrite_diff; I started on it, but realized I was just copying the
code.

However, we should at least do something before 1.6.1; as it is, it spews binary garbage.

I discovered this because one of my textconv'd files had a rewrite (which apparently happens by changing a few lines in a word document :)) and gave me a bogus diff. However, the problem is not unique to textconv; patch 1/2 handles binary files and 2/2 handles the textconv case.

I didn't put the tests into the existing t4022-diff-rewrite because I wanted to keep the textconv tests together with these tests, and I think it makes sense to test features in isolation before testing them together. IOW:

  t4022 - rewrite works
  t4030 - textconv works
  t4031 - rewrite AND textconv work together

 diff.c                         |    4 ++-
 t/t4031-diff-rewrite-binary.sh |   42 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletions(-)
 create mode 100755 t/t4031-diff-rewrite-binary.sh

diff --git a/diff.c b/diff.c
index f644947..ea958a2 100644
--- a/diff.c
+++ b/diff.c
@@ -1376,7 +1376,9 @@ static void builtin_diff(const char *name_a,
 		 */
 		if ((one->mode ^ two->mode) & S_IFMT)
 			goto free_ab_and_return;
-		if (complete_rewrite) {
+		if (complete_rewrite &&
+		    !diff_filespec_is_binary(one) &&
+		    !diff_filespec_is_binary(two)) {
 			emit_rewrite_diff(name_a, name_b, one, two, o);
 			o->found_changes = 1;
 			goto free_ab_and_return;
diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
new file mode 100755
index 0000000..4b522f7
--- /dev/null
+++ b/t/t4031-diff-rewrite-binary.sh
@@ -0,0 +1,42 @@
+#!/bin/sh
+
+test_description='rewrite diff on binary file'
+
+. ./test-lib.sh
+
+# We must be large enough to meet the MINIMUM_BREAK_SIZE
+# requirement.
+make_file() {
+	for i in 1 2 3 4 5 6 7 8 9 10; do
+		for j in 1 2 3 4 5 6 7 9 10; do
+			for k in 1 2 3 4 5; do
+				printf "$1\n"
+			done
+		done
+	done >file
+}
+
+test_expect_success 'create binary file with changes' '
+	make_file "\\0" &&
+	git add file &&
+	make_file "\\01"
+'
+
+test_expect_success 'vanilla diff is binary' '
+	git diff >diff &&
+	grep "Binary files a/file and b/file differ" diff
+'
+
+test_expect_success 'rewrite diff is binary' '
+	git diff -B >diff &&
+	grep "dissimilarity index" diff &&
+	grep "Binary files a/file and b/file differ" diff
+'
+
+test_expect_success 'rewrite diff can show binary patch' '
+	git diff -B --binary >diff &&
+	grep "dissimilarity index" diff &&
+	grep "GIT binary patch" diff
+'
+
+test_done

-- 
1.6.1.rc2.1.g8f945.dirty