Re: Bug: git log --numstat counts wrong

Junio C Hamano [off-list ref] writes:

Alexander Pepper [off-list ref] writes:

quoted

Am 21.09.2011 um 14:24 schrieb Junio C Hamano:

quoted

quoted

$ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260
[...]
11      10      src/java/voldemort/server/storage/StorageService.java

Didn't we update it this already? I seem to get 10/9 here not 11/10.

Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...

That's a tad old master you seem to have.

Strangely, bisection points at 27af01d5523, which was supposed to be only
about performance and never about correctness. There is something fishy
going on....

In any case, I think the real issue is that depending on how much context
you ask, the resulting diff is different (and both are valid diffs). If
you ask "log -p" (or "diff" or "show") to produce a patch, then we use the
default 3-line context. And then you feed that to an external diffstat to
count the number of deleted and added lines to get one set of numbers.

The --numstat (and --diffstat) code seems to be running the internal diff
machinery with 0-line context and counting the resulting diff internally.

And of course the results between the above two would be different because
diff can match lines differently when given different number of context
lines to include in the result.

So perhaps a good sanity-check for you to try (note: not checking your
sanity, but checking the sanity of the above analysis) would be to do:

  $ git show 48a07e7e53 -- $that_path | diffstat
  $ git show -U0 48a07e7e53 -- $that_path | diffstat
  $ git show --numstat 48a07e7e53 -- $that_path
  $ git show --stat 48a07e7e53 -- $that_path

and see how they compare (make sure to use the same version of git for
these experiments). The first one uses the default 3-lines context, the
second one forces 0-line context, and the last two uses 0-line context
hardwired in the code.

Applying the following patch should make the last two use the default
context or -U$num given from the command line to be consistent with the
codepath where we generate textual patches.

 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 9038f19..302ef33 100644
--- a/diff.c
+++ b/diff.c

@@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 		memset(&xpp, 0, sizeof(xpp));
 		memset(&xecfg, 0, sizeof(xecfg));
 		xpp.flags = o->xdl_opts;
+		xecfg.ctxlen = o->context;
+		xecfg.interhunkctxlen = o->interhunkcontext;
 		xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
 			      &xpp, &xecfg);
 	}

On Fri, Sep 23, 2011 at 1:51 AM, Junio C Hamano [off-list ref] wrote:

Thanks Junio.

But wait, where does this patch go? Before or after 27af01d? If I'm
understanding the situation correctly, this patch won't change the
reporting 10/9 for --numstat, no?

Anyway, this patch looks right.

  Acked-by: Tay Ray Chuan [off-list ref]

Interesting to find that we have many xdiff users that don't respect
diff options on the command line (or may not acess to them), like
patch-id, merge. I wonder if there would be less conflicts if merge's
ctxlen could be overriden...

-- 
Cheers,
Ray Chuan

Am 22.09.2011 um 19:51 schrieb Junio C Hamano:

So perhaps a good sanity-check for you to try (note: not checking your
sanity, but checking the sanity of the above analysis) would be to do:

 $ git show 48a07e7e53 -- $that_path | diffstat
 $ git show -U0 48a07e7e53 -- $that_path | diffstat
 $ git show --numstat 48a07e7e53 -- $that_path
 $ git show --stat 48a07e7e53 -- $that_path

[...]

First of all: thank you for your extended feedback, your respectful e-mails and your patch!

I did some benachmarking. I compared git version 1.7.6.3 with 1.7.7.rc2 and 1.7.7.rc2 with the above patch.

My test setup:
git version 1.7.6.3: 740a8fc2
git version 1.7.7.rc2: 167a5800
git version 1.7.7.rc2': 167a5800 with the above patch applied. 
Tuple (15,07) shows 15 lines added and 7 lines removed

$ git show $rev -- $that_path | diffstat
$ git show -U0 $rev -- $that_path | diffstat
$ git log --numstat -n1 --oneline $rev
$ git show --stat --oneline -n1 $rev -- $that_path

