Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
react to errors in xdi_diff
When we call into xdiff to perform a diff, we generally lose
the return code completely. Typically by ignoring the return
of our xdi_diff wrapper, but sometimes we even propagate
that return value up and then ignore it later.  This can
lead to us silently producing incorrect diffs (e.g., "git
log" might produce no output at all, not even a diff header,
for a content-level diff).

In practice this does not happen very often, because the
typical reason for xdiff to report failure is that it
malloc() failed (it uses straight malloc, and not our
xmalloc wrapper).  But it could also happen when xdiff
triggers one our callbacks, which returns an error (e.g.,
outf() in builtin/rerere.c tries to report a write failure
in this way). And the next patch also plans to add more
failure modes.

Let's notice an error return from xdiff and react
appropriately. In most of the diff.c code, we can simply
die(), which matches the surrounding code (e.g., that is
what we do if we fail to load a file for diffing in the
first place). This is not that elegant, but we are probably
better off dying to let the user know there was a problem,
rather than simply generating bogus output.

We could also just die() directly in xdi_diff, but the
callers typically have a bit more context, and can provide a
better message (and if we do later decide to pass errors up,
we're one step closer to doing so).

There is one interesting case, which is in diff_grep(). Here
if we cannot generate the diff, there is nothing to match,
and we silently return "no hits". This is actually what the
existing code does already, but we make it a little more
explicit.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Sep 28, 2015
1 parent ecad27c commit 3efb988
Show file tree
Hide file tree
Showing 7 changed files with 41 additions and 24 deletions.
9 changes: 7 additions & 2 deletions builtin/blame.c
Expand Up @@ -972,7 +972,10 @@ static void pass_blame_to_parent(struct scoreboard *sb,
fill_origin_blob(&sb->revs->diffopt, target, &file_o);
num_get_patch++;

diff_hunks(&file_p, &file_o, 0, blame_chunk_cb, &d);
if (diff_hunks(&file_p, &file_o, 0, blame_chunk_cb, &d))
die("unable to generate diff (%s -> %s)",
sha1_to_hex(parent->commit->object.sha1),
sha1_to_hex(target->commit->object.sha1));
/* The rest are the same as the parent */
blame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent);
*d.dstq = NULL;
Expand Down Expand Up @@ -1118,7 +1121,9 @@ static void find_copy_in_blob(struct scoreboard *sb,
* file_p partially may match that image.
*/
memset(split, 0, sizeof(struct blame_entry [3]));
diff_hunks(file_p, &file_o, 1, handle_split_cb, &d);
if (diff_hunks(file_p, &file_o, 1, handle_split_cb, &d))
die("unable to generate diff (%s)",
sha1_to_hex(parent->commit->object.sha1));
/* remainder, if any, all match the preimage */
handle_split(sb, ent, d.tlno, d.plno, ent->num_lines, parent, split);
}
Expand Down
3 changes: 2 additions & 1 deletion builtin/merge-tree.c
Expand Up @@ -118,7 +118,8 @@ static void show_diff(struct merge_list *entry)
if (!dst.ptr)
size = 0;
dst.size = size;
xdi_diff(&src, &dst, &xpp, &xecfg, &ecb);
if (xdi_diff(&src, &dst, &xpp, &xecfg, &ecb))
die("unable to generate diff");
free(src.ptr);
free(dst.ptr);
}
Expand Down
10 changes: 6 additions & 4 deletions builtin/rerere.c
Expand Up @@ -29,9 +29,10 @@ static int diff_two(const char *file1, const char *label1,
xdemitconf_t xecfg;
xdemitcb_t ecb;
mmfile_t minus, plus;
int ret;

if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
return 1;
return -1;

printf("--- a/%s\n+++ b/%s\n", label1, label2);
fflush(stdout);
Expand All @@ -40,11 +41,11 @@ static int diff_two(const char *file1, const char *label1,
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = 3;
ecb.outf = outf;
xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
ret = xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);

free(minus.ptr);
free(plus.ptr);
return 0;
return ret;
}

int cmd_rerere(int argc, const char **argv, const char *prefix)
Expand Down Expand Up @@ -104,7 +105,8 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
for (i = 0; i < merge_rr.nr; i++) {
const char *path = merge_rr.items[i].string;
const char *name = (const char *)merge_rr.items[i].util;
diff_two(rerere_path(name, "preimage"), path, path, path);
if (diff_two(rerere_path(name, "preimage"), path, path, path))
die("unable to generate diff for %s", name);
}
else
usage_with_options(rerere_usage, options);
Expand Down
6 changes: 4 additions & 2 deletions combine-diff.c
Expand Up @@ -419,8 +419,10 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
state.num_parent = num_parent;
state.n = n;

