Share git_diff_blobs/git_diff_blob_to_buffer code

This moves the implementation of these two APIs into common code
that will be shared between the two.  Also, this adds tests for
the `git_diff_blob_to_buffer` API.  Lastly, this adds some extra
`const` to a few places that can use it.

include/git2/blob.h

@@ -91,7 +91,7 @@ GIT_INLINE(const git_oid *) git_blob_id(const git_blob *blob)
  * @param blob pointer to the blob
  * @return the pointer; NULL if the blob has no contents
  */
-GIT_EXTERN(const void *) git_blob_rawcontent(git_blob *blob);
+GIT_EXTERN(const void *) git_blob_rawcontent(const git_blob *blob);
 
 /**
  * Get the size in bytes of the contents of a blob
@@ -99,7 +99,7 @@ GIT_EXTERN(const void *) git_blob_rawcontent(git_blob *blob);
  * @param blob pointer to the blob
  * @return size on bytes
  */
-GIT_EXTERN(git_off_t) git_blob_rawsize(git_blob *blob);
+GIT_EXTERN(git_off_t) git_blob_rawsize(const git_blob *blob);
 
 /**
  * Read a file from the working folder of a repository

include/git2/diff.h

@@ -802,22 +802,25 @@ GIT_EXTERN(int) git_diff_patch_to_str(
  */
 
 /**
- * Directly run a text diff on two blobs.
+ * Directly run a 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.
+ * the `git_diff_file` given to the callback will have some fake data; i.e.
+ * `mode` will be 0 and `path` will be NULL.
  *
- * 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.
+ * NULL is allowed for either `old_blob` or `new_blob` and will be treated
+ * as an empty blob, with the `oid` set to NULL in the `git_diff_file` data.
+ *
+ * We do run a binary content check on the two blobs and if either of the
+ * blobs looks like binary data, the `git_diff_delta` binary attribute will
+ * be set to 1 and no call to the hunk_cb nor line_cb will be made (unless
+ * you pass `GIT_DIFF_FORCE_TEXT` of course).
  *
  * @return 0 on success, GIT_EUSER on non-zero callback, or error code
  */
 GIT_EXTERN(int) git_diff_blobs(
-	git_blob *old_blob,
-	git_blob *new_blob,
+	const git_blob *old_blob,
+	const git_blob *new_blob,
 	const git_diff_options *options,
 	git_diff_file_cb file_cb,
 	git_diff_hunk_cb hunk_cb,
@@ -825,22 +828,17 @@ GIT_EXTERN(int) git_diff_blobs(
 	void *payload);
 
 /**
- * Directly run a text diff between a blob and a buffer.
- *
- * Compared to a file, a blob and a buffer lack 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.
+ * Directly run a diff between a blob and a buffer.
  *
- * When at least the blob or the buffer are 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.
+ * As with `git_diff_blobs`, comparing a blob and buffer lacks some context,
+ * so the `git_diff_file` parameters to the callbacks will be faked a la the
+ * rules for `git_diff_blobs()`.
  *
  * @return 0 on success, GIT_EUSER on non-zero callback, or error code
  */
 GIT_EXTERN(int) git_diff_blob_to_buffer(
-	git_blob *old_blob,
-	char *buffer,
+	const git_blob *old_blob,
+	const char *buffer,
 	size_t buffer_len,
 	const git_diff_options *options,
 	git_diff_file_cb file_cb,

src/blob.c

@@ -13,13 +13,13 @@
 #include "blob.h"
 #include "filter.h"
 
-const void *git_blob_rawcontent(git_blob *blob)
+const void *git_blob_rawcontent(const git_blob *blob)
 {
 	assert(blob);
 	return blob->odb_object->raw.data;
 }
 
-git_off_t git_blob_rawsize(git_blob *blob)
+git_off_t git_blob_rawsize(const git_blob *blob)
 {
 	assert(blob);
 	return (git_off_t)blob->odb_object->raw.len;

src/diff_output.c

@@ -132,16 +132,16 @@ static int diff_delta_is_binary_by_attr(
 }
 
 static int diff_delta_is_binary_by_content(
-	diff_context *ctxt, git_diff_delta *delta, git_diff_file *file, git_map *map)
+	diff_context *ctxt,
+	git_diff_delta *delta,
+	git_diff_file *file,
+	const git_map *map)
 {
-	git_buf search;
+	const git_buf search = { map->data, 0, min(map->len, 4000) };
 
 	GIT_UNUSED(ctxt);
 
 	if ((file->flags & KNOWN_BINARY_FLAGS) == 0) {
-		search.ptr  = map->data;
-		search.size = min(map->len, 4000);
-
 		if (git_buf_text_is_binary(&search))
 			file->flags |= GIT_DIFF_FILE_BINARY;
 		else
@@ -1232,7 +1232,7 @@ int git_diff_print_patch(
 }
 
 static void set_data_from_blob(
-	git_blob *blob, git_map *map, git_diff_file *file)
+	const git_blob *blob, git_map *map, git_diff_file *file)
 {
 	if (blob) {
 		file->size = git_blob_rawsize(blob);
@@ -1250,97 +1250,95 @@ static void set_data_from_blob(
 	}
 }
 
-int git_diff_blobs(
-	git_blob *old_blob,
-	git_blob *new_blob,
-	const git_diff_options *options,
-	git_diff_file_cb file_cb,
-	git_diff_hunk_cb hunk_cb,
-	git_diff_data_cb data_cb,
-	void *payload)
+static void set_data_from_buffer(
+	const char *buffer, size_t buffer_len, git_map *map, git_diff_file *file)
 {
-	int error;
-	git_repository *repo;
+	file->size = (git_off_t)buffer_len;
+	file->mode = 0644;
+
+	if (!buffer)
+		file->flags |= GIT_DIFF_FILE_NO_DATA;
+	else
+		git_odb_hash(&file->oid, buffer, buffer_len, GIT_OBJ_BLOB);
+
+	map->len   = buffer_len;
+	map->data  = (char *)buffer;
+}
+
+typedef struct {
 	diff_context   ctxt;
 	git_diff_delta delta;
 	git_diff_patch patch;
+} diff_single_data;
 
-	GITERR_CHECK_VERSION(options, GIT_DIFF_OPTIONS_VERSION, "git_diff_options");
-
-	if (options && (options->flags & GIT_DIFF_REVERSE)) {
-		git_blob *swap = old_blob;
-		old_blob = new_blob;
-		new_blob = swap;
-	}
+static int diff_single_init(
+	diff_single_data *data,
+	git_repository *repo,
+	const git_diff_options *opts,
+	git_diff_file_cb file_cb,
+	git_diff_hunk_cb hunk_cb,
+	git_diff_data_cb data_cb,
+	void *payload)
+{
+	GITERR_CHECK_VERSION(opts, GIT_DIFF_OPTIONS_VERSION, "git_diff_options");
 
-	if (new_blob)
-		repo = git_object_owner((git_object *)new_blob);
-	else if (old_blob)
-		repo = git_object_owner((git_object *)old_blob);
-	else
-		repo = NULL;
+	memset(data, 0, sizeof(*data));
 
 	diff_context_init(
-		&ctxt, NULL, repo, options,
-		file_cb, hunk_cb, data_cb, payload);
+		&data->ctxt, NULL, repo, opts, file_cb, hunk_cb, data_cb, payload);
 
-	diff_patch_init(&ctxt, &patch);
+	diff_patch_init(&data->ctxt, &data->patch);
 
-	/* create a fake delta record and simulate diff_patch_load */
+	return 0;
+}
 
-	memset(&delta, 0, sizeof(delta));
-	delta.binary = -1;
+static int diff_single_apply(diff_single_data *data)
+{
+	int error;
+	git_diff_delta *delta = &data->delta;
+	bool has_old = ((delta->old_file.flags & GIT_DIFF_FILE_NO_DATA) == 0);
+	bool has_new = ((delta->new_file.flags & GIT_DIFF_FILE_NO_DATA) == 0);
 
-	set_data_from_blob(old_blob, &patch.old_data, &delta.old_file);
-	set_data_from_blob(new_blob, &patch.new_data, &delta.new_file);
+	/* finish setting up fake git_diff_delta record and loaded data */
 
-	delta.status = new_blob ?
-		(old_blob ? GIT_DELTA_MODIFIED : GIT_DELTA_ADDED) :
-		(old_blob ? GIT_DELTA_DELETED : GIT_DELTA_UNTRACKED);
+	data->patch.delta = delta;
+	delta->binary = -1;
 
-	if (git_oid_cmp(&delta.new_file.oid, &delta.old_file.oid) == 0)
-		delta.status = GIT_DELTA_UNMODIFIED;
+	delta->status = has_new ?
+		(has_old ? GIT_DELTA_MODIFIED : GIT_DELTA_ADDED) :
+		(has_old ? GIT_DELTA_DELETED : GIT_DELTA_UNTRACKED);
 
-	patch.delta = δ
+	if (git_oid_cmp(&delta->new_file.oid, &delta->old_file.oid) == 0)
+		delta->status = GIT_DELTA_UNMODIFIED;
 
 	if ((error = diff_delta_is_binary_by_content(
-			 &ctxt, &delta, &delta.old_file, &patch.old_data)) < 0 ||
+			&data->ctxt, delta, &delta->old_file, &data->patch.old_data)) < 0 ||
 		(error = diff_delta_is_binary_by_content(
-			&ctxt, &delta, &delta.new_file, &patch.new_data)) < 0)
+			&data->ctxt, delta, &delta->new_file, &data->patch.new_data)) < 0)
 		goto cleanup;
 
