diff: handle diffstat of rewritten binary files

The logic in builtin_diffstat assumes that a
complete_rewrite pair should have its lines counted. This is
nonsensical for binary files and leads to confusing things
like:

  $ git diff --stat --summary HEAD^ HEAD
   foo.rand |  Bin 4096 -> 4096 bytes
   1 files changed, 0 insertions(+), 0 deletions(-)

  $ git diff --stat --summary -B HEAD^ HEAD
   foo.rand |   34 +++++++++++++++-------------------
   1 files changed, 15 insertions(+), 19 deletions(-)
   rewrite foo.rand (100%)

So let's reorder the function to handle binary files first
(which from diffstat's perspective look like complete
rewrites anyway), then rewrites, then actual diffstats.

There are two bonus prizes to this reorder:

  1. It gets rid of a now-superfluous goto.

  2. The binary case is at the top, which means we can
     further optimize it in the next patch.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/diff.c b/diff.c
index 381cc8d4fd69ca31fb8fc8af31422160e3ec1fd3..14a354147c6c61d369497626ce0bda8e5b4b3060 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -1811,26 +1811,31 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
                data->is_unmerged = 1;
                return;
        }
-       if (complete_rewrite) {
+
+       if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
+               if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+                       die("unable to read files to diff");
+               data->is_binary = 1;
+               data->added = mf2.size;
+               data->deleted = mf1.size;
+       }
+
+       else if (complete_rewrite) {
                diff_populate_filespec(one, 0);
                diff_populate_filespec(two, 0);
                data->deleted = count_lines(one->data, one->size);
                data->added = count_lines(two->data, two->size);
-               goto free_and_return;
        }
-       if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
-               die("unable to read files to diff");
 
-       if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
-               data->is_binary = 1;
-               data->added = mf2.size;
-               data->deleted = mf1.size;
-       } else {
+       else {
                /* Crazy xdl interfaces.. */
                xpparam_t xpp;
                xdemitconf_t xecfg;
                xdemitcb_t ecb;
 
+               if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+                       die("unable to read files to diff");
+
                memset(&xpp, 0, sizeof(xpp));
                memset(&xecfg, 0, sizeof(xecfg));
                xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
@@ -1838,7 +1843,6 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
                              &xpp, &xecfg, &ecb);
        }
 
- free_and_return:
        diff_free_filespec_data(one);
        diff_free_filespec_data(two);
 }

diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
index 7e7b307a24606131b4880817a0056af11973f3d2..7d7470f21b66a937e7414f4fe5419f8830fd8e86 100755 (executable)
--- a/t/t4031-diff-rewrite-binary.sh
+++ b/t/t4031-diff-rewrite-binary.sh
@@ -44,6 +44,13 @@ test_expect_success 'rewrite diff can show binary patch' '
        grep "GIT binary patch" diff
 '
 
+test_expect_success 'rewrite diff --stat shows binary changes' '
+       git diff -B --stat --summary >diff &&
+       grep "Bin" diff &&
+       grep "0 insertions.*0 deletions" diff &&
+       grep " rewrite file" diff
+'
+
 {
        echo "#!$SHELL_PATH"
        cat <<'EOF'