Test 1:
repo='https://github.com/voldemort/voldemort.git'
rev='48a07e7e'
that_path='src/java/voldemort/server/storage/StorageService.java'
Results:
1.7.6.3	1.7.7.rc2	1.7.7.rc2'
(10,09)	(10,09)	(10,09)
(11,10)	(10,09)	(10,09)
(11,10)	(10,09)	(10,09)
(11,10)	(10,09)	(10,09)

Test 2:
repo='https://github.com/voldemort/voldemort.git'
rev='c21ad764'
that_path='contrib/hadoop-store-builder/src/java/voldemort/store/readonly/mr/HadoopStoreBuilderReducer.java'
Results:
1.7.6.3	1.7.7.rc2	1.7.7.rc2'
(30,27)	(25,22)	(25,22)
(25,22)	(25,22)	(25,22)
(25,22)	(25,22)	(25,22)
(25,22)	(25,22)	(25,22)

Test 3:
repo='private repo'
rev='bd61f26e'
that_path='[...]JmeterTest/loadtests/JMeterLoadTest.jmx'
Results:
1.7.6.3	1.7.7.rc2	1.7.7.rc2'
(450,3544)	(450,3544)	(450,3544)
(401,3495)	(401,3495)	(401,3495)
(401,3495)	(401,3495)	(450,3544)
(401,3495)	(401,3495)	(450,3544)

In Test 1 the patch seems to be different formatted from 1.7.7.rc2, so the context doesn't matter with the newer version.
In Test 2 the patch seems to be different formatted from 1.7.7.rc2, so the context doesn't matter with the newer version.
In Test 3 (which is a private repo) where a lot of different lines where changes, some single lines, some multiple lines long makes the biggest difference. With the different contexts different output is observed. I'm sorry that I can not find an open source example for that, but your patch seems to fix this.

So it seems that the patch output in general changed between 1.7.6.3 and 1.7.7.rc2. If I had the right to vote for your patch, I would give it a +1 :-)

Greetings from Berlin
Alex

PS: Will this patch be in the final version of 1.7.7?

On Fri, Sep 23, 2011 at 5:18 PM, Tay Ray Chuan [off-list ref] wrote:

On Fri, Sep 23, 2011 at 1:51 AM, Junio C Hamano [off-list ref] wrote:

quoted

[snip]
Applying the following patch should make the last two use the default
context or -U$num given from the command line to be consistent with the
codepath where we generate textual patches.

 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 9038f19..302ef33 100644
--- a/diff.c
+++ b/diff.c

@@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,

               memset(&xpp, 0, sizeof(xpp));
               memset(&xecfg, 0, sizeof(xecfg));
               xpp.flags = o->xdl_opts;
+               xecfg.ctxlen = o->context;
+               xecfg.interhunkctxlen = o->interhunkcontext;
               xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
                             &xpp, &xecfg);
       }

Thanks Junio.

But wait, where does this patch go? Before or after 27af01d? If I'm
understanding the situation correctly, this patch won't change the
reporting 10/9 for --numstat, no?

I think I can answer this - on to v1.7.6, which is before 27af01d was merged in.

Anyway, this patch looks right.

On further thought, I think the patch merely side-steps the problem -
ie. that -U0 generates "incorrect" diffs.

Further digging reveals a xdiff-interface.c::trim_common_tail();
commenting its one and only call (patch below) gives back 10/9. Note
that it only has effect when -U0.

I think this function is incorrect. xdl_cleanup_records() and
xdl_clean_mmatch() may potentially look into common tail lines, so it
may not be "safe" to drop all common tail lines.

-- >8 --

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 0e2c169..da4fab6 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c

@@ -131,7 +131,7 @@
        mmfile_t a = *mf1;
        mmfile_t b = *mf2;

-       trim_common_tail(&a, &b, xecfg->ctxlen);
+/*     trim_common_tail(&a, &b, xecfg->ctxlen);  */

        return xdl_diff(&a, &b, xpp, xecfg, xecb);
 }
-- >8 --

-- 
Cheers,
Ray Chuan