diff: treat binary patches with no data specialethomson/diff-only-load-binaries-when-requested

When creating and printing diffs, deal with binary deltas that have
binary data specially, versus diffs that have a binary file but lack the
actual binary data.

8 files changed, 64 insertions, 19 deletions

diff --git a/include/git2/diff.h b/include/git2/diff.h
index c5e463fe3..70664637b 100644
--- a/include/git2/diff.h
+++ b/include/git2/diff.h
@@ -490,6 +490,13 @@ 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 non-zero, then this was generated knowing only that a binary
+	 * file changed but without providing the data.  For example, seeing
+	 * only `BBinary files a/file.txt and b/file.txt differ`.
+	 */
+	unsigned int empty_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..f05fc7c2f 100644
--- a/src/apply.c
+++ b/src/apply.c
@@ -293,6 +293,11 @@ static int apply_binary(
 	git_buf reverse = GIT_BUF_INIT;
 	int error;
 
+	if (patch->binary.empty_data) {
+		error = apply_err("patch does not contain binary data");
+		goto done;
+	}
+
 	/* first, apply the new_file delta to the given source */
 	if ((error = apply_binary_delta(out, source, source_len,
 			&patch->binary.new_file)) < 0)
diff --git a/src/diff_print.c b/src/diff_print.c
index f72ca8935..c19693251 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,10 @@ static int diff_print_patch_file_binary(
 	size_t pre_binary_size;
 	int error;
 
-	if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 0)
+	if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 0 || binary->empty_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++;
@@ -564,7 +560,8 @@ static int diff_print_patch_file(
 		(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;
+		MAX(delta->old_file.id_abbrev, delta->new_file.id_abbrev) :
+		pi->id_strlen;
 
 	GIT_UNUSED(progress);
 
diff --git a/src/patch_generate.c b/src/patch_generate.c
index 67a13b706..d0103a85c 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,
@@ -366,6 +366,8 @@ static int diff_binary(git_patch_generated_output *output, git_patch_generated *
 				&binary.new_file.inflatedlen,
 				old_data, old_len, new_data, new_len)) < 0)
 			return error;
+	} else {
+		binary.empty_data = 1;
 	}
 
 	error = giterr_set_after_callback_function(
diff --git a/src/patch_parse.c b/src/patch_parse.c
index 44bcf8f75..95c381b2e 100644
--- a/src/patch_parse.c
+++ b/src/patch_parse.c
@@ -74,7 +74,10 @@ static int parse_advance_expected(
 	return 0;
 }
 
-#define parse_advance_expected_s(ctx, str) \
+#define parse_advance_expected_str(ctx, str) \
+	parse_advance_expected(ctx, str, strlen(str))
+
+#define parse_advance_expected_const_str(ctx, str) \
 	parse_advance_expected(ctx, str, sizeof(str) - 1)
 
 static int parse_advance_ws(git_patch_parse_ctx *ctx)
@@ -220,7 +223,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_const_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 +339,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_const_str(ctx, "%") < 0)
 		return -1;
 
 	if (val > 100)
@@ -379,6 +382,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 +408,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_const_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 +447,7 @@ static int parse_header_git(
 				goto done;
 
 			parse_advance_ws(ctx);
-			parse_advance_expected_s(ctx, "\n");
+			parse_advance_expected_const_str(ctx, "\n");
 
 			if (ctx->line_len > 0) {
 				error = parse_err("trailing data at line %d", ctx->line_num);
@@ -505,27 +509,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_const_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_const_str(ctx, ",") < 0 ||
 			parse_int(&hunk->hunk.old_lines, ctx) < 0)
 			goto fail;
 	}
 
-	if (parse_advance_expected_s(ctx, " +") < 0 ||
+	if (parse_advance_expected_const_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_const_str(ctx, ",") < 0 ||
 			parse_int(&hunk->hunk.new_lines, ctx) < 0)
 			goto fail;
 	}
 
-	if (parse_advance_expected_s(ctx, " @@") < 0)
+	if (parse_advance_expected_const_str(ctx, " @@") < 0)
 		goto fail;
 
 	parse_advance_line(ctx);
@@ -782,7 +786,7 @@ static int parse_patch_binary(
 {
 	int error;
 
-	if (parse_advance_expected_s(ctx, "GIT binary patch") < 0 ||
+	if (parse_advance_expected_const_str(ctx, "GIT binary patch") < 0 ||
 		parse_advance_nl(ctx) < 0)
 		return parse_err("corrupt git binary header at line %d", ctx->line_num);
 
@@ -808,6 +812,23 @@ static int parse_patch_binary(
 	return 0;
 }
 
+static int parse_patch_binary_nodata(
+	git_patch_parsed *patch,
+	git_patch_parse_ctx *ctx)
+{
+	if (parse_advance_expected_const_str(ctx, "Binary files ") < 0 ||
+		parse_advance_expected_str(ctx, patch->header_old_path) < 0 ||
+		parse_advance_expected_const_str(ctx, " and ") < 0 ||
+		parse_advance_expected_str(ctx, patch->header_new_path) < 0 ||
+		parse_advance_expected_const_str(ctx, " differ") < 0 ||
+		parse_advance_nl(ctx) < 0)
+		return parse_err("corrupt git binary header at line %d", ctx->line_num);
+
+	patch->base.delta->flags |= GIT_DIFF_FLAG_BINARY;
+	patch->base.binary.empty_data = 1;
+	return 0;
+}
+
 static int parse_patch_hunks(
 	git_patch_parsed *patch,
 	git_patch_parse_ctx *ctx)
@@ -840,6 +861,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/apply/fromdiff.c b/tests/apply/fromdiff.c
index 349773964..acaa63356 100644
--- a/tests/apply/fromdiff.c
+++ b/tests/apply/fromdiff.c
@@ -240,7 +240,7 @@ void test_apply_fromdiff__binary_no_change(void)
 	cl_git_pass(apply_gitbuf(
 		&original, "binary.bin",
 		&original, "binary.bin",
-		"", &binary_opts));
+		"GIT binary patch\nliteral 0\n\nliteral 0\n\n", &binary_opts));
 }
 
 void test_apply_fromdiff__binary_change_delta(void)
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));
+}