-	patch.flags |= GIT_DIFF_PATCH_LOADED;
-	if (delta.binary != 1 && delta.status != GIT_DELTA_UNMODIFIED)
-		patch.flags |= GIT_DIFF_PATCH_DIFFABLE;
+	data->patch.flags |= GIT_DIFF_PATCH_LOADED;
+
+	if (delta->binary != 1 && delta->status != GIT_DELTA_UNMODIFIED)
+		data->patch.flags |= GIT_DIFF_PATCH_DIFFABLE;
 
 	/* do diffs */
 
-	if (!(error = diff_delta_file_callback(&ctxt, patch.delta, 1)))
-		error = diff_patch_generate(&ctxt, &patch);
+	if (!(error = diff_delta_file_callback(&data->ctxt, delta, 1)))
+		error = diff_patch_generate(&data->ctxt, &data->patch);
 
 cleanup:
-	diff_patch_unload(&patch);
-
 	if (error == GIT_EUSER)
 		giterr_clear();
 
-	return error;
-}
+	diff_patch_unload(&data->patch);
 
-static void set_data_from_buffer(
-	char *buffer, size_t buffer_len, git_map *map, git_diff_file *file)
-{
-	file->size = buffer_len;
-	file->mode = 0644;
-
-	map->len   = (size_t)file->size;
-	map->data  = (char *)buffer;
+	return error;
 }
 
