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/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);
