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>

Jeff King committed Sep 24, 2015 at 19:12 UTC 3efb988098858bf6b974b1e673a190f9d2965d1d
7 files changed +41 -24
builtin/blame.c
+7 -2
@@ -972,7 +972,10 @@ static void pass_blame_to_parent(struct scoreboard *sb,
972 fill_origin_blob(&sb->revs->diffopt, target, &file_o);
973 num_get_patch++;
974
975 - diff_hunks(&file_p, &file_o, 0, blame_chunk_cb, &d);
975 + if (diff_hunks(&file_p, &file_o, 0, blame_chunk_cb, &d))
976 + die("unable to generate diff (%s -> %s)",
977 + sha1_to_hex(parent->commit->object.sha1),
978 + sha1_to_hex(target->commit->object.sha1));
979 /* The rest are the same as the parent */
980 blame_chunk(&d.dstq, &d.srcq, INT_MAX, d.offset, INT_MAX, parent);
981 *d.dstq = NULL;
@@ -1118,7 +1121,9 @@ static void find_copy_in_blob(struct scoreboard *sb,
1121 * file_p partially may match that image.
1122 */
1123 memset(split, 0, sizeof(struct blame_entry [3]));
1121 - diff_hunks(file_p, &file_o, 1, handle_split_cb, &d);
1124 + if (diff_hunks(file_p, &file_o, 1, handle_split_cb, &d))
1125 + die("unable to generate diff (%s)",
1126 + sha1_to_hex(parent->commit->object.sha1));
1127 /* remainder, if any, all match the preimage */
1128 handle_split(sb, ent, d.tlno, d.plno, ent->num_lines, parent, split);
1129 }
builtin/merge-tree.c
+2 -1
@@ -118,7 +118,8 @@ static void show_diff(struct merge_list *entry)
118 if (!dst.ptr)
119 size = 0;
120 dst.size = size;
121 - xdi_diff(&src, &dst, &xpp, &xecfg, &ecb);
121 + if (xdi_diff(&src, &dst, &xpp, &xecfg, &ecb))
122 + die("unable to generate diff");
123 free(src.ptr);
124 free(dst.ptr);
125 }
builtin/rerere.c
+6 -4
@@ -29,9 +29,10 @@ static int diff_two(const char *file1, const char *label1,
29 xdemitconf_t xecfg;
30 xdemitcb_t ecb;
31 mmfile_t minus, plus;
32 + int ret;
33
34 if (read_mmfile(&minus, file1) || read_mmfile(&plus, file2))
34 - return 1;
35 + return -1;
36
37 printf("--- a/%s\n+++ b/%s\n", label1, label2);
38 fflush(stdout);
@@ -40,11 +41,11 @@ static int diff_two(const char *file1, const char *label1,
41 memset(&xecfg, 0, sizeof(xecfg));
42 xecfg.ctxlen = 3;
43 ecb.outf = outf;
43 - xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
44 + ret = xdi_diff(&minus, &plus, &xpp, &xecfg, &ecb);
45
46 free(minus.ptr);
47 free(plus.ptr);
47 - return 0;
48 + return ret;
49 }
50
51 int cmd_rerere(int argc, const char **argv, const char *prefix)
@@ -104,7 +105,8 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
105 for (i = 0; i < merge_rr.nr; i++) {
106 const char *path = merge_rr.items[i].string;
107 const char *name = (const char *)merge_rr.items[i].util;
107 - diff_two(rerere_path(name, "preimage"), path, path, path);
108 + if (diff_two(rerere_path(name, "preimage"), path, path, path))
109 + die("unable to generate diff for %s", name);
110 }
111 else
112 usage_with_options(rerere_usage, options);
combine-diff.c
+4 -2
@@ -419,8 +419,10 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
419 state.num_parent = num_parent;
420 state.n = n;
421
422 - xdi_diff_outf(&parent_file, result_file, consume_line, &state,
423 - &xpp, &xecfg);
422 + if (xdi_diff_outf(&parent_file, result_file, consume_line, &state,
423 + &xpp, &xecfg))
424 + die("unable to generate combined diff for %s",
425 + sha1_to_hex(parent));
426 free(parent_file.ptr);
427
428 /* Assign line numbers for this parent.
diff.c
+16 -10
@@ -1002,8 +1002,9 @@ static void diff_words_show(struct diff_words_data *diff_words)
1002 xpp.flags = 0;
1003 /* as only the hunk header will be parsed, we need a 0-context */
1004 xecfg.ctxlen = 0;
1005 - xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,
1006 - &xpp, &xecfg);
1005 + if (xdi_diff_outf(&minus, &plus, fn_out_diff_words_aux, diff_words,
1006 + &xpp, &xecfg))
1007 + die("unable to generate word diff");
1008 free(minus.ptr);
1009 free(plus.ptr);
1010 if (diff_words->current_plus != diff_words->plus.text.ptr +
@@ -2400,8 +2401,9 @@ static void builtin_diff(const char *name_a,
2401 xecfg.ctxlen = strtoul(v, NULL, 10);
2402 if (o->word_diff)
2403 init_diff_words_data(&ecbdata, o, one, two);
2403 - xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
2404 - &xpp, &xecfg);
2404 + if (xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
2405 + &xpp, &xecfg))
2406 + die("unable to generate diff for %s", one->path);
2407 if (o->word_diff)
2408 free_diff_words_data(&ecbdata);
2409 if (textconv_one)
@@ -2478,8 +2480,9 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
2480 xpp.flags = o->xdl_opts;
2481 xecfg.ctxlen = o->context;
2482 xecfg.interhunkctxlen = o->interhunkcontext;
2481 - xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
2482 - &xpp, &xecfg);
2483 + if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
2484 + &xpp, &xecfg))
2485 + die("unable to generate diffstat for %s", one->path);
2486 }
2487
2488 diff_free_filespec_data(one);
@@ -2525,8 +2528,9 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
2528 memset(&xecfg, 0, sizeof(xecfg));
2529 xecfg.ctxlen = 1; /* at least one context line */
2530 xpp.flags = 0;
2528 - xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,
2529 - &xpp, &xecfg);
2531 + if (xdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,
2532 + &xpp, &xecfg))
2533 + die("unable to generate checkdiff for %s", one->path);
2534
2535 if (data.ws_rule & WS_BLANK_AT_EOF) {
2536 struct emit_callback ecbdata;
@@ -4425,8 +4429,10 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
4429 xpp.flags = 0;
4430 xecfg.ctxlen = 3;
4431 xecfg.flags = 0;
4428 - xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
4429 - &xpp, &xecfg);
4432 + if (xdi_diff_outf(&mf1, &mf2, patch_id_consume, &data,
4433 + &xpp, &xecfg))
4434 + return error("unable to generate patch-id diff for %s",
4435 + p->one->path);
4436 }
4437
4438 git_SHA1_Final(sha1, &ctx);
diffcore-pickaxe.c
+2 -2
@@ -62,8 +62,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
62 ecbdata.hit = 0;
63 xecfg.ctxlen = o->context;
64 xecfg.interhunkctxlen = o->interhunkcontext;
65 - xdi_diff_outf(one, two, diffgrep_consume, &ecbdata,
66 - &xpp, &xecfg);
65 + if (xdi_diff_outf(one, two, diffgrep_consume, &ecbdata, &xpp, &xecfg))
66 + return 0;
67 return ecbdata.hit;
68 }
69
line-log.c
+4 -3
@@ -325,7 +325,7 @@ static int collect_diff_cb(long start_a, long count_a,
325 return 0;
326 }
327
328 -static void collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
328 +static int collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
329 {
330 struct collect_diff_cbdata cbdata = {NULL};
331 xpparam_t xpp;
@@ -340,7 +340,7 @@ static void collect_diff(mmfile_t *parent, mmfile_t *target, struct diff_ranges
340 xecfg.hunk_func = collect_diff_cb;
341 memset(&ecb, 0, sizeof(ecb));
342 ecb.priv = &cbdata;
343 - xdi_diff(parent, target, &xpp, &xecfg, &ecb);
343 + return xdi_diff(parent, target, &xpp, &xecfg, &ecb);
344 }
345
346 /*
@@ -1030,7 +1030,8 @@ static int process_diff_filepair(struct rev_info *rev,
1030 }
1031
1032 diff_ranges_init(&diff);
1033 - collect_diff(&file_parent, &file_target, &diff);
1033 + if (collect_diff(&file_parent, &file_target, &diff))
1034 + die("unable to generate diff for %s", pair->one->path);
1035
1036 /* NEEDSWORK should apply some heuristics to prevent mismatches */
1037 free(rg->path);