From 4bfd7c63fcd920af8366e521b7e985ded7cb3e84 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Thu, 1 Sep 2016 16:55:27 -0500 Subject: [PATCH 1/3] patch: error on diff callback failure --- src/patch.c | 3 +++ 1 file changed, 3 insertions(+) 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); From f4e3dae75ff7246952f6707ad2a2fdea758e03ea Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Fri, 2 Sep 2016 11:26:16 -0500 Subject: [PATCH 2/3] diff_print: change test for skipping binary printing Instead of skipping printing a binary diff when there is no data, skip printing when we have a status of `UNMODIFIED`. This is more in-line with our internal data model and allows us to expand the notion of binary data. In the future, there may have no data because the files were unmodified (there was no data to produce) or it may have no data because there was no data given to us in a patch. We want to treat these cases separately. --- src/diff_print.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/diff_print.c b/src/diff_print.c index f72ca8935..264bd19e9 100644 --- a/src/diff_print.c +++ b/src/diff_print.c @@ -521,13 +521,13 @@ static int diff_print_patch_file_binary( size_t pre_binary_size; int error; + if (delta->status == GIT_DELTA_UNMODIFIED) + return 0; + if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 0) 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++; From adedac5aba9e4525475fd59d751cd02c6f2b3a4f Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Fri, 2 Sep 2016 02:03:45 -0500 Subject: [PATCH 3/3] diff: treat binary patches with no data special 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. --- include/git2/diff.h | 8 +++++++ src/apply.c | 10 ++++++++- src/diff_print.c | 10 +++++---- src/patch_generate.c | 15 +++++++++++-- src/patch_parse.c | 45 ++++++++++++++++++++++++++++---------- tests/patch/patch_common.h | 5 +++++ tests/patch/print.c | 6 +++++ 7 files changed, 80 insertions(+), 19 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 264bd19e9..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", @@ -524,7 +523,7 @@ static int diff_print_patch_file_binary( if (delta->status == GIT_DELTA_UNMODIFIED) return 0; - if ((pi->flags & GIT_DIFF_SHOW_BINARY) == 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); @@ -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_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)); +}