{"thread":{"id":"28456","subject":"Bug: git log --numstat counts wrong","startedAt":"2011-09-21T09:03:30Z","lastAt":"2011-09-25T17:53:35Z","messageCount":16,"participants":["Alexander Pepper","Junio C Hamano","René Scharfe","Tay Ray Chuan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"175930","messageId":"D3CF0A47-64DA-4EBB-9DCD-D2D714596C50@inf.fu-berlin.de","threadId":"28456","inReplyTo":null,"subject":"Bug: git log --numstat counts wrong","fromName":"Alexander Pepper","fromEmail":"pepper@inf.fu-berlin.de","sentAt":"2011-09-21T09:03:30Z","receivedAt":"2011-09-21T09:03:30Z","isPatch":false,"sender":{"key":"pepper@inf.fu-berlin.de","avatar":null},"body":"Hello there.\n\nI already reported some similar bug with git log --numstat to this mailinglist (see http://www.spinics.net/lists/git/msg163358.html ). Back then empty lines seems to be the issue, but the bug was never fixed.\n\nI found another case, where git log --numstat counts wrong. This time git log --numstat yields bigger numbers than diffstat.\n\nMinimal example:\n$ git clone https://github.com/voldemort/voldemort.git\n$ cd voldemort/\n$ git show 48a07e7e533f507228e8d1c99d4d48e175e14260 -- src/java/voldemort/server/storage/StorageService.java | diffstat\n StorageService.java |   19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n$ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n[...]\n11      10      src/java/voldemort/server/storage/StorageService.java\n\n\nSo git log --numstat claimes that 11 lines where added, where diffstat only counts 10! A closer look inside the StorageService.java reveals no empty lines.\n\nMy system:\n* Mac osx 10.6.8\n* git 1.7.5.4 (but also check with a self compiled 1.7.6.1)\n\nCan you confirm that this is a bug? If so, are there plans to fix it in the future?\n\nGreetings from Berlin\nAlex"},{"id":"175939","messageId":"7vr53a2icn.fsf@alter.siamese.dyndns.org","threadId":"28456","inReplyTo":"D3CF0A47-64DA-4EBB-9DCD-D2D714596C50@inf.fu-berlin.de","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-21T12:24:56Z","receivedAt":"2011-09-21T12:24:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n\n> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n> [...]\n> 11      10      src/java/voldemort/server/storage/StorageService.java\n\nDidn't we update it this already? I seem to get 10/9 here not 11/10.\n"},{"id":"175946","messageId":"C03FC526-B7D0-4EFA-9E35-68F28D950C4B@inf.fu-berlin.de","threadId":"28456","inReplyTo":"7vr53a2icn.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Alexander Pepper","fromEmail":"pepper@inf.fu-berlin.de","sentAt":"2011-09-21T13:40:41Z","receivedAt":"2011-09-21T13:40:41Z","isPatch":false,"sender":{"key":"pepper@inf.fu-berlin.de","avatar":null},"body":"Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>> [...]\n>> 11      10      src/java/voldemort/server/storage/StorageService.java\n> \n> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n\nI just compiled git fresh from the master (4b5eac7f) and the issue is still active there:\n\n$ ../git/git --version\ngit version 1.7.7.rc0.72.g4b5ea\n$ ../git/git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n[...]\n11      10      src/java/voldemort/server/storage/StorageService.java\n\nShould I check another branch?"},{"id":"175948","messageId":"3BF8BA51-4CAA-40A2-8B45-D39AAEE58E6F@inf.fu-berlin.de","threadId":"28456","inReplyTo":"7vr53a2icn.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Alexander Pepper","fromEmail":"pepper@inf.fu-berlin.de","sentAt":"2011-09-21T14:23:58Z","receivedAt":"2011-09-21T14:23:58Z","isPatch":false,"sender":{"key":"pepper@inf.fu-berlin.de","avatar":null},"body":"Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>> [...]\n>> 11      10      src/java/voldemort/server/storage/StorageService.java\n> \n> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n\nBesides master I tried some other branches. Current 'maint' (cd2b8ae9), 'master' (4b5eac7f) and v1.7.7-rc0 (a452d148) are still counting wrong, but 'next' (3be2039f) and 'pu' (b4bcbace) do count correctly.\n\nI'll have a closer look with the 'next' branch if all counts are correct now."},{"id":"175963","messageId":"7vobyd1vmo.fsf@alter.siamese.dyndns.org","threadId":"28456","inReplyTo":"3BF8BA51-4CAA-40A2-8B45-D39AAEE58E6F@inf.fu-berlin.de","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-21T20:35:43Z","receivedAt":"2011-09-21T20:35:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n\n> Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>>> [...]\n>>> 11      10      src/java/voldemort/server/storage/StorageService.java\n>> \n>> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n>\n> Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...\n\nThat's a tad old master you seem to have.\n\nStrangely, bisection points at 27af01d5523, which was supposed to be only\nabout performance and never about correctness. There is something fishy\ngoing on....\n"},{"id":"175994","messageId":"FAB0B05E-6BAD-488C-8478-F4B80493FB96@inf.fu-berlin.de","threadId":"28456","inReplyTo":"7vobyd1vmo.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Alexander Pepper","fromEmail":"pepper@inf.fu-berlin.de","sentAt":"2011-09-22T13:19:39Z","receivedAt":"2011-09-22T13:19:39Z","isPatch":false,"sender":{"key":"pepper@inf.fu-berlin.de","avatar":null},"body":"Am 21.09.2011 um 22:35 schrieb Junio C Hamano:\n> That's a tad old master you seem to have.\nUp until now I only followed the github clone, but that seems to be dated. Since kernel.org is still down, I'll now follow the google code clone.\n\n\nAm 21.09.2011 um 22:35 schrieb Junio C Hamano:\n>> Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>>>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>>>> [...]\n>>>> 11      10      src/java/voldemort/server/storage/StorageService.java\n>>> \n>>> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n>> \n>> Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...\n> \n> Strangely, bisection points at 27af01d5523, which was supposed to be only\n> about performance and never about correctness. There is something fishy\n> going on....\nI also did some tests, and besides --numstat also git show sometimes show different patches in comparison to older git versions. The last version with the \"old\" git show output is 1.7.7.rc0 and the first version with the \"new\" git show output is 1.7.7.rc1.\n\nwith git version 1.7.7.rc0:\n$ git show 679e1d5cd007a0a9cb2813bd155622d7a1e904bd :\n[...]\ndiff --git a/src/java/voldemort/store/stats/RequestCounter.java b/src/java/voldemort/store/stats/RequestCounter.java\nindex b012e98..c6be603 100644\n--- a/src/java/voldemort/store/stats/RequestCounter.java\n+++ b/src/java/voldemort/store/stats/RequestCounter.java\n@@ -64,16 +64,21 @@ public class RequestCounter {\n         Accumulator accum = values.get();\n         long now = System.currentTimeMillis();\n \n-        if(now - accum.startTimeMS > durationMS) {\n-            Accumulator newWithTotal = accum.newWithTotal();\n-            values.set(newWithTotal);\n-\n-            /*\n-             * try to set.  if we fail, then someone else set it, so just keep going\n-             */\n-            if(values.compareAndSet(accum, newWithTotal)) {\n-                return newWithTotal;\n-            }\n+        /*\n+         *  if still in the window, just return it\n+         */\n+        if(now - accum.startTimeMS <= durationMS) {\n+            return accum;\n+        }\n+\n+        /*\n+         * try to set.  if we fail, then someone else set it, so just return that new one\n+         */\n+\n+        Accumulator newWithTotal = accum.newWithTotal();\n+\n+        if(values.compareAndSet(accum, newWithTotal)) {\n+            return newWithTotal;\n         }\n \n         return values.get();\n\n\nwith git version 1.7.7.rc1:\n$ git show 679e1d5cd007a0a9cb2813bd155622d7a1e904bd\n[...]\ndiff --git a/src/java/voldemort/store/stats/RequestCounter.java b/src/java/voldemort/store/stats/RequestCounter.java\nindex b012e98..c6be603 100644\n--- a/src/java/voldemort/store/stats/RequestCounter.java\n+++ b/src/java/voldemort/store/stats/RequestCounter.java\n@@ -64,16 +64,21 @@ public class RequestCounter {\n         Accumulator accum = values.get();\n         long now = System.currentTimeMillis();\n \n-        if(now - accum.startTimeMS > durationMS) {\n-            Accumulator newWithTotal = accum.newWithTotal();\n-            values.set(newWithTotal);\n+        /*\n+         *  if still in the window, just return it\n+         */\n+        if(now - accum.startTimeMS <= durationMS) {\n+            return accum;\n+        }\n \n-            /*\n-             * try to set.  if we fail, then someone else set it, so just keep going\n-             */\n-            if(values.compareAndSet(accum, newWithTotal)) {\n-                return newWithTotal;\n-            }\n+        /*\n+         * try to set.  if we fail, then someone else set it, so just return that new one\n+         */\n+\n+        Accumulator newWithTotal = accum.newWithTotal();\n+\n+        if(values.compareAndSet(accum, newWithTotal)) {\n+            return newWithTotal;\n         }\n \n         return values.get();\n\n\nThe difference is, that now it's shown as two delete and two added hunks instead of one bigger delete and one bigger added hunk.\n\nwith git version 1.7.7.rc1:\n$ git show 679e1d5cd007a0a9cb2813bd155622d7a1e904bd | diffstat\n RequestCounter.java |   23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\nwith git version 1.7.7.rc0:\n$ git show 679e1d5cd007a0a9cb2813bd155622d7a1e904bd | diffstat\n RequestCounter.java |   25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\nWhen used git version 1.7.7.rc1 I didn't observed any case where git show and git log --numstat mismatch. I'm only a little confused, that 'git show' yields different results, depending on the git version.\n\nGreetings from Berlin\nAlex"},{"id":"175997","messageId":"4E7B5F28.2020204@lsrfire.ath.cx","threadId":"28456","inReplyTo":"7vobyd1vmo.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-09-22T16:15:36Z","receivedAt":"2011-09-22T16:15:36Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.09.2011 22:35, schrieb Junio C Hamano:\n> Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n> \n>> Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>>>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>>>> [...]\n>>>> 11      10      src/java/voldemort/server/storage/StorageService.java\n>>>\n>>> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n>>\n>> Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...\n> \n> That's a tad old master you seem to have.\n> \n> Strangely, bisection points at 27af01d5523, which was supposed to be only\n> about performance and never about correctness. There is something fishy\n> going on....\n\nThe patch below reverts a part of 27af01d5523 that's not explained in its\ncommit message and doesn't seem to contribute to the intended speedup.  It\nseems to restore the original diff output.  I don't know how it's actually\ndoing that, though, as I haven't dug into the code at all.\n\nAlexander, can you confirm that this patch restores the old behaviour of\ngit diff and git show for your test cases?\n\nRay, are you able to write a commit message for this patch if it turns out\nto be useful?\n\nRené\n\n\ndiff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\nindex 5a33d1a..e419f4f 100644\n--- a/xdiff/xprepare.c\n+++ b/xdiff/xprepare.c\n@@ -383,7 +383,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {\n  * might be potentially discarded if they happear in a run of discardable.\n  */\n static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {\n-\tlong i, nm, nreff;\n+\tlong i, nm, nreff, mlim;\n \txrecord_t **recs;\n \txdlclass_t *rcrec;\n \tchar *dis, *dis1, *dis2;\n@@ -396,16 +396,20 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd\n \tdis1 = dis;\n \tdis2 = dis1 + xdf1->nrec + 1;\n \n+\tif ((mlim = xdl_bogosqrt(xdf1->nrec)) > XDL_MAX_EQLIMIT)\n+\t\tmlim = XDL_MAX_EQLIMIT;\n \tfor (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {\n \t\trcrec = cf->rcrecs[(*recs)->ha];\n \t\tnm = rcrec ? rcrec->len2 : 0;\n-\t\tdis1[i] = (nm == 0) ? 0: 1;\n+\t\tdis1[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n \t}\n \n+\tif ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)\n+\t\tmlim = XDL_MAX_EQLIMIT;\n \tfor (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {\n \t\trcrec = cf->rcrecs[(*recs)->ha];\n \t\tnm = rcrec ? rcrec->len1 : 0;\n-\t\tdis2[i] = (nm == 0) ? 0: 1;\n+\t\tdis2[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n \t}\n \n \tfor (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];\n"},{"id":"176003","messageId":"7vsjnoxz2n.fsf@alter.siamese.dyndns.org","threadId":"28456","inReplyTo":"FAB0B05E-6BAD-488C-8478-F4B80493FB96@inf.fu-berlin.de","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-22T17:32:32Z","receivedAt":"2011-09-22T17:32:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n\n> When used git version 1.7.7.rc1 I didn't observed any case where git\n> show and git log --numstat mismatch. I'm only a little confused, that\n> 'git show' yields different results, depending on the git version.\n\nIn general it is not surprising nor unexpected--as long as both patches\ndescribe the change correctly, they are both valid.\n\nWhat was unexpected to me was that 27af01d (xdiff/xprepare: improve O(n*m)\nperformance in xdl_cleanup_records(), 2011-08-17) which was supposed to be\nonly about performance and not about logic made that difference.\n"},{"id":"176007","messageId":"7vobycxy71.fsf@alter.siamese.dyndns.org","threadId":"28456","inReplyTo":"7vobyd1vmo.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-22T17:51:30Z","receivedAt":"2011-09-22T17:51:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n>\n>> Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>>>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>>>> [...]\n>>>> 11      10      src/java/voldemort/server/storage/StorageService.java\n>>> \n>>> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n>>\n>> Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...\n>\n> That's a tad old master you seem to have.\n>\n> Strangely, bisection points at 27af01d5523, which was supposed to be only\n> about performance and never about correctness. There is something fishy\n> going on....\n\nIn any case, I think the real issue is that depending on how much context\nyou ask, the resulting diff is different (and both are valid diffs). If\nyou ask \"log -p\" (or \"diff\" or \"show\") to produce a patch, then we use the\ndefault 3-line context. And then you feed that to an external diffstat to\ncount the number of deleted and added lines to get one set of numbers.\n\nThe --numstat (and --diffstat) code seems to be running the internal diff\nmachinery with 0-line context and counting the resulting diff internally.\n\nAnd of course the results between the above two would be different because\ndiff can match lines differently when given different number of context\nlines to include in the result.\n\nSo perhaps a good sanity-check for you to try (note: not checking your\nsanity, but checking the sanity of the above analysis) would be to do:\n\n  $ git show 48a07e7e53 -- $that_path | diffstat\n  $ git show -U0 48a07e7e53 -- $that_path | diffstat\n  $ git show --numstat 48a07e7e53 -- $that_path\n  $ git show --stat 48a07e7e53 -- $that_path\n\nand see how they compare (make sure to use the same version of git for\nthese experiments). The first one uses the default 3-lines context, the\nsecond one forces 0-line context, and the last two uses 0-line context\nhardwired in the code.\n\nApplying the following patch should make the last two use the default\ncontext or -U$num given from the command line to be consistent with the\ncodepath where we generate textual patches.\n\n diff.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9038f19..302ef33 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t\tmemset(&xpp, 0, sizeof(xpp));\n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n \t\txpp.flags = o->xdl_opts;\n+\t\txecfg.ctxlen = o->context;\n+\t\txecfg.interhunkctxlen = o->interhunkcontext;\n \t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n \t\t\t      &xpp, &xecfg);\n \t}\n"},{"id":"176044","messageId":"CALUzUxprUFGMR-WVEMOOvYiwkev1cfxHOyBmZq9bKJcHq5E2VA@mail.gmail.com","threadId":"28456","inReplyTo":"4E7B5F28.2020204@lsrfire.ath.cx","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-09-23T06:30:40Z","receivedAt":"2011-09-23T06:30:40Z","isPatch":false,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Fri, Sep 23, 2011 at 12:15 AM, René Scharfe\n<rene.scharfe@lsrfire.ath.cx> wrote:\n> The patch below reverts a part of 27af01d5523 that's not explained in its\n> commit message and doesn't seem to contribute to the intended speedup.  It\n> seems to restore the original diff output.  I don't know how it's actually\n> doing that, though, as I haven't dug into the code at all.\n>\n> [snip]\n>\n> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\n> index 5a33d1a..e419f4f 100644\n> --- a/xdiff/xprepare.c\n> +++ b/xdiff/xprepare.c\n> @@ -383,7 +383,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {\n>  * might be potentially discarded if they happear in a run of discardable.\n>  */\n>  static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {\n> -       long i, nm, nreff;\n> +       long i, nm, nreff, mlim;\n>        xrecord_t **recs;\n>        xdlclass_t *rcrec;\n>        char *dis, *dis1, *dis2;\n> @@ -396,16 +396,20 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd\n>        dis1 = dis;\n>        dis2 = dis1 + xdf1->nrec + 1;\n>\n> +       if ((mlim = xdl_bogosqrt(xdf1->nrec)) > XDL_MAX_EQLIMIT)\n> +               mlim = XDL_MAX_EQLIMIT;\n>        for (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {\n>                rcrec = cf->rcrecs[(*recs)->ha];\n>                nm = rcrec ? rcrec->len2 : 0;\n> -               dis1[i] = (nm == 0) ? 0: 1;\n> +               dis1[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n>        }\n>\n> +       if ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)\n> +               mlim = XDL_MAX_EQLIMIT;\n>        for (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {\n>                rcrec = cf->rcrecs[(*recs)->ha];\n>                nm = rcrec ? rcrec->len1 : 0;\n> -               dis2[i] = (nm == 0) ? 0: 1;\n> +               dis2[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n>        }\n>\n>        for (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];\n>\n>\n>\n\nThanks for the patch, René.\n\nSorry for not explaining that part of the change.\n\nMy understanding of mlim is that it \"caps\" how deep the for loop at\naround line 387 goes through a hash bucket/record chaing to find a\nmatching record from side A in side B (and vice-versa in a later\nloop), probably to prevent running time from becoming too long.\n\nBut with 27af01d, this is no longer a concern. We can get an *exact*,\npre-computed count of matching records in the other side, so we don't\nhave go through the hash bucket. Thus mlim is no longer needed.\n\nSo re-introducing mlim doesn't seem right, even though it may fix this\n\"bug\" (ie restore the old behaviour).\n\n-- \nCheers,\nRay Chuan\n"},{"id":"176050","messageId":"CALUzUxrswZ+AREq+OeqpTsnoB4J+_aExfmAA6X3cauJqj8RnpQ@mail.gmail.com","threadId":"28456","inReplyTo":"7vobycxy71.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-09-23T09:18:15Z","receivedAt":"2011-09-23T09:18:15Z","isPatch":false,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Fri, Sep 23, 2011 at 1:51 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Alexander Pepper <pepper@inf.fu-berlin.de> writes:\n>>\n>>> Am 21.09.2011 um 14:24 schrieb Junio C Hamano:\n>>>>> $ git log --numstat 48a07e7e533f507228e8d1c99d4d48e175e14260\n>>>>> [...]\n>>>>> 11      10      src/java/voldemort/server/storage/StorageService.java\n>>>>\n>>>> Didn't we update it this already? I seem to get 10/9 here not 11/10.\n>>>\n>>> Current 'maint' (cd2b8ae9), 'master' (4b5eac7f)...\n>>\n>> That's a tad old master you seem to have.\n>>\n>> Strangely, bisection points at 27af01d5523, which was supposed to be only\n>> about performance and never about correctness. There is something fishy\n>> going on....\n>\n> In any case, I think the real issue is that depending on how much context\n> you ask, the resulting diff is different (and both are valid diffs). If\n> you ask \"log -p\" (or \"diff\" or \"show\") to produce a patch, then we use the\n> default 3-line context. And then you feed that to an external diffstat to\n> count the number of deleted and added lines to get one set of numbers.\n>\n> The --numstat (and --diffstat) code seems to be running the internal diff\n> machinery with 0-line context and counting the resulting diff internally.\n>\n> And of course the results between the above two would be different because\n> diff can match lines differently when given different number of context\n> lines to include in the result.\n>\n> So perhaps a good sanity-check for you to try (note: not checking your\n> sanity, but checking the sanity of the above analysis) would be to do:\n>\n>  $ git show 48a07e7e53 -- $that_path | diffstat\n>  $ git show -U0 48a07e7e53 -- $that_path | diffstat\n>  $ git show --numstat 48a07e7e53 -- $that_path\n>  $ git show --stat 48a07e7e53 -- $that_path\n>\n> and see how they compare (make sure to use the same version of git for\n> these experiments). The first one uses the default 3-lines context, the\n> second one forces 0-line context, and the last two uses 0-line context\n> hardwired in the code.\n>\n> Applying the following patch should make the last two use the default\n> context or -U$num given from the command line to be consistent with the\n> codepath where we generate textual patches.\n>\n>  diff.c |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 9038f19..302ef33 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n>                memset(&xpp, 0, sizeof(xpp));\n>                memset(&xecfg, 0, sizeof(xecfg));\n>                xpp.flags = o->xdl_opts;\n> +               xecfg.ctxlen = o->context;\n> +               xecfg.interhunkctxlen = o->interhunkcontext;\n>                xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n>                              &xpp, &xecfg);\n>        }\n\nThanks Junio.\n\nBut wait, where does this patch go? Before or after 27af01d? If I'm\nunderstanding the situation correctly, this patch won't change the\nreporting 10/9 for --numstat, no?\n\nAnyway, this patch looks right.\n\n  Acked-by: Tay Ray Chuan <rctay89@gmail.com>\n\nInteresting to find that we have many xdiff users that don't respect\ndiff options on the command line (or may not acess to them), like\npatch-id, merge. I wonder if there would be less conflicts if merge's\nctxlen could be overriden...\n\n-- \nCheers,\nRay Chuan\n"},{"id":"176056","messageId":"8AEDF5F8-19B5-4502-BB53-EC6CEE0E5CB2@inf.fu-berlin.de","threadId":"28456","inReplyTo":"7vobycxy71.fsf@alter.siamese.dyndns.org","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Alexander Pepper","fromEmail":"pepper@inf.fu-berlin.de","sentAt":"2011-09-23T10:30:13Z","receivedAt":"2011-09-23T10:30:13Z","isPatch":false,"sender":{"key":"pepper@inf.fu-berlin.de","avatar":null},"body":"Am 22.09.2011 um 19:51 schrieb Junio C Hamano:\n> So perhaps a good sanity-check for you to try (note: not checking your\n> sanity, but checking the sanity of the above analysis) would be to do:\n> \n>  $ git show 48a07e7e53 -- $that_path | diffstat\n>  $ git show -U0 48a07e7e53 -- $that_path | diffstat\n>  $ git show --numstat 48a07e7e53 -- $that_path\n>  $ git show --stat 48a07e7e53 -- $that_path\n[...]\n> Applying the following patch should make the last two use the default\n> context or -U$num given from the command line to be consistent with the\n> codepath where we generate textual patches.\n> \n> diff.c |    2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 9038f19..302ef33 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n> \t\tmemset(&xpp, 0, sizeof(xpp));\n> \t\tmemset(&xecfg, 0, sizeof(xecfg));\n> \t\txpp.flags = o->xdl_opts;\n> +\t\txecfg.ctxlen = o->context;\n> +\t\txecfg.interhunkctxlen = o->interhunkcontext;\n> \t\txdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n> \t\t\t      &xpp, &xecfg);\n> \t}\n\nFirst of all: thank you for your extended feedback, your respectful e-mails and your patch!\n\nI 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.\n\nMy test setup:\ngit version 1.7.6.3: 740a8fc2\ngit version 1.7.7.rc2: 167a5800\ngit version 1.7.7.rc2': 167a5800 with the above patch applied. \nTuple (15,07) shows 15 lines added and 7 lines removed\n\n$ git show $rev -- $that_path | diffstat\n$ git show -U0 $rev -- $that_path | diffstat\n$ git log --numstat -n1 --oneline $rev\n$ git show --stat --oneline -n1 $rev -- $that_path\n\nTest 1:\nrepo='https://github.com/voldemort/voldemort.git'\nrev='48a07e7e'\nthat_path='src/java/voldemort/server/storage/StorageService.java'\nResults:\n1.7.6.3\t1.7.7.rc2\t1.7.7.rc2'\n(10,09)\t(10,09)\t(10,09)\n(11,10)\t(10,09)\t(10,09)\n(11,10)\t(10,09)\t(10,09)\n(11,10)\t(10,09)\t(10,09)\n\nTest 2:\nrepo='https://github.com/voldemort/voldemort.git'\nrev='c21ad764'\nthat_path='contrib/hadoop-store-builder/src/java/voldemort/store/readonly/mr/HadoopStoreBuilderReducer.java'\nResults:\n1.7.6.3\t1.7.7.rc2\t1.7.7.rc2'\n(30,27)\t(25,22)\t(25,22)\n(25,22)\t(25,22)\t(25,22)\n(25,22)\t(25,22)\t(25,22)\n(25,22)\t(25,22)\t(25,22)\n\nTest 3:\nrepo='private repo'\nrev='bd61f26e'\nthat_path='[...]JmeterTest/loadtests/JMeterLoadTest.jmx'\nResults:\n1.7.6.3\t1.7.7.rc2\t1.7.7.rc2'\n(450,3544)\t(450,3544)\t(450,3544)\n(401,3495)\t(401,3495)\t(401,3495)\n(401,3495)\t(401,3495)\t(450,3544)\n(401,3495)\t(401,3495)\t(450,3544)\n\nIn Test 1 the patch seems to be different formatted from 1.7.7.rc2, so the context doesn't matter with the newer version.\nIn Test 2 the patch seems to be different formatted from 1.7.7.rc2, so the context doesn't matter with the newer version.\nIn 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.\n\nSo 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 :-)\n\nGreetings from Berlin\nAlex\n\nPS: Will this patch be in the final version of 1.7.7?"},{"id":"176074","messageId":"CALUzUxoujys1eWL6i6YJmFZZakcQx8oa8ZbRjixUzANB1Hpb3Q@mail.gmail.com","threadId":"28456","inReplyTo":"CALUzUxrswZ+AREq+OeqpTsnoB4J+_aExfmAA6X3cauJqj8RnpQ@mail.gmail.com","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-09-23T16:38:19Z","receivedAt":"2011-09-23T16:38:19Z","isPatch":false,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Fri, Sep 23, 2011 at 5:18 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:\n> On Fri, Sep 23, 2011 at 1:51 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> [snip]\n>> Applying the following patch should make the last two use the default\n>> context or -U$num given from the command line to be consistent with the\n>> codepath where we generate textual patches.\n>>\n>>  diff.c |    2 ++\n>>  1 files changed, 2 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 9038f19..302ef33 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -2251,6 +2251,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n>>                memset(&xpp, 0, sizeof(xpp));\n>>                memset(&xecfg, 0, sizeof(xecfg));\n>>                xpp.flags = o->xdl_opts;\n>> +               xecfg.ctxlen = o->context;\n>> +               xecfg.interhunkctxlen = o->interhunkcontext;\n>>                xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,\n>>                              &xpp, &xecfg);\n>>        }\n>\n> Thanks Junio.\n>\n> But wait, where does this patch go? Before or after 27af01d? If I'm\n> understanding the situation correctly, this patch won't change the\n> reporting 10/9 for --numstat, no?\n\nI think I can answer this - on to v1.7.6, which is before 27af01d was merged in.\n\n> Anyway, this patch looks right.\n\nOn further thought, I think the patch merely side-steps the problem -\nie. that -U0 generates \"incorrect\" diffs.\n\nFurther digging reveals a xdiff-interface.c::trim_common_tail();\ncommenting its one and only call (patch below) gives back 10/9. Note\nthat it only has effect when -U0.\n\nI think this function is incorrect. xdl_cleanup_records() and\nxdl_clean_mmatch() may potentially look into common tail lines, so it\nmay not be \"safe\" to drop all common tail lines.\n\n-- >8 --\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 0e2c169..da4fab6 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -131,7 +131,7 @@\n        mmfile_t a = *mf1;\n        mmfile_t b = *mf2;\n\n-       trim_common_tail(&a, &b, xecfg->ctxlen);\n+/*     trim_common_tail(&a, &b, xecfg->ctxlen);  */\n\n        return xdl_diff(&a, &b, xpp, xecfg, xecb);\n }\n-- >8 --\n\n-- \nCheers,\nRay Chuan\n"},{"id":"176090","messageId":"7vwrczt64j.fsf@alter.siamese.dyndns.org","threadId":"28456","inReplyTo":"CALUzUxoujys1eWL6i6YJmFZZakcQx8oa8ZbRjixUzANB1Hpb3Q@mail.gmail.com","subject":"Re: Bug: git log --numstat counts wrong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-23T19:23:40Z","receivedAt":"2011-09-23T19:23:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n> On further thought, I think the patch merely side-steps the problem -\n> ie. that -U0 generates \"incorrect\" diffs.\n\nI do not know if there anything \"incorrect\" about it.\n\nWhen the file have common blocks lines in different places that are not\nmodified, the comparison between preimage and postimage is free to choose\nhow these common blocks are matched, and for that reason it is incorrect\nto expect that \"diff -U0\", \"diff -U3\" and \"diff -U20\" would produce the\nidentical results.\n\n> I think this function is incorrect. xdl_cleanup_records() and\n> xdl_clean_mmatch() may potentially look into common tail lines, so it\n> may not be \"safe\" to drop all common tail lines.\n\nI would prefer to keep that common trimming optimization, and also to see\nthe same common trimming logic extended to trim (and adjust offsets) at\nthe beginning as well in the longer term.  If xdl_cleanup_records() and\nxdl_clean_mmatch() need to become aware of the change in the total number\nof lines made by trim_common_tail(), please make it so.\n\n>\n> -- >8 --\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 0e2c169..da4fab6 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -131,7 +131,7 @@\n>         mmfile_t a = *mf1;\n>         mmfile_t b = *mf2;\n>\n> -       trim_common_tail(&a, &b, xecfg->ctxlen);\n> +/*     trim_common_tail(&a, &b, xecfg->ctxlen);  */\n>\n>         return xdl_diff(&a, &b, xpp, xecfg, xecb);\n>  }\n> -- >8 --\n"},{"id":"176168","messageId":"1316957948-1908-1-git-send-email-rctay89@gmail.com","threadId":"28456","inReplyTo":"4E7B5F28.2020204@lsrfire.ath.cx","subject":"[PATCH] Revert removal of multi-match discard heuristic in 27af01","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-09-25T13:39:08Z","receivedAt":"2011-09-25T13:39:08Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"From: René Scharfe <rene.scharfe@lsrfire.ath.cx>\n\n27af01d (xdiff/xprepare: improve O(n*m) performance in\nxdl_cleanup_records(), 2011-08-17) was supposed to be a performance\nboost only. However, it unexpectedly changed the behaviour of diff.\n\nRevert a part of 27af01d that removes logic that mark lines as\n\"multi-match\" (ie. dis[i] == 2). This was preventing the multi-match\ndiscard heuristic (performed in xdl_cleanup_records() and\nxdl_clean_mmatch()) from executing.\n\nReported-by: Alexander Pepper <pepper@inf.fu-berlin.de>\nSigned-off-by: Tay Ray Chuan <rctay89@gmail.com>\n\n---\n\nJunio, this replaces the patch the one in the\n'rs/diff-cleanup-records-fix' topic in 'pu'. The only difference is in\nthe patch message.\n\nRené, will need your SOB on this. Thanks for working to produce the\npatch. Please disregard my earlier message [1], further reading has\nshown my previous understanding to be wrong.\n\n[1] <CALUzUxprUFGMR-WVEMOOvYiwkev1cfxHOyBmZq9bKJcHq5E2VA@mail.gmail.com>\n---\n xdiff/xprepare.c |   10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\nindex 05a8f01..4c447ca 100644\n--- a/xdiff/xprepare.c\n+++ b/xdiff/xprepare.c\n@@ -398,7 +398,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {\n  * might be potentially discarded if they happear in a run of discardable.\n  */\n static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {\n-\tlong i, nm, nreff;\n+\tlong i, nm, nreff, mlim;\n \txrecord_t **recs;\n \txdlclass_t *rcrec;\n \tchar *dis, *dis1, *dis2;\n@@ -411,16 +411,20 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd\n \tdis1 = dis;\n \tdis2 = dis1 + xdf1->nrec + 1;\n \n+\tif ((mlim = xdl_bogosqrt(xdf1->nrec)) > XDL_MAX_EQLIMIT)\n+\t\tmlim = XDL_MAX_EQLIMIT;\n \tfor (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {\n \t\trcrec = cf->rcrecs[(*recs)->ha];\n \t\tnm = rcrec ? rcrec->len2 : 0;\n-\t\tdis1[i] = (nm == 0) ? 0: 1;\n+\t\tdis1[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n \t}\n \n+\tif ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)\n+\t\tmlim = XDL_MAX_EQLIMIT;\n \tfor (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {\n \t\trcrec = cf->rcrecs[(*recs)->ha];\n \t\tnm = rcrec ? rcrec->len1 : 0;\n-\t\tdis2[i] = (nm == 0) ? 0: 1;\n+\t\tdis2[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n \t}\n \n \tfor (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];\n-- \n1.7.7.rc3.432.g6bcf0\n"},{"id":"176176","messageId":"4E7F6A9F.6050501@lsrfire.ath.cx","threadId":"28456","inReplyTo":"1316957948-1908-1-git-send-email-rctay89@gmail.com","subject":"Re: [PATCH] Revert removal of multi-match discard heuristic in 27af01","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-09-25T17:53:35Z","receivedAt":"2011-09-25T17:53:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.09.2011 15:39, schrieb Tay Ray Chuan:\n> From: René Scharfe <rene.scharfe@lsrfire.ath.cx>\n> \n> 27af01d (xdiff/xprepare: improve O(n*m) performance in\n> xdl_cleanup_records(), 2011-08-17) was supposed to be a performance\n> boost only. However, it unexpectedly changed the behaviour of diff.\n> \n> Revert a part of 27af01d that removes logic that mark lines as\n> \"multi-match\" (ie. dis[i] == 2). This was preventing the multi-match\n> discard heuristic (performed in xdl_cleanup_records() and\n> xdl_clean_mmatch()) from executing.\n> \n> Reported-by: Alexander Pepper <pepper@inf.fu-berlin.de>\n> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>\n> \n> ---\n> \n> Junio, this replaces the patch the one in the\n> 'rs/diff-cleanup-records-fix' topic in 'pu'. The only difference is in\n> the patch message.\n> \n> René, will need your SOB on this. Thanks for working to produce the\n> patch. Please disregard my earlier message [1], further reading has\n> shown my previous understanding to be wrong.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n\n> \n> [1] <CALUzUxprUFGMR-WVEMOOvYiwkev1cfxHOyBmZq9bKJcHq5E2VA@mail.gmail.com>\n> ---\n>  xdiff/xprepare.c |   10 +++++++---\n>  1 files changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c\n> index 05a8f01..4c447ca 100644\n> --- a/xdiff/xprepare.c\n> +++ b/xdiff/xprepare.c\n> @@ -398,7 +398,7 @@ static int xdl_clean_mmatch(char const *dis, long i, long s, long e) {\n>   * might be potentially discarded if they happear in a run of discardable.\n>   */\n>  static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xdf2) {\n> -\tlong i, nm, nreff;\n> +\tlong i, nm, nreff, mlim;\n>  \txrecord_t **recs;\n>  \txdlclass_t *rcrec;\n>  \tchar *dis, *dis1, *dis2;\n> @@ -411,16 +411,20 @@ static int xdl_cleanup_records(xdlclassifier_t *cf, xdfile_t *xdf1, xdfile_t *xd\n>  \tdis1 = dis;\n>  \tdis2 = dis1 + xdf1->nrec + 1;\n>  \n> +\tif ((mlim = xdl_bogosqrt(xdf1->nrec)) > XDL_MAX_EQLIMIT)\n> +\t\tmlim = XDL_MAX_EQLIMIT;\n>  \tfor (i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart]; i <= xdf1->dend; i++, recs++) {\n>  \t\trcrec = cf->rcrecs[(*recs)->ha];\n>  \t\tnm = rcrec ? rcrec->len2 : 0;\n> -\t\tdis1[i] = (nm == 0) ? 0: 1;\n> +\t\tdis1[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n>  \t}\n>  \n> +\tif ((mlim = xdl_bogosqrt(xdf2->nrec)) > XDL_MAX_EQLIMIT)\n> +\t\tmlim = XDL_MAX_EQLIMIT;\n>  \tfor (i = xdf2->dstart, recs = &xdf2->recs[xdf2->dstart]; i <= xdf2->dend; i++, recs++) {\n>  \t\trcrec = cf->rcrecs[(*recs)->ha];\n>  \t\tnm = rcrec ? rcrec->len1 : 0;\n> -\t\tdis2[i] = (nm == 0) ? 0: 1;\n> +\t\tdis2[i] = (nm == 0) ? 0: (nm >= mlim) ? 2: 1;\n>  \t}\n>  \n>  \tfor (nreff = 0, i = xdf1->dstart, recs = &xdf1->recs[xdf1->dstart];\n"}]}