Merge pull request #3254 from ethomson/diff-binary-patch

Handle binary DIFFABLEness properly

src/diff_patch.c

@@ -121,6 +121,35 @@ GIT_INLINE(bool) should_skip_binary(git_patch *patch, git_diff_file *file)
 	return (file->flags & GIT_DIFF_FLAG_BINARY) != 0;
 }
 
+static bool diff_patch_diffable(git_patch *patch)
+{
+	size_t olen, nlen;
+
+	if (patch->delta->status == GIT_DELTA_UNMODIFIED)
+		return false;
+
+	/* if we've determined this to be binary (and we are not showing binary
+	 * data) then we have skipped loading the map data.  instead, query the
+	 * file data itself.
+	 */
+	if ((patch->delta->flags & GIT_DIFF_FLAG_BINARY) != 0 &&
+		(patch->diff_opts.flags & GIT_DIFF_SHOW_BINARY) == 0) {
+		olen = (size_t)patch->ofile.file->size;
+		nlen = (size_t)patch->nfile.file->size;
+	} else {
+		olen = patch->ofile.map.len;
+		nlen = patch->nfile.map.len;
+	}
+
+	/* if both sides are empty, files are identical */
+	if (!olen && !nlen)
+		return false;
+
+	/* otherwise, check the file sizes and the oid */
+	return (olen != nlen ||
+		!git_oid_equal(&patch->ofile.file->id, &patch->nfile.file->id));
+}
+
 static int diff_patch_load(git_patch *patch, git_diff_output *output)
 {
 	int error = 0;
@@ -186,18 +215,7 @@ cleanup:
 	diff_patch_update_binary(patch);
 
 	if (!error) {
-		bool skip_binary =
+		if (diff_patch_diffable(patch))
-			(patch->delta->flags & GIT_DIFF_FLAG_BINARY) != 0 &&
-			(patch->diff_opts.flags & GIT_DIFF_SHOW_BINARY) == 0;
-
-		/* patch is diffable only for non-binary, modified files where
-		* at least one side has data and the data actually changed
-		*/
-		if (!skip_binary &&
-			patch->delta->status != GIT_DELTA_UNMODIFIED &&
-			(patch->ofile.map.len || patch->nfile.map.len) &&
-			(patch->ofile.map.len != patch->nfile.map.len ||
-			 !git_oid_equal(&patch->ofile.file->id, &patch->nfile.file->id)))
 			patch->flags |= GIT_DIFF_PATCH_DIFFABLE;
 
 		patch->flags |= GIT_DIFF_PATCH_LOADED;

src/diff_patch.h

@@ -24,7 +24,9 @@ enum {
 	GIT_DIFF_PATCH_ALLOCATED = (1 << 0),
 	GIT_DIFF_PATCH_INITIALIZED = (1 << 1),
 	GIT_DIFF_PATCH_LOADED = (1 << 2),
+	/* the two sides are different */
 	GIT_DIFF_PATCH_DIFFABLE = (1 << 3),
+	/* the difference between the two sides has been computed */
 	GIT_DIFF_PATCH_DIFFED = (1 << 4),
 	GIT_DIFF_PATCH_FLATTENED = (1 << 5),
 };

tests/diff/binary.c

@@ -1,5 +1,7 @@
 #include "clar_libgit2.h"
 
+#include "git2/sys/diff.h"
+
 #include "buffer.h"
 #include "filebuf.h"
 
@@ -49,6 +51,11 @@ void test_patch(
 
 	cl_assert_equal_s(expected, actual.ptr);
 
+	git_buf_clear(&actual);
+	cl_git_pass(git_diff_print(diff, GIT_DIFF_FORMAT_PATCH, git_diff_print_callback__to_buf, &actual));
+
+	cl_assert_equal_s(expected, actual.ptr);
+
 	git_buf_free(&actual);
 	git_patch_free(patch);
 	git_diff_free(diff);
@@ -262,6 +269,36 @@ void test_diff_binary__delta_append(void)
 	git_index_free(index);
 }
 
+void test_diff_binary__empty_for_no_diff(void)
+{
+	git_diff_options opts = GIT_DIFF_OPTIONS_INIT;
+	git_oid id;
+	git_commit *commit;
+	git_tree *tree;
+	git_diff *diff;
+	git_buf actual = GIT_BUF_INIT;
+	const char *expected = "";
+
+	opts.flags = GIT_DIFF_SHOW_BINARY | GIT_DIFF_FORCE_BINARY;
+	opts.id_abbrev = GIT_OID_HEXSZ;
+
+	repo = cl_git_sandbox_init("renames");
+
+	cl_git_pass(git_oid_fromstr(&id, "19dd32dfb1520a64e5bbaae8dce6ef423dfa2f13"));
+	cl_git_pass(git_commit_lookup(&commit, repo, &id));
+	cl_git_pass(git_commit_tree(&tree, commit));
+
+	cl_git_pass(git_diff_tree_to_tree(&diff, repo, tree, tree, &opts));
+	cl_git_pass(git_diff_print(diff, GIT_DIFF_FORMAT_PATCH, git_diff_print_callback__to_buf, &actual));
+
+	cl_assert_equal_s("", actual.ptr);
+
+	git_buf_free(&actual);
+	git_diff_free(diff);
+	git_commit_free(commit);
+	git_tree_free(tree);
+}
+
 void test_diff_binary__index_to_workdir(void)
 {
 	git_index *index;