xdi_diff_outf(&parent_file, result_file, consume_line, &state,
&xpp, &xecfg);
if (xdi_diff_outf(&parent_file, result_file, consume_line, &state,
&xpp, &xecfg))
die("unable to generate combined diff for %s",
sha1_to_hex(parent));
free(parent_file.ptr);

/* Assign line numbers for this parent.
Expand Down
26 changes: 16 additions & 10 deletions diff.c
Expand Up @@ -1002,8 +1002,9 @@ static void diff_words_show(struct diff_words_data *diff_words)
xpp.flags = 0;
/* as only the hunk header will be parsed, we need a 0-context */
xecfg.ctxlen = 0;
xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,
&xpp, &xecfg);
if (xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,
&xpp, &xecfg))
die("unable to generate word diff");
free(minus.ptr);
free(plus.ptr);
if (diff_words->current_plus != diff_words->plus.text.ptr +
Expand Down Expand Up @@ -2400,8 +2401,9 @@ static void builtin_diff(const char *name_a,
xecfg.ctxlen = strtoul(v, NULL, 10);
if (o->word_diff)
init_diff_words_data(&ecbdata, o, one, two);
xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
&xpp, &xecfg);
if (xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
&xpp, &xecfg))
die("unable to generate diff for %s", one->path);
if (o->word_diff)
free_diff_words_data(&ecbdata);
if (textconv_one)
Expand Down Expand Up @@ -2478,8 +2480,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
xpp.flags = o->xdl_opts;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
&xpp, &xecfg);
if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
&xpp, &xecfg))
die("unable to generate diffstat for %s", one->path);
}

diff_free_filespec_data(one);
Expand Down Expand Up @@ -2525,8 +2528,9 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = 1; /* at least one context line */
xpp.flags = 0;
xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,
&xpp, &xecfg);
if (xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,
&xpp, &xecfg))
die("unable to generate checkdiff for %s", one->path);

if (data.ws_rule & WS_BLANK_AT_EOF) {
struct emit_callback ecbdata;
Expand Down Expand Up @@ -4425,8 +4429,10 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
xpp.flags = 0;
xecfg.ctxlen = 3;
xecfg.flags = 0;
xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
&xpp, &xecfg);
if (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
&xpp, &xecfg))
return error("unable to generate patch-id diff for %s",
p->one->path);
}

git_SHA1_Final(sha1, &ctx);
Expand Down
4 changes: 2 additions & 2 deletions diffcore-pickaxe.c
Expand Up @@ -62,8 +62,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
ecbdata.hit = 0;
xecfg.ctxlen = o->context;
xecfg.interhunkctxlen = o->interhunkcontext;
xdi_diff_outf(one, two, diffgrep_consume, &ecbdata,
&xpp, &xecfg);
if (xdi_diff_outf(one, two, diffgrep_consume, &ecbdata, &xpp, &xecfg))
return 0;
return ecbdata.hit;
}

Expand Down
7 changes: 4 additions & 3 deletions line-log.c
Expand Up @@ -325,7 +325,7 @@ static int collect_diff_cb(long start_a, long count_a,
return 0;
}

static void collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
static int collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
{
struct collect_diff_cbdata cbdata = {NULL};
xpparam_t xpp;
Expand All @@ -340,7 +340,7 @@ static void collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges
xecfg.hunk_func = collect_diff_cb;
memset(&ecb, 0, sizeof(ecb));
ecb.priv = &cbdata;
xdi_diff(parent, target, &xpp, &xecfg, &ecb);
return xdi_diff(parent, target, &xpp, &xecfg, &ecb);
}

/*
Expand Down Expand Up @@ -1030,7 +1030,8 @@ static int process_diff_filepair(struct rev_info *rev,
}

diff_ranges_init(&diff);
collect_diff(&file_parent, &file_target, &diff);
if (collect_diff(&file_parent, &file_target, &diff))
die("unable to generate diff for %s", pair->one->path);

/* NEEDSWORK should apply some heuristics to prevent mismatches */
free(rg->path);
Expand Down

0 comments on commit 3efb988

Please sign in to comment.