Merge pull request #3923 from libgit2/ethomson/diff-read-empty-binary

Read binary patches (with no binary data)

8 files changed, 86 insertions, 22 deletions

diff --git a/include/git2/diff.h b/include/git2/diff.h
index c5e463fe3..4f0871dab 100644
--- a/include/git2/diff.h
+++ b/include/git2/diff.h
@@ -490,6 +490,14 @@ typedef struct {
 
 /** Structure describing the binary contents of a diff. */
 typedef struct {
+	/**
+	 * Whether there is data in this binary structure or not.  If this
+	 * is `1`, then this was produced and included binary content.  If
+	 * this is `0` then this was generated knowing only that a binary
+	 * file changed but without providing the data, probably from a patch
+	 * that said `Binary files a/file.txt and b/file.txt differ`.
+	 */
+	unsigned int contains_data;
 	git_diff_binary_file old_file; /**< The contents of the old file. */
 	git_diff_binary_file new_file; /**< The contents of the new file. */
 } git_diff_binary;
diff --git a/src/apply.c b/src/apply.c
index 10bf1f492..f70172469 100644
--- a/src/apply.c
+++ b/src/apply.c
@@ -291,7 +291,15 @@ static int apply_binary(
 	git_patch *patch)
 {
 	git_buf reverse = GIT_BUF_INIT;
-	int error;
+	int error = 0;
+
+	if (!patch->binary.contains_data) {
+		error = apply_err("patch does not contain binary data");
+		goto done;
+	}
+
+	if (!patch->binary.old_file.datalen && !patch->binary.new_file.datalen)
+		goto done;
 
 	/* first, apply the new_file delta to the given source */
 	if ((error = apply_binary_delta(out, source, source_len,
diff --git a/src/diff_print.c b/src/diff_print.c
index f72ca8935..fd1a186c1 100644
--- a/src/diff_print.c
+++ b/src/diff_print.c
@@ -500,7 +500,6 @@ static int diff_print_patch_file_binary_noshow(
 			&new_path, new_pfx, delta->new_file.path)) < 0)
 		goto done;
 
-
 	pi->line.num_lines = 1;
 	error = diff_delta_format_with_paths(
 		pi->buf, delta, "Binary files %s and %s differ\n",
@@ -521,13 +520,13 @@ static int diff_print_patch_file_binary(
 	size_t pre_binary_size;
 	int error;
 
-	if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 0)
+	if (delta->status == GIT_DELTA_UNMODIFIED)
+		return 0;
+
+	if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 0 || !binary->contains_data)
 		return diff_print_patch_file_binary_noshow(
 			pi, delta, old_pfx, new_pfx);
 
-	if (binary->new_file.datalen == 0 && binary->old_file.datalen == 0)
-		return 0;
-
 	pre_binary_size = pi->buf->size;
 	git_buf_printf(pi->buf, "GIT binary patch\n");
 	pi->line.num_lines++;
@@ -563,8 +562,11 @@ static int diff_print_patch_file(
 	bool binary = (delta->flags & GIT_DIFF_FLAG_BINARY) ||
 		(pi->flags & GIT_DIFF_FORCE_BINARY);
 	bool show_binary = !!(pi->flags & GIT_DIFF_SHOW_BINARY);
-	int id_strlen = binary && show_binary ?
-		GIT_OID_HEXSZ : pi->id_strlen;
+	int id_strlen = pi->id_strlen;
+
+	if (binary && show_binary)
+		id_strlen = delta->old_file.id_abbrev ? delta->old_file.id_abbrev :
+			delta->new_file.id_abbrev;
 
 	GIT_UNUSED(progress);
 
diff --git a/src/patch.c b/src/patch.c
index ad24b0714..9b7c9c64c 100644
--- a/src/patch.c
+++ b/src/patch.c
@@ -17,6 +17,9 @@ int git_patch__invoke_callbacks(
 	if (file_cb)
 		error = file_cb(patch->delta, 0, payload);
 
+	if (error)
+		return error;
+
 	if ((patch->delta->flags & GIT_DIFF_FLAG_BINARY) != 0) {
 		if (binary_cb)
 			error = binary_cb(patch->delta, &patch->binary, payload);
diff --git a/src/patch_generate.c b/src/patch_generate.c
index 67a13b706..a13f2ff5d 100644
--- a/src/patch_generate.c
+++ b/src/patch_generate.c
@@ -342,7 +342,7 @@ done:
 
 static int diff_binary(git_patch_generated_output *output, git_patch_generated *patch)
 {
-	git_diff_binary binary = {{0}};
+	git_diff_binary binary = {0};
 	const char *old_data = patch->ofile.map.data;
 	const char *new_data = patch->nfile.map.data;
 	size_t old_len = patch->ofile.map.len,
@@ -352,6 +352,8 @@ static int diff_binary(git_patch_generated_output *output, git_patch_generated *
 	/* Only load contents if the user actually wants to diff
 	 * binary files. */
 	if (patch->base.diff_opts.flags & GIT_DIFF_SHOW_BINARY) {
+		binary.contains_data = 1;
+
 		/* Create the old->new delta (as the "new" side of the patch),
 		 * and the new->old delta (as the "old" side)
 		 */
@@ -492,8 +494,17 @@ static int diff_single_generate(patch_generated_with_delta *pd, git_xdiff_output
 	patch_generated_init_common(patch);
 
 	if (pd->delta.status == GIT_DELTA_UNMODIFIED &&
-		!(patch->ofile.opts_flags & GIT_DIFF_INCLUDE_UNMODIFIED))
+		!(patch->ofile.opts_flags & GIT_DIFF_INCLUDE_UNMODIFIED)) {
+
+		/* Even empty patches are flagged as binary, and even though
+		 * there's no difference, we flag this as "containing data"
+		 * (the data is known to be empty, as opposed to wholly unknown).
+		 */
+		if (patch->base.diff_opts.flags & GIT_DIFF_SHOW_BINARY)
+			patch->base.binary.contains_data = 1;
+
 		return error;
+	}
 
 	error = patch_generated_invoke_file_callback(patch, (git_patch_generated_output *)xo);
 
diff --git a/src/patch_parse.c b/src/patch_parse.c
index 44bcf8f75..5ee09ee27 100644
--- a/src/patch_parse.c
+++ b/src/patch_parse.c
@@ -74,8 +74,8 @@ static int parse_advance_expected(
 	return 0;
 }
 
-#define parse_advance_expected_s(ctx, str) \
-	parse_advance_expected(ctx, str, sizeof(str) - 1)
+#define parse_advance_expected_str(ctx, str) \
+	parse_advance_expected(ctx, str, strlen(str))
 
 static int parse_advance_ws(git_patch_parse_ctx *ctx)
 {
@@ -220,7 +220,7 @@ static int parse_header_git_index(
 {
 	if (parse_header_oid(&patch->base.delta->old_file.id,
 			&patch->base.delta->old_file.id_abbrev, ctx) < 0 ||
-		parse_advance_expected_s(ctx, "..") < 0 ||
+		parse_advance_expected_str(ctx, "..") < 0 ||
 		parse_header_oid(&patch->base.delta->new_file.id,
 			&patch->base.delta->new_file.id_abbrev, ctx) < 0)
 		return -1;
@@ -336,7 +336,7 @@ static int parse_header_percent(uint16_t *out, git_patch_parse_ctx *ctx)
 
 	parse_advance_chars(ctx, (end - ctx->line));
 
-	if (parse_advance_expected_s(ctx, "%") < 0)
+	if (parse_advance_expected_str(ctx, "%") < 0)
 		return -1;
 
 	if (val > 100)
@@ -379,6 +379,7 @@ static const header_git_op header_git_ops[] = {
 	{ "diff --git ", NULL },
 	{ "@@ -", NULL },
 	{ "GIT binary patch", NULL },
+	{ "Binary files ", NULL },
 	{ "--- ", parse_header_git_oldpath },
 	{ "+++ ", parse_header_git_newpath },
 	{ "index ", parse_header_git_index },
@@ -404,7 +405,7 @@ static int parse_header_git(
 	int error = 0;
 
 	/* Parse the diff --git line */
-	if (parse_advance_expected_s(ctx, "diff --git ") < 0)
+	if (parse_advance_expected_str(ctx, "diff --git ") < 0)
 		return parse_err("corrupt git diff header at line %d", ctx->line_num);
 
 	if (parse_header_path(&patch->header_old_path, ctx) < 0)
@@ -443,7 +444,7 @@ static int parse_header_git(
 				goto done;
 
 			parse_advance_ws(ctx);
-			parse_advance_expected_s(ctx, "\n");
+			parse_advance_expected_str(ctx, "\n");
 
 			if (ctx->line_len > 0) {
 				error = parse_err("trailing data at line %d", ctx->line_num);
@@ -505,27 +506,27 @@ static int parse_hunk_header(
 	hunk->hunk.old_lines = 1;
 	hunk->hunk.new_lines = 1;
 
-	if (parse_advance_expected_s(ctx, "@@ -") < 0 ||
+	if (parse_advance_expected_str(ctx, "@@ -") < 0 ||
 		parse_int(&hunk->hunk.old_start, ctx) < 0)
 		goto fail;
 
 	if (ctx->line_len > 0 && ctx->line[0] == ',') {
-		if (parse_advance_expected_s(ctx, ",") < 0 ||
+		if (parse_advance_expected_str(ctx, ",") < 0 ||
 			parse_int(&hunk->hunk.old_lines, ctx) < 0)
 			goto fail;
 	}
 
-	if (parse_advance_expected_s(ctx, " +") < 0 ||
+	if (parse_advance_expected_str(ctx, " +") < 0 ||
 		parse_int(&hunk->hunk.new_start, ctx) < 0)
 		goto fail;
 
 	if (ctx->line_len > 0 && ctx->line[0] == ',') {
-		if (parse_advance_expected_s(ctx, ",") < 0 ||
+		if (parse_advance_expected_str(ctx, ",") < 0 ||
 			parse_int(&hunk->hunk.new_lines, ctx) < 0)
 			goto fail;
 	}
 
-	if (parse_advance_expected_s(ctx, " @@") < 0)
+	if (parse_advance_expected_str(ctx, " @@") < 0)
 		goto fail;
 
 	parse_advance_line(ctx);
@@ -782,7 +783,7 @@ static int parse_patch_binary(
 {
 	int error;
 
-	if (parse_advance_expected_s(ctx, "GIT binary patch") < 0 ||
+	if (parse_advance_expected_str(ctx, "GIT binary patch") < 0 ||
 		parse_advance_nl(ctx) < 0)
 		return parse_err("corrupt git binary header at line %d", ctx->line_num);
 
@@ -804,6 +805,24 @@ static int parse_patch_binary(
 		return parse_err("corrupt git binary patch separator at line %d",
 			ctx->line_num);
 
+	patch->base.binary.contains_data = 1;
+	patch->base.delta->flags |= GIT_DIFF_FLAG_BINARY;
+	return 0;
+}
+
+static int parse_patch_binary_nodata(
+	git_patch_parsed *patch,
+	git_patch_parse_ctx *ctx)
+{
+	if (parse_advance_expected_str(ctx, "Binary files ") < 0 ||
+		parse_advance_expected_str(ctx, patch->header_old_path) < 0 ||
+		parse_advance_expected_str(ctx, " and ") < 0 ||
+		parse_advance_expected_str(ctx, patch->header_new_path) < 0 ||
+		parse_advance_expected_str(ctx, " differ") < 0 ||
+		parse_advance_nl(ctx) < 0)
+		return parse_err("corrupt git binary header at line %d", ctx->line_num);
+
+	patch->base.binary.contains_data = 0;
 	patch->base.delta->flags |= GIT_DIFF_FLAG_BINARY;
 	return 0;
 }
@@ -840,6 +859,8 @@ static int parse_patch_body(
 {
 	if (parse_ctx_contains_s(ctx, "GIT binary patch"))
 		return parse_patch_binary(patch, ctx);
+	else if (parse_ctx_contains_s(ctx, "Binary files "))
+		return parse_patch_binary_nodata(patch, ctx);
 	else
 		return parse_patch_hunks(patch, ctx);
 }
diff --git a/tests/patch/patch_common.h b/tests/patch/patch_common.h
index e097062d2..6ec554690 100644
--- a/tests/patch/patch_common.h
+++ b/tests/patch/patch_common.h
@@ -661,3 +661,8 @@
 	"\n" \
 	"delta 48\n" \
 	"mc$~Y)c#%<%fq{_;hPgsAGK(h)CJASj=y9P)1m{m|^9BI99|yz$\n\n"
+
+#define PATCH_BINARY_NOT_PRINTED \
+	"diff --git a/binary.bin b/binary.bin\n" \
+	"index 27184d9..7c94f9e 100644\n" \
+	"Binary files a/binary.bin and b/binary.bin differ\n"
diff --git a/tests/patch/print.c b/tests/patch/print.c
index 5a86573b3..62e50b93e 100644
--- a/tests/patch/print.c
+++ b/tests/patch/print.c
@@ -166,3 +166,9 @@ void test_patch_print__not_reversible(void)
 	patch_print_from_patchfile(PATCH_BINARY_NOT_REVERSIBLE,
 		strlen(PATCH_BINARY_NOT_REVERSIBLE));
 }
+
+void test_patch_print__binary_not_shown(void)
+{
+	patch_print_from_patchfile(PATCH_BINARY_NOT_PRINTED,
+		strlen(PATCH_BINARY_NOT_PRINTED));
+}