Merge pull request #668 from nulltoken/topic/binary-blobs

Enhancing the blob diffing experience

include/git2/diff.h

@@ -337,12 +337,21 @@ GIT_EXTERN(int) git_diff_print_patch(
 
 /**
  * Directly run a text diff on two blobs.
+ *
+ * Compared to a file, a blob lacks some contextual information. As such, the
+ * `git_diff_file` parameters of the callbacks will be filled accordingly to the following:
+ * `mode` will be set to 0, `path` will be set to NULL. When dealing with a NULL blob, `oid`
+ * will be set to 0.
+ *
+ * When at least one of the blobs being dealt with is binary, the `git_diff_delta` binary
+ * attribute will be set to 1 and no call to the hunk_cb nor line_cb will be made.
  */
 GIT_EXTERN(int) git_diff_blobs(
 	git_blob *old_blob,
 	git_blob *new_blob,
 	git_diff_options *options,
 	void *cb_data,
+	git_diff_file_fn file_cb,
 	git_diff_hunk_fn hunk_cb,
 	git_diff_data_fn line_cb);
 

src/diff_output.c

@@ -169,7 +169,6 @@ static int file_is_binary_by_attr(
 }
 
 static int file_is_binary_by_content(
-	git_diff_list *diff,
 	git_diff_delta *delta,
 	git_map *old_data,
 	git_map *new_data)
@@ -177,8 +176,6 @@ static int file_is_binary_by_content(
 	git_buf search;
 	git_text_stats stats;
 
-	GIT_UNUSED(diff);
-
 	if ((delta->old_file.flags & BINARY_DIFF_FLAGS) == 0) {
 		search.ptr  = old_data->data;
 		search.size = min(old_data->len, 4000);
@@ -301,6 +298,16 @@ static void release_content(git_diff_file *file, git_map *map, git_blob *blob)
 	}
 }
 
+static void fill_map_from_mmfile(git_map *dst, mmfile_t *src) {
+	assert(dst && src);
+
+	dst->data = src->ptr;
+	dst->len = src->size;
+#ifdef GIT_WIN32
+	dst->fmh = NULL;
+#endif
+}
+
 int git_diff_foreach(
 	git_diff_list *diff,
 	void *data,
@@ -408,7 +415,7 @@ int git_diff_foreach(
 		 */
 		if (delta->binary == -1) {
 			error = file_is_binary_by_content(
-				diff, delta, &old_data, &new_data);
+				delta, &old_data, &new_data);
 			if (error < 0)
 				goto cleanup;
 		}
@@ -689,55 +696,83 @@ int git_diff_print_patch(
 	return error;
 }
 
-
 int git_diff_blobs(
 	git_blob *old_blob,
 	git_blob *new_blob,
 	git_diff_options *options,
 	void *cb_data,
+	git_diff_file_fn file_cb,
 	git_diff_hunk_fn hunk_cb,
 	git_diff_data_fn line_cb)
 {
 	diff_output_info info;
 	git_diff_delta delta;
 	mmfile_t old_data, new_data;
+	git_map old_map, new_map;
 	xpparam_t xdiff_params;
 	xdemitconf_t xdiff_config;
 	xdemitcb_t xdiff_callback;
+	git_blob *new, *old;
+
+	memset(&delta, 0, sizeof(delta));
+
+	new = new_blob;
+	old = old_blob;
 
 	if (options && (options->flags & GIT_DIFF_REVERSE)) {
-		git_blob *swap = old_blob;
-		old_blob = new_blob;
-		new_blob = swap;
+		git_blob *swap = old;
+		old = new;
+		new = swap;
 	}
 
-	if (old_blob) {
-		old_data.ptr  = (char *)git_blob_rawcontent(old_blob);
-		old_data.size = git_blob_rawsize(old_blob);
+	if (old) {
+		old_data.ptr  = (char *)git_blob_rawcontent(old);
+		old_data.size = git_blob_rawsize(old);
+		git_oid_cpy(&delta.old_file.oid, git_object_id((const git_object *)old));
 	} else {
 		old_data.ptr  = "";
 		old_data.size = 0;
 	}
 
-	if (new_blob) {
-		new_data.ptr  = (char *)git_blob_rawcontent(new_blob);
-		new_data.size = git_blob_rawsize(new_blob);
+	if (new) {
+		new_data.ptr  = (char *)git_blob_rawcontent(new);
+		new_data.size = git_blob_rawsize(new);
+		git_oid_cpy(&delta.new_file.oid, git_object_id((const git_object *)new));
 	} else {
 		new_data.ptr  = "";
 		new_data.size = 0;
 	}
 
 	/* populate a "fake" delta record */
-	delta.status = old_data.ptr ?
-		(new_data.ptr ? GIT_DELTA_MODIFIED : GIT_DELTA_DELETED) :
-		(new_data.ptr ? GIT_DELTA_ADDED : GIT_DELTA_UNTRACKED);
-	delta.old_file.mode = 0100644; /* can't know the truth from a blob alone */
-	delta.new_file.mode = 0100644;
-	git_oid_cpy(&delta.old_file.oid, git_object_id((const git_object *)old_blob));
-	git_oid_cpy(&delta.new_file.oid, git_object_id((const git_object *)new_blob));
-	delta.old_file.path = NULL;
-	delta.new_file.path = NULL;
-	delta.similarity = 0;
+	delta.status = new ?
+		(old ? GIT_DELTA_MODIFIED : GIT_DELTA_ADDED) :
+		(old ? GIT_DELTA_DELETED : GIT_DELTA_UNTRACKED);
+
+	if (git_oid_cmp(&delta.new_file.oid, &delta.old_file.oid) == 0)
+		delta.status = GIT_DELTA_UNMODIFIED;
+
+	delta.old_file.size = old_data.size;
+	delta.new_file.size = new_data.size;
+
+	fill_map_from_mmfile(&old_map, &old_data);
+	fill_map_from_mmfile(&new_map, &new_data);
+
+	if (file_is_binary_by_content(&delta, &old_map, &new_map) < 0)
+		return -1;
+
+	if (file_cb != NULL) {
+		int error = file_cb(cb_data, &delta, 1);
+		if (error < 0)
+			return error;
+	}
+
+	/* don't do hunk and line diffs if the two blobs are identical */
+	if (delta.status == GIT_DELTA_UNMODIFIED)
+		return 0;
+
+	/* don't do hunk and line diffs if file is binary */
+	if (delta.binary == 1)
+		return 0;
 
 	info.diff    = NULL;
 	info.delta   = &delta;

tests-clar/diff/blob.c

@@ -2,23 +2,43 @@
 #include "diff_helpers.h"
 
 static git_repository *g_repo = NULL;
+static diff_expects exp;
+static git_diff_options opts;
+static git_blob *d, *alien;
 
 void test_diff_blob__initialize(void)
 {
+	git_oid oid;
+
 	g_repo = cl_git_sandbox_init("attr");
+
+	memset(&opts, 0, sizeof(opts));
+	opts.context_lines = 1;
+	opts.interhunk_lines = 1;
+
+	memset(&exp, 0, sizeof(exp));
+
+	/* tests/resources/attr/root_test4.txt */
+	cl_git_pass(git_oid_fromstrn(&oid, "fe773770c5a6", 12));
+	cl_git_pass(git_blob_lookup_prefix(&d, g_repo, &oid, 6));
+
+	/* alien.png */
+	cl_git_pass(git_oid_fromstrn(&oid, "edf3dcee", 8));
+	cl_git_pass(git_blob_lookup_prefix(&alien, g_repo, &oid, 4));
 }
 
 void test_diff_blob__cleanup(void)
 {
+	git_blob_free(d);
+	git_blob_free(alien);
+
 	cl_git_sandbox_cleanup();
 }
 
-void test_diff_blob__0(void)
+void test_diff_blob__can_compare_text_blobs(void)
 {
-	git_blob *a, *b, *c, *d;
-	git_oid a_oid, b_oid, c_oid, d_oid;
-	git_diff_options opts = {0};
-	diff_expects exp;
+	git_blob *a, *b, *c;
+	git_oid a_oid, b_oid, c_oid;
 
 	/* tests/resources/attr/root_test1 */
 	cl_git_pass(git_oid_fromstrn(&a_oid, "45141a79", 8));
@@ -32,18 +52,14 @@ void test_diff_blob__0(void)
 	cl_git_pass(git_oid_fromstrn(&c_oid, "c96bbb2c2557a832", 16));
 	cl_git_pass(git_blob_lookup_prefix(&c, g_repo, &c_oid, 8));
 
-	/* tests/resources/attr/root_test4.txt */
-	cl_git_pass(git_oid_fromstrn(&d_oid, "fe773770c5a6", 12));
-	cl_git_pass(git_blob_lookup_prefix(&d, g_repo, &d_oid, 6));
-
 	/* Doing the equivalent of a `git diff -U1` on these files */
 
-	opts.context_lines = 1;
-	opts.interhunk_lines = 1;
-
-	memset(&exp, 0, sizeof(exp));
 	cl_git_pass(git_diff_blobs(
-		a, b, &opts, &exp, diff_hunk_fn, diff_line_fn));
+		a, b, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_mods == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
 
 	cl_assert(exp.hunks == 1);
 	cl_assert(exp.lines == 6);
@@ -53,7 +69,11 @@ void test_diff_blob__0(void)
 
 	memset(&exp, 0, sizeof(exp));
 	cl_git_pass(git_diff_blobs(
-		b, c, &opts, &exp, diff_hunk_fn, diff_line_fn));
+		b, c, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_mods == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
 
 	cl_assert(exp.hunks == 1);
 	cl_assert(exp.lines == 15);
@@ -63,7 +83,11 @@ void test_diff_blob__0(void)
 
 	memset(&exp, 0, sizeof(exp));
 	cl_git_pass(git_diff_blobs(
-		a, c, &opts, &exp, diff_hunk_fn, diff_line_fn));
+		a, c, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_mods == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
 
 	cl_assert(exp.hunks == 1);
 	cl_assert(exp.lines == 13);
@@ -75,7 +99,11 @@ void test_diff_blob__0(void)
 
 	memset(&exp, 0, sizeof(exp));
 	cl_git_pass(git_diff_blobs(
-		c, d, &opts, &exp, diff_hunk_fn, diff_line_fn));
+		c, d, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_mods == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
 
 	cl_assert(exp.hunks == 2);
 	cl_assert(exp.lines == 14);
@@ -86,6 +114,141 @@ void test_diff_blob__0(void)
 	git_blob_free(a);
 	git_blob_free(b);
 	git_blob_free(c);
-	git_blob_free(d);
 }
 
+void test_diff_blob__can_compare_against_null_blobs(void)
+{
+	git_blob *e = NULL;
+
+	cl_git_pass(git_diff_blobs(
+		d, e, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_dels == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
+
+	cl_assert(exp.hunks == 1);
+	cl_assert(exp.hunk_old_lines == 14);
+	cl_assert(exp.lines == 14);
+	cl_assert(exp.line_dels == 14);
+
+	opts.flags |= GIT_DIFF_REVERSE;
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_blobs(
+		d, e, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_adds == 1);
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
+
+	cl_assert(exp.hunks == 1);
+	cl_assert(exp.hunk_new_lines == 14);
+	cl_assert(exp.lines == 14);
+	cl_assert(exp.line_adds == 14);
+
+	opts.flags ^= GIT_DIFF_REVERSE;
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_blobs(
+		alien, NULL, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.at_least_one_of_them_is_binary == true);
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_dels == 1);
+	cl_assert(exp.hunks == 0);
+	cl_assert(exp.lines == 0);
+
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_blobs(
+		NULL, alien, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.at_least_one_of_them_is_binary == true);
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_adds == 1);
+	cl_assert(exp.hunks == 0);
+	cl_assert(exp.lines == 0);
+}
+
+void assert_identical_blobs_comparison(diff_expects exp)
+{
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_unmodified == 1);
+	cl_assert(exp.hunks == 0);
+	cl_assert(exp.lines == 0);
+}
+
+void test_diff_blob__can_compare_identical_blobs(void)
+{
+	cl_git_pass(git_diff_blobs(
+		d, d, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
+	assert_identical_blobs_comparison(exp);
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		NULL, NULL, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.at_least_one_of_them_is_binary == false);
+	assert_identical_blobs_comparison(exp);
+
+	memset(&exp, 0, sizeof(exp));
+	cl_git_pass(git_diff_blobs(
+		alien, alien, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	cl_assert(exp.at_least_one_of_them_is_binary == true);
+	assert_identical_blobs_comparison(exp);
+}
+
+void assert_binary_blobs_comparison(diff_expects exp)
+{
+	cl_assert(exp.at_least_one_of_them_is_binary == true);
+
+	cl_assert(exp.files == 1);
+	cl_assert(exp.file_mods == 1);
+	cl_assert(exp.hunks == 0);
+	cl_assert(exp.lines == 0);
+}
+
+void test_diff_blob__can_compare_two_binary_blobs(void)
+{
+	git_blob *heart;
+	git_oid h_oid;
+
+	/* heart.png */
+	cl_git_pass(git_oid_fromstrn(&h_oid, "de863bff", 8));
+	cl_git_pass(git_blob_lookup_prefix(&heart, g_repo, &h_oid, 4));
+
+	cl_git_pass(git_diff_blobs(
+		alien, heart, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	assert_binary_blobs_comparison(exp);
+
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_blobs(
+		heart, alien, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	assert_binary_blobs_comparison(exp);
+
+	git_blob_free(heart);
+}
+
+void test_diff_blob__can_compare_a_binary_blob_and_a_text_blob(void)
+{
+	cl_git_pass(git_diff_blobs(
+		alien, d, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	assert_binary_blobs_comparison(exp);
+
+	memset(&exp, 0, sizeof(exp));
+
+	cl_git_pass(git_diff_blobs(
+		d, alien, &opts, &exp, diff_file_fn, diff_hunk_fn, diff_line_fn));
+
+	assert_binary_blobs_comparison(exp);
+}

tests-clar/diff/diff_helpers.c

@@ -30,6 +30,8 @@ int diff_file_fn(
 
 	GIT_UNUSED(progress);
 
+	e->	at_least_one_of_them_is_binary = delta->binary;
+
 	e->files++;
 	switch (delta->status) {
 	case GIT_DELTA_ADDED: e->file_adds++; break;
@@ -37,6 +39,7 @@ int diff_file_fn(
 	case GIT_DELTA_MODIFIED: e->file_mods++; break;
 	case GIT_DELTA_IGNORED: e->file_ignored++; break;
 	case GIT_DELTA_UNTRACKED: e->file_untracked++; break;
+	case GIT_DELTA_UNMODIFIED: e->file_unmodified++; break;
 	default: break;
 	}
 	return 0;

tests-clar/diff/diff_helpers.h

@@ -11,6 +11,7 @@ typedef struct {
 	int file_mods;
 	int file_ignored;
 	int file_untracked;
+	int file_unmodified;
 
 	int hunks;
 	int hunk_new_lines;
@@ -20,6 +21,8 @@ typedef struct {
 	int line_ctxt;
 	int line_adds;
 	int line_dels;
+
+	bool at_least_one_of_them_is_binary;
 } diff_expects;
 
 extern int diff_file_fn(

tests-clar/diff/tree.c

@@ -113,16 +113,16 @@ void test_diff_tree__options(void)
 	 */
 	diff_expects test_expects[] = {
 		/* a vs b tests */
-		{ 5, 3, 0, 2, 0, 0, 4, 0, 0, 51, 2, 46, 3 },
-		{ 5, 3, 0, 2, 0, 0, 4, 0, 0, 53, 4, 46, 3 },
-		{ 5, 0, 3, 2, 0, 0, 4, 0, 0, 52, 3, 3, 46 },
-		{ 5, 3, 0, 2, 0, 0, 5, 0, 0, 54, 3, 48, 3 },
+		{ 5, 3, 0, 2, 0, 0, 0, 4, 0, 0, 51, 2, 46, 3 },
+		{ 5, 3, 0, 2, 0, 0, 0, 4, 0, 0, 53, 4, 46, 3 },
+		{ 5, 0, 3, 2, 0, 0, 0, 4, 0, 0, 52, 3, 3, 46 },
+		{ 5, 3, 0, 2, 0, 0, 0, 5, 0, 0, 54, 3, 48, 3 },
 		/* c vs d tests */
-		{ 1, 0, 0, 1, 0, 0, 1, 0, 0, 22, 9, 10, 3 },
-		{ 1, 0, 0, 1, 0, 0, 1, 0, 0, 19, 12, 7, 0 },
-		{ 1, 0, 0, 1, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
-		{ 1, 0, 0, 1, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
-		{ 1, 0, 0, 1, 0, 0, 1, 0, 0, 18, 11, 0, 7 },
+		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 22, 9, 10, 3 },
+		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 19, 12, 7, 0 },
+		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
+		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 20, 11, 8, 1 },
+		{ 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 18, 11, 0, 7 },
 		{ 0 },
 	};
 	diff_expects *expected;