-int git_diff_blob_to_buffer(
-	git_blob *old_blob,
-	char *buffer,
-	size_t buffer_len,
+int git_diff_blobs(
+	const git_blob *old_blob,
+	const git_blob *new_blob,
 	const git_diff_options *options,
 	git_diff_file_cb file_cb,
 	git_diff_hunk_cb hunk_cb,
@@ -1348,71 +1346,57 @@ int git_diff_blob_to_buffer(
 	void *payload)
 {
 	int error;
-	git_repository *repo;
-	diff_context ctxt;
-	git_diff_delta delta;
-	git_diff_patch patch;
+	diff_single_data d;
+	git_repository *repo =
+		new_blob ? git_object_owner((const git_object *)new_blob) :
+		old_blob ? git_object_owner((const git_object *)old_blob) : NULL;
 
-	GITERR_CHECK_VERSION(options, GIT_DIFF_OPTIONS_VERSION, "git_diff_options");
+	if ((error = diff_single_init(
+			&d, repo, options, file_cb, hunk_cb, data_cb, payload)) < 0)
+		return error;
 
-	if (old_blob)
-		repo = git_object_owner((git_object *)old_blob);
-	else
-		repo = NULL;
+	if (options && (options->flags & GIT_DIFF_REVERSE) != 0) {
+		const git_blob *swap = old_blob;
+		old_blob = new_blob;
+		new_blob = swap;
+	}
 
-	diff_context_init(
-		&ctxt, NULL, repo, options,
-		file_cb, hunk_cb, data_cb, payload);
+	set_data_from_blob(old_blob, &d.patch.old_data, &d.delta.old_file);
+	set_data_from_blob(new_blob, &d.patch.new_data, &d.delta.new_file);
 
-	diff_patch_init(&ctxt, &patch);
+	return diff_single_apply(&d);
+}
 
-	/* create a fake delta record and simulate diff_patch_load */
+int git_diff_blob_to_buffer(
+	const git_blob *old_blob,
+	const char *buf,
+	size_t buflen,
+	const git_diff_options *options,
+	git_diff_file_cb file_cb,
+	git_diff_hunk_cb hunk_cb,
+	git_diff_data_cb data_cb,
+	void *payload)
+{
+	int error;
+	diff_single_data d;
+	git_repository *repo =
+		old_blob ? git_object_owner((const git_object *)old_blob) : NULL;
 
-	memset(&delta, 0, sizeof(delta));
-	delta.binary = -1;
+	if ((error = diff_single_init(
+			&d, repo, options, file_cb, hunk_cb, data_cb, payload)) < 0)
+		return error;
 
-	if (options && (options->flags & GIT_DIFF_REVERSE)) {
-		set_data_from_blob(old_blob, &patch.new_data, &delta.new_file);
-		set_data_from_buffer(buffer, buffer_len, &patch.old_data, &delta.old_file);
+	if (options && (options->flags & GIT_DIFF_REVERSE) != 0) {
+		set_data_from_buffer(buf, buflen, &d.patch.old_data, &d.delta.old_file);
+		set_data_from_blob(old_blob, &d.patch.new_data, &d.delta.new_file);
 	} else {
-		set_data_from_blob(old_blob, &patch.old_data, &delta.old_file);
-		set_data_from_buffer(buffer, buffer_len, &patch.new_data, &delta.new_file);
+		set_data_from_blob(old_blob, &d.patch.old_data, &d.delta.old_file);
+		set_data_from_buffer(buf, buflen, &d.patch.new_data, &d.delta.new_file);
 	}
 
-	delta.status = buffer ?
-		(old_blob ? GIT_DELTA_MODIFIED : GIT_DELTA_ADDED) :
-		(old_blob ? GIT_DELTA_DELETED : GIT_DELTA_UNTRACKED);
-
-	if (git_oid_cmp(&delta.new_file.oid, &delta.old_file.oid) == 0)
-		delta.status = GIT_DELTA_UNMODIFIED;
-
-	patch.delta = &delta;
-
-	if ((error = diff_delta_is_binary_by_content(
-			 &ctxt, &delta, &delta.old_file, &patch.old_data)) < 0 ||
-		(error = diff_delta_is_binary_by_content(
-			&ctxt, &delta, &delta.new_file, &patch.new_data)) < 0)
-		goto cleanup;
-
-	patch.flags |= GIT_DIFF_PATCH_LOADED;
-	if (delta.binary != 1 && delta.status != GIT_DELTA_UNMODIFIED)
-		patch.flags |= GIT_DIFF_PATCH_DIFFABLE;
-
-	/* do diffs */
-
-	if (!(error = diff_delta_file_callback(&ctxt, patch.delta, 1)))
-		error = diff_patch_generate(&ctxt, &patch);
-
-cleanup:
-	diff_patch_unload(&patch);
-
-	if (error == GIT_EUSER)
-		giterr_clear();
-
-	return error;
+	return diff_single_apply(&d);
 }
 
-
 size_t git_diff_num_deltas(git_diff_list *diff)
 {
 	assert(diff);

tests-clar/diff/blob.c

@@ -347,3 +347,81 @@ void test_diff_blob__can_correctly_detect_a_textual_blob_as_non_binary(void)
 	/* tests/resources/attr/root_test4.txt */
 	cl_assert_equal_i(false, git_blob_is_binary(d));
 }
+
+/*
+ * git_diff_blob_to_buffer tests
+ */
+
+void test_diff_blob__can_compare_blob_to_buffer(void)
+{
+	git_blob *a;
+	git_oid a_oid;
+	const char *a_content = "Hello from the root\n";
+	const char *b_content = "Hello from the root\n\nSome additional lines\n\nDown here below\n\n";
+
+	/* tests/resources/attr/root_test1 */
+	cl_git_pass(git_oid_fromstrn(&a_oid, "45141a79", 8));
+	cl_git_pass(git_blob_lookup_prefix(&a, g_repo, &a_oid, 4));
+
+	/* diff from blob a to content of b */
+	cl_git_pass(git_diff_blob_to_buffer(
+		a, b_content, strlen(b_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_MODIFIED]);
+	cl_assert_equal_i(0, expected.files_binary);
+	cl_assert_equal_i(1, expected.hunks);
+	cl_assert_equal_i(6, expected.lines);
+	cl_assert_equal_i(1, expected.line_ctxt);
+	cl_assert_equal_i(5, expected.line_adds);
+	cl_assert_equal_i(0, expected.line_dels);
+
+	/* diff from blob a to content of a */
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		a, a_content, strlen(a_content),
+		&opts, diff_file_cb, diff_hunk_cb, diff_line_cb, &expected));
+
+	assert_identical_blobs_comparison(&expected);
+
+	/* diff from NULL blob to content of b */
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		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);
+
+	/* diff from blob a to NULL buffer */
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		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);
+
+	/* diff with reverse */
+	opts.flags ^= GIT_DIFF_REVERSE;
+
+	memset(&expected, 0, sizeof(expected));
+	cl_git_pass(git_diff_blob_to_buffer(
+		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);
+
+	git_blob_free(a);
+}