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>
diff --git a/builtin/blame.c b/builtin/blame.c index 2b1f9dd..66b4fbf 100644 --- a/builtin/blame.c +++ b/builtin/blame.c
@@ -972,7 +972,10 @@ 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; @@ -1118,7 +1121,9 @@ * 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); }
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index f9ab485..2a4aafe 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c
@@ -118,7 +118,8 @@ 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); }
diff --git a/builtin/rerere.c b/builtin/rerere.c index 98eb8c5..aab8f3b 100644 --- a/builtin/rerere.c +++ b/builtin/rerere.c
@@ -29,9 +29,10 @@ 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); @@ -40,11 +41,11 @@ 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) @@ -104,7 +105,8 @@ 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);
diff --git a/combine-diff.c b/combine-diff.c index 91edce5..284bec6 100644 --- a/combine-diff.c +++ b/combine-diff.c
@@ -419,8 +419,10 @@ 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.
diff --git a/diff.c b/diff.c index 7500c55..6bbf28b 100644 --- a/diff.c +++ b/diff.c
@@ -1002,8 +1002,9 @@ 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 + @@ -2400,8 +2401,9 @@ 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) @@ -2478,8 +2480,9 @@ 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); @@ -2525,8 +2528,9 @@ 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; @@ -4425,8 +4429,10 @@ 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);
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c index 185f86b..7715c13 100644 --- a/diffcore-pickaxe.c +++ b/diffcore-pickaxe.c
@@ -62,8 +62,8 @@ 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; }
diff --git a/line-log.c b/line-log.c index 1a6bc59..d4e29a5 100644 --- a/line-log.c +++ b/line-log.c
@@ -325,7 +325,7 @@ 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; @@ -340,7 +340,7 @@ 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); } /* @@ -1030,7 +1030,8 @@ } 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);