Read binary patches (with no binary data) by ethomson · Pull Request #3923 · libgit2/libgit2 · GitHub
Skip to content

Read binary patches (with no binary data) #3923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ethomson merged 3 commits into from
Sep 6, 2016
Merged

Read binary patches (with no binary data) #3923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bfd7c6
patch: error on diff callback failure
Sep 1, 2016
f4e3dae
diff_print: change test for skipping binary printing
Sep 2, 2016
adedac5
diff: treat binary patches with no data special
Sep 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions include/git2/diff.h
View file
Open in desktop
10 changes: 9 additions & 1 deletion src/apply.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

@pks-t pks-t Sep 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this even be triggered? If contains_data == 0, we'll usually have old_file.datalen == 0 && new_file.datalen == 0 due to the patch not being parsed. So actually, we'd already skip this check due to the previous check. I'd rather expect that this statement and the previous statement needs to be swapped.

@ethomson ethomson Sep 5, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right. I did flip these checks, which covered up the fact that I wasn't successfully setting contains_data = 1 when we were creating a diff from unmodified binary files. Fixed this up.


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,
Expand Down
16 changes: 9 additions & 7 deletions src/diff_print.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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++;
Expand Down Expand Up @@ -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);

Expand Down
3 changes: 3 additions & 0 deletions src/patch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 13 additions & 2 deletions src/patch_generate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ static int create_binary(

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,
Expand All @@ -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)
*/
Expand Down Expand Up @@ -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);

Expand Down
45 changes: 33 additions & 12 deletions src/patch_parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 },
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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);

@pks-t pks-t Sep 5, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering: can it happen that we get as input a patch with patch->base.binary.contains_data == 1? Probably not, but maybe we should still explicitly set contains_data = 0 to make it more explicit.

@ethomson ethomson Sep 5, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only when we're actually given binary data, eg GIT binary patch..., which will branch to parse_patch_binary_data. patch->base.binary gets the memset(0) treatment, so the default is contains_data = 0, but I will make this more explicit.

patch->base.binary.contains_data = 0;
patch->base.delta->flags |= GIT_DIFF_FLAG_BINARY;
return 0;
}
Expand Down Expand Up @@ -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);
}
Expand Down
5 changes: 5 additions & 0 deletions tests/patch/patch_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
6 changes: 6 additions & 0 deletions tests/patch/print.c
View file
Open in desktop