Merge pull request #1246 from arrbee/fix-force-text-for-diff-blobs

Add FORCE_TEXT check into git_diff_blobs code path

src/diff_output.c

@@ -89,13 +89,14 @@ static void update_delta_is_binary(git_diff_delta *delta)
 	/* otherwise leave delta->binary value untouched */
 }
 
-static int diff_delta_is_binary_by_attr(
-	diff_context *ctxt, git_diff_patch *patch)
+/* returns if we forced binary setting (and no further checks needed) */
+static bool diff_delta_is_binary_forced(
+	diff_context *ctxt,
+	git_diff_delta *delta)
 {
-	int error = 0, mirror_new;
-	git_diff_delta *delta = patch->delta;
-
-	delta->binary = -1;
+	/* return true if binary-ness has already been settled */
+	if (delta->binary != -1)
+		return true;
 
 	/* make sure files are conceivably mmap-able */
 	if ((git_off_t)((size_t)delta->old_file.size) != delta->old_file.size ||
@@ -104,7 +105,7 @@ static int diff_delta_is_binary_by_attr(
 		delta->old_file.flags |= GIT_DIFF_FILE_BINARY;
 		delta->new_file.flags |= GIT_DIFF_FILE_BINARY;
 		delta->binary = 1;
-		return 0;
+		return true;
 	}
 
 	/* check if user is forcing us to text diff these files */
@@ -112,9 +113,23 @@ static int diff_delta_is_binary_by_attr(
 		delta->old_file.flags |= GIT_DIFF_FILE_NOT_BINARY;
 		delta->new_file.flags |= GIT_DIFF_FILE_NOT_BINARY;
 		delta->binary = 0;
-		return 0;
+		return true;
 	}
 
+	return false;
+}
+
+static int diff_delta_is_binary_by_attr(
+	diff_context *ctxt, git_diff_patch *patch)
+{
+	int error = 0, mirror_new;
+	git_diff_delta *delta = patch->delta;
+
+	delta->binary = -1;
+
+	if (diff_delta_is_binary_forced(ctxt, delta))
+		return 0;
+
 	/* check diff attribute +, -, or 0 */
 	if (update_file_is_binary_by_attr(ctxt->repo, &delta->old_file) < 0)
 		return -1;
@@ -137,15 +152,17 @@ static int diff_delta_is_binary_by_content(
 	git_diff_file *file,
 	const git_map *map)
 {
-	const git_buf search = { map->data, 0, min(map->len, 4000) };
-
-	GIT_UNUSED(ctxt);
+	if (diff_delta_is_binary_forced(ctxt, delta))
+		return 0;
 
 	if ((file->flags & KNOWN_BINARY_FLAGS) == 0) {
+		const git_buf search = { map->data, 0, min(map->len, 4000) };
+
 		/* TODO: provide encoding / binary detection callbacks that can
 		 * be UTF-8 aware, etc.  For now, instead of trying to be smart,
 		 * let's just use the simple NUL-byte detection that core git uses.
 		 */
+
 		/* previously was: if (git_buf_text_is_binary(&search)) */
 		if (git_buf_text_contains_nul(&search))
 			file->flags |= GIT_DIFF_FILE_BINARY;

tests-clar/diff/blob.c

@@ -352,6 +352,20 @@ void test_diff_blob__can_correctly_detect_a_textual_blob_as_non_binary(void)
  * git_diff_blob_to_buffer tests
  */
 
+static void assert_changed_single_one_line_file(
+	diff_expects *expected, git_delta_t mod)
+{
+	cl_assert_equal_i(1, expected->files);
+	cl_assert_equal_i(1, expected->file_status[mod]);
+	cl_assert_equal_i(1, expected->hunks);
+	cl_assert_equal_i(1, expected->lines);
+
+	if (mod == GIT_DELTA_ADDED)
+		cl_assert_equal_i(1, expected->line_adds);
+	else if (mod == GIT_DELTA_DELETED)
+		cl_assert_equal_i(1, expected->line_dels);
+}
+
 void test_diff_blob__can_compare_blob_to_buffer(void)
 {
 	git_blob *a;
@@ -391,11 +405,7 @@ void test_diff_blob__can_compare_blob_to_buffer(void)
 		NULL, a_content, strlen(a_content),
 		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
 
-	cl_assert_equal_i(1, expected.files);
-	cl_assert_equal_i(1, expected.file_status[GIT_DELTA_ADDED]);
-	cl_assert_equal_i(1, expected.hunks);
-	cl_assert_equal_i(1, expected.lines);
-	cl_assert_equal_i(1, expected.line_adds);
+	assert_changed_single_one_line_file(&expected, GIT_DELTA_ADDED);
 
 	/* diff from blob a to NULL buffer */
 	memset(&expected, 0, sizeof(expected));
@@ -403,11 +413,7 @@ void test_diff_blob__can_compare_blob_to_buffer(void)
 		a, NULL, 0,
 		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
 
-	cl_assert_equal_i(1, expected.files);
-	cl_assert_equal_i(1, expected.file_status[GIT_DELTA_DELETED]);
-	cl_assert_equal_i(1, expected.hunks);
-	cl_assert_equal_i(1, expected.lines);
-	cl_assert_equal_i(1, expected.line_dels);
+	assert_changed_single_one_line_file(&expected, GIT_DELTA_DELETED);
 
 	/* diff with reverse */
 	opts.flags ^= GIT_DIFF_REVERSE;
@@ -417,11 +423,109 @@ void test_diff_blob__can_compare_blob_to_buffer(void)
 		a, NULL, 0,
 		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
 
-	cl_assert_equal_i(1, expected.files);
-	cl_assert_equal_i(1, expected.file_status[GIT_DELTA_ADDED]);
-	cl_assert_equal_i(1, expected.hunks);
-	cl_assert_equal_i(1, expected.lines);
-	cl_assert_equal_i(1, expected.line_adds);
+	assert_changed_single_one_line_file(&expected, GIT_DELTA_ADDED);
 
 	git_blob_free(a);
 }
+
+
+static void assert_one_modified_with_lines(diff_expects *expected, int lines)
+{
+	cl_assert_equal_i(1, expected->files);
+	cl_assert_equal_i(1, expected->file_status[GIT_DELTA_MODIFIED]);
+	cl_assert_equal_i(0, expected->files_binary);
+	cl_assert_equal_i(lines, expected->lines);
+}
+
+void test_diff_blob__binary_data_comparisons(void)
+{
+	git_blob *bin, *nonbin;
+	git_oid oid;
+	const char *nonbin_content = "Hello from the root\n";
+	size_t nonbin_len = 20;
+	const char *bin_content = "0123456789\n\x01\x02\x03\x04\x05\x06\x07\x08\x09\x00\n0123456789\n";
+	size_t bin_len = 33;
+
+	cl_git_pass(git_oid_fromstrn(&oid, "45141a79", 8));
+	cl_git_pass(git_blob_lookup_prefix(&nonbin, g_repo, &oid, 4));
+
+	cl_git_pass(git_oid_fromstrn(&oid, "b435cd56", 8));
+	cl_git_pass(git_blob_lookup_prefix(&bin, g_repo, &oid, 4));
+
+	/* non-binary to reference content */
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		nonbin, nonbin_content, nonbin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_identical_blobs_comparison(&expected);
+	cl_assert_equal_i(0, expected.files_binary);
+
+	/* binary to reference content */
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		bin, bin_content, bin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_identical_blobs_comparison(&expected);
+
+	cl_assert_equal_i(1, expected.files_binary);
+
+	/* non-binary to binary content */
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		nonbin, bin_content, bin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_binary_blobs_comparison(&expected);
+
+	/* binary to non-binary content */
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		bin, nonbin_content, nonbin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_binary_blobs_comparison(&expected);
+
+	/* non-binary to binary blob */
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blobs(
+		bin, nonbin, &opts,
+		diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_binary_blobs_comparison(&expected);
+
+	/*
+	 * repeat with FORCE_TEXT
+	 */
+
+	opts.flags |= GIT_DIFF_FORCE_TEXT;
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		bin, bin_content, bin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_identical_blobs_comparison(&expected);
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		nonbin, bin_content, bin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_one_modified_with_lines(&expected, 4);
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		bin, nonbin_content, nonbin_len,
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_one_modified_with_lines(&expected, 4);
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blobs(
+		bin, nonbin, &opts,
+		diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+	assert_one_modified_with_lines(&expected, 4);
+
+	/* cleanup */
+	git_blob_free(bin);
+	git_blob_free(nonbin);
+}