Merge pull request libgit2#4160 from pks-t/pks/diff-fixes

Diff fixes

Author: pks-t

src/diff.c

@@ -120,6 +120,41 @@ int git_diff_get_perfdata(git_diff_perfdata *out, const git_diff *diff)
 	return 0;
 }
 
+int git_diff_foreach(
+	git_diff *diff,
+	git_diff_file_cb file_cb,
+	git_diff_binary_cb binary_cb,
+	git_diff_hunk_cb hunk_cb,
+	git_diff_line_cb data_cb,
+	void *payload)
+{
+	int error = 0;
+	git_diff_delta *delta;
+	size_t idx;
+
+	assert(diff);
+
+	git_vector_foreach(&diff->deltas, idx, delta) {
+		git_patch *patch;
+
+		/* check flags against patch status */
+		if (git_diff_delta__should_skip(&diff->opts, delta))
+			continue;
+
+		if ((error = git_patch_from_diff(&patch, diff, idx)) != 0)
+			break;
+
+		error = git_patch__invoke_callbacks(patch, file_cb, binary_cb,
+						    hunk_cb, data_cb, payload);
+		git_patch_free(patch);
+
+		if (error)
+			break;
+	}
+
+	return error;
+}
+
 int git_diff_format_email__append_header_tobuf(
 	git_buf *out,
 	const git_oid *id,

src/diff_parse.c

@@ -37,14 +37,16 @@ static git_diff_parsed *diff_parsed_alloc(void)
 
 	GIT_REFCOUNT_INC(diff);
 	diff->base.type = GIT_DIFF_TYPE_PARSED;
-	diff->base.opts.flags &= ~GIT_DIFF_IGNORE_CASE;
 	diff->base.strcomp = git__strcmp;
 	diff->base.strncomp = git__strncmp;
 	diff->base.pfxcomp = git__prefixcmp;
 	diff->base.entrycomp = git_diff__entry_cmp;
 	diff->base.patch_fn = git_patch_parsed_from_diff;
 	diff->base.free_fn = diff_parsed_free;
 
+	git_diff_init_options(&diff->base.opts, GIT_DIFF_OPTIONS_VERSION);
+	diff->base.opts.flags &= ~GIT_DIFF_IGNORE_CASE;
+
 	git_pool_init(&diff->base.pool, 1);
 
 	if (git_vector_init(&diff->patches, 0, NULL) < 0 ||

src/patch_generate.c

@@ -206,35 +206,14 @@ static int patch_generated_load(git_patch_generated *patch, git_patch_generated_
 		 ((patch->nfile.flags & GIT_DIFF_FLAG__NO_DATA) != 0 ||
 		  (patch->nfile.file->flags & GIT_DIFF_FLAG_VALID_ID) != 0));
 
-	/* always try to load workdir content first because filtering may
-	 * need 2x data size and this minimizes peak memory footprint
-	 */
-	if (patch->ofile.src == GIT_ITERATOR_TYPE_WORKDIR) {
-		if ((error = git_diff_file_content__load(
-				&patch->ofile, &patch->base.diff_opts)) < 0 ||
-			should_skip_binary(patch, patch->ofile.file))
-			goto cleanup;
-	}
-	if (patch->nfile.src == GIT_ITERATOR_TYPE_WORKDIR) {
-		if ((error = git_diff_file_content__load(
-				&patch->nfile, &patch->base.diff_opts)) < 0 ||
-			should_skip_binary(patch, patch->nfile.file))
-			goto cleanup;
-	}
-
-	/* once workdir has been tried, load other data as needed */
-	if (patch->ofile.src != GIT_ITERATOR_TYPE_WORKDIR) {
-		if ((error = git_diff_file_content__load(
-				&patch->ofile, &patch->base.diff_opts)) < 0 ||
-			should_skip_binary(patch, patch->ofile.file))
-			goto cleanup;
-	}
-	if (patch->nfile.src != GIT_ITERATOR_TYPE_WORKDIR) {
-		if ((error = git_diff_file_content__load(
-				&patch->nfile, &patch->base.diff_opts)) < 0 ||
-			should_skip_binary(patch, patch->nfile.file))
-			goto cleanup;
-	}
+	if ((error = git_diff_file_content__load(
+			&patch->ofile, &patch->base.diff_opts)) < 0 ||
+		should_skip_binary(patch, patch->ofile.file))
+		goto cleanup;
+	if ((error = git_diff_file_content__load(
+			&patch->nfile, &patch->base.diff_opts)) < 0 ||
+		should_skip_binary(patch, patch->nfile.file))
+		goto cleanup;
 
 	/* if previously missing an oid, and now that we have it the two sides
 	 * are the same (and not submodules), update MODIFIED -> UNMODIFIED
@@ -421,56 +400,6 @@ static int diff_required(git_diff *diff, const char *action)
 	return -1;
 }
 
-int git_diff_foreach(
-	git_diff *diff,
-	git_diff_file_cb file_cb,
-	git_diff_binary_cb binary_cb,
-	git_diff_hunk_cb hunk_cb,
-	git_diff_line_cb data_cb,
-	void *payload)
-{
-	int error = 0;
-	git_xdiff_output xo;
-	size_t idx;
-	git_patch_generated patch;
-
-	if ((error = diff_required(diff, "git_diff_foreach")) < 0)
-		return error;
-
-	memset(&xo, 0, sizeof(xo));
-	memset(&patch, 0, sizeof(patch));
-	diff_output_init(
-		&xo.output, &diff->opts, file_cb, binary_cb, hunk_cb, data_cb, payload);
-	git_xdiff_init(&xo, &diff->opts);
-
-	git_vector_foreach(&diff->deltas, idx, patch.base.delta) {
-
-		/* check flags against patch status */
-		if (git_diff_delta__should_skip(&diff->opts, patch.base.delta))
-			continue;
-
-		if (binary_cb || hunk_cb || data_cb) {
-			if ((error = patch_generated_init(&patch, diff, idx)) != 0 ||
-				(error = patch_generated_load(&patch, &xo.output)) != 0) {
-				git_patch_free(&patch.base);
-				return error;
-			}
-		}
-
-		if ((error = patch_generated_invoke_file_callback(&patch, &xo.output)) == 0) {
-			if (binary_cb || hunk_cb || data_cb)
-					error = patch_generated_create(&patch, &xo.output);
-		}
-
-		git_patch_free(&patch.base);
-
-		if (error)
-			break;
-	}
-
-	return error;
-}
-
 typedef struct {
 	git_patch_generated patch;
 	git_diff_delta delta;

src/patch_parse.c

@@ -562,8 +562,9 @@ static int parse_hunk_body(
 	int newlines = hunk->hunk.new_lines;
 
 	for (;
-		ctx->remain_len > 4 && (oldlines || newlines) &&
-		memcmp(ctx->line, "@@ -", 4) != 0;
+		ctx->remain_len > 1 &&
+		(oldlines || newlines) &&
+		(ctx->remain_len <= 4 || memcmp(ctx->line, "@@ -", 4) != 0);
 		parse_advance_line(ctx)) {
 
 		int origin;

tests/diff/parse.c

@@ -196,3 +196,74 @@ void test_diff_parse__get_patch_from_diff(void)
 
 	cl_git_sandbox_cleanup();
 }
+
+static int file_cb(const git_diff_delta *delta, float progress, void *payload)
+{
+    int *called = (int *) payload;
+    GIT_UNUSED(delta);
+    GIT_UNUSED(progress);
+    (*called)++;
+    return 0;
+}
+
+void test_diff_parse__foreach_works_with_parsed_patch(void)
+{
+	const char patch[] =
+	    "diff --git a/obj1 b/obj2\n"
+	    "index 1234567..7654321 10644\n"
+	    "--- a/obj1\n"
+	    "+++ b/obj2\n"
+	    "@@ -1 +1 @@\n"
+	    "-abcde\n"
+	    "+12345\n";
+	int called = 0;
+	git_diff *diff;
+
+	cl_git_pass(git_diff_from_buffer(&diff, patch, strlen(patch)));
+	cl_git_pass(git_diff_foreach(diff, file_cb, NULL, NULL, NULL, &called));
+	cl_assert_equal_i(called, 1);
+
+	git_diff_free(diff);
+}
+
+void test_diff_parse__parsing_minimal_patch_succeeds(void)
+{
+	const char patch[] =
+	    "diff --git a/obj1 b/obj2\n"
+	    "index 1234567..7654321 10644\n"
+	    "--- a/obj1\n"
+	    "+++ b/obj2\n"
+	    "@@ -1 +1 @@\n"
+	    "-a\n"
+	    "+\n";
+	git_buf buf = GIT_BUF_INIT;
+	git_diff *diff;
+
+	cl_git_pass(git_diff_from_buffer(&diff, patch, strlen(patch)));
+	cl_git_pass(git_diff_to_buf(&buf, diff, GIT_DIFF_FORMAT_PATCH));
+	cl_assert_equal_s(patch, buf.ptr);
+
+	git_diff_free(diff);
+	git_buf_free(&buf);
+}
+
+void test_diff_parse__patch_roundtrip_succeeds(void)
+{
+	const char buf1[] = "a\n", buf2[] = "b\n";
+	git_buf patchbuf = GIT_BUF_INIT, diffbuf = GIT_BUF_INIT;
+	git_patch *patch;
+	git_diff *diff;
+
+	cl_git_pass(git_patch_from_buffers(&patch, buf1, strlen(buf1), "obj1", buf2, strlen(buf2), "obj2", NULL));
+	cl_git_pass(git_patch_to_buf(&patchbuf, patch));
+
+	cl_git_pass(git_diff_from_buffer(&diff, patchbuf.ptr, patchbuf.size));
+	cl_git_pass(git_diff_to_buf(&diffbuf, diff, GIT_DIFF_FORMAT_PATCH));
+
+	cl_assert_equal_s(patchbuf.ptr, diffbuf.ptr);
+
+	git_patch_free(patch);
+	git_diff_free(diff);
+	git_buf_free(&patchbuf);
+	git_buf_free(&diffbuf);
+}