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 2b1f9dd6cd01be506c65441685b8d4b79cc74010..66b4fbf466539a0947b0d6dde9e138b7cc0ed632 100644 (file)
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -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;
@@ -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);
 }

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index f9ab48597e58ed97fc208b58c17b8becb105e095..2a4aafec6a906e22713b88d90f350071bfdb1d59 100644 (file)
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -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);
 }

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 98eb8c5404914e4046fdb886866b611351deeacf..aab8f3b1f00fb9f5595e4485529759df89283a89 100644 (file)
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -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);
@@ -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)
@@ -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);

diff --git a/combine-diff.c b/combine-diff.c
index 91edce58e15b82428fcc5f3b006e23bf5380d38c..284bec6ad50de768f04a66d738d3655eff64a2c4 100644 (file)
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -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.

diff --git a/diff.c b/diff.c
index 7500c5509550ccd9a86d620e06c51a71d61f8b6c..6bbf28bff2c8b04281c4a3224dd424990a17a153 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -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 +
@@ -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)
@@ -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);
@@ -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;
@@ -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);

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 185f86b2840d3337eac9fb2b17b260ca0c53fbab..7715c13ec4780a755ec2a6552c0aec9994691087 100644 (file)
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -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;
 }
 

diff --git a/line-log.c b/line-log.c
index 1a6bc5921b4890273e4e9edab4d456cc27fd4064..d4e29a574f1fdbf4d5fc7426a9fc388bb68e4a14 100644 (file)
--- a/line-log.c
+++ b/line-log.c
@@ -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;
@@ -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);
 }
 
 /*
@@ -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);