{"thread":{"id":"53196","subject":"[git 2.26] stat counts reported by commit and log are different","startedAt":"2020-04-09T02:04:17Z","lastAt":"2020-04-09T23:12:56Z","messageCount":6,"participants":["Norbert Kiesel","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"395110","messageId":"CAM+g_Nv4UqQNAuYyo5zsTsiomCe4ueoM6ZGU1aqAjLGV9+jQJg@mail.gmail.com","threadId":"53196","inReplyTo":null,"subject":"[git 2.26] stat counts reported by commit and log are different","fromName":"Norbert Kiesel","fromEmail":"nkiesel@gmail.com","sentAt":"2020-04-09T02:04:02Z","receivedAt":"2020-04-09T02:04:17Z","isPatch":false,"sender":{"key":"nkiesel@gmail.com","avatar":null},"body":"Hi,\n\nI just noticed that the numbers reported are different:\n\n% git commit -m 'major code cleanup for SCIMPatchOperationUtil'\n[nkiesel/nextrelease/SCIM_2 d4db6f6d83f] major code cleanup for\nSCIMPatchOperationUtil\n 1 file changed, 2106 insertions(+), 3367 deletions(-)\n rewrite utils/SCIMPatchOperationUtil.java (74%)\n% git log --stat -1\ncommit d4db6f6d83f (HEAD -> nkiesel/nextrelease/SCIM_2)\nAuthor: Norbert Kiesel \nDate: Wed Apr 8 18:49:27 2020 -0700\n\n major code cleanup for SCIMPatchOperationUtil\n\n utils/SCIMPatchOperationUtil.java | 3807\n+++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------\n 1 file changed, 1273 insertions(+), 2534 deletions(-)\n% wc -l utils/SCIMPatchOperationUtil.java\n2106 utils/SCIMPatchOperationUtil.java\n\nAs you can see, the number of lines in the current file match the\nnumber of insertions reported by commit. I suspect some \"automated\nline ending conversion magic\" is at fault here (the file has \"\\r\\n\"\nfor my Windows-impaired co-developers but I modified it on a Linux\nsystem). `git config --get core.autocrlf` on my machine returns\n`input`. However, I think it would still be better to always report\nthe same numbers, no?\n"},{"id":"395122","messageId":"20200409135959.GB3494212@coredump.intra.peff.net","threadId":"53196","inReplyTo":"CAM+g_Nv4UqQNAuYyo5zsTsiomCe4ueoM6ZGU1aqAjLGV9+jQJg@mail.gmail.com","subject":"Re: [git 2.26] stat counts reported by commit and log are different","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T13:59:59Z","receivedAt":"2020-04-09T14:00:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 08, 2020 at 07:04:02PM -0700, Norbert Kiesel wrote:\n\n> % git commit -m 'major code cleanup for SCIMPatchOperationUtil'\n> [nkiesel/nextrelease/SCIM_2 d4db6f6d83f] major code cleanup for\n> SCIMPatchOperationUtil\n> 1 file changed, 2106 insertions(+), 3367 deletions(-)\n> rewrite utils/SCIMPatchOperationUtil.java (74%)\n\nThe commit summary diff is done with the break-detection option on. You\ncan tell it kicked in here because of the \"rewrite\" line. What that\nmeans is that the changes to the file were so extensive that Git decided\nthe file had been totally rewritten, and broke it into a separate\nadd/delete. That has two implications:\n\n - we'd consider it a candidate for rename detection; that didn't\n happen here because there were no other files added or deleted to\n serve as source/dest candidates\n\n - the diff will be reported as a complete removal of the old content\n and the addition of the new, _even for lines that were the same_\n\n> % git log --stat -1\n> commit d4db6f6d83f (HEAD -> nkiesel/nextrelease/SCIM_2)\n> Author: Norbert Kiesel \n> Date: Wed Apr 8 18:49:27 2020 -0700\n> \n> major code cleanup for SCIMPatchOperationUtil\n> \n> utils/SCIMPatchOperationUtil.java | 3807\n> +++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------\n> 1 file changed, 1273 insertions(+), 2534 deletions(-)\n\nBut in git-log break-detection isn't on by default. Try adding \"-B\" and\nyou will get the other numbers.\n\nTry also adding \"-p\" to see the whole-diff in action.\n\n-Peff\n"},{"id":"395145","messageId":"CAM+g_NuZ3pKAd80+HoR8-_0=N9wV28L-yyb1VhJhTbYH+RS0og@mail.gmail.com","threadId":"53196","inReplyTo":"20200409135959.GB3494212@coredump.intra.peff.net","subject":"Re: [git 2.26] stat counts reported by commit and log are different","fromName":"Norbert Kiesel","fromEmail":"nkiesel@gmail.com","sentAt":"2020-04-09T21:58:25Z","receivedAt":"2020-04-09T21:58:39Z","isPatch":false,"sender":{"key":"nkiesel@gmail.com","avatar":null},"body":"Thanks for the explanation. I still wonder why break-detection is by\ndefault enabled for commit but disabled for log. Is there any\nrationale for this?\n\nOn Thu, Apr 9, 2020 at 7:00 AM Jeff King wrote:\n>\n> On Wed, Apr 08, 2020 at 07:04:02PM -0700, Norbert Kiesel wrote:\n>\n> > % git commit -m 'major code cleanup for SCIMPatchOperationUtil'\n> > [nkiesel/nextrelease/SCIM_2 d4db6f6d83f] major code cleanup for\n> > SCIMPatchOperationUtil\n> > 1 file changed, 2106 insertions(+), 3367 deletions(-)\n> > rewrite utils/SCIMPatchOperationUtil.java (74%)\n>\n> The commit summary diff is done with the break-detection option on. You\n> can tell it kicked in here because of the \"rewrite\" line. What that\n> means is that the changes to the file were so extensive that Git decided\n> the file had been totally rewritten, and broke it into a separate\n> add/delete. That has two implications:\n>\n> - we'd consider it a candidate for rename detection; that didn't\n> happen here because there were no other files added or deleted to\n> serve as source/dest candidates\n>\n> - the diff will be reported as a complete removal of the old content\n> and the addition of the new, _even for lines that were the same_\n>\n> > % git log --stat -1\n> > commit d4db6f6d83f (HEAD -> nkiesel/nextrelease/SCIM_2)\n> > Author: Norbert Kiesel \n> > Date: Wed Apr 8 18:49:27 2020 -0700\n> >\n> > major code cleanup for SCIMPatchOperationUtil\n> >\n> > utils/SCIMPatchOperationUtil.java | 3807\n> > +++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------\n> > 1 file changed, 1273 insertions(+), 2534 deletions(-)\n>\n> But in git-log break-detection isn't on by default. Try adding \"-B\" and\n> you will get the other numbers.\n>\n> Try also adding \"-p\" to see the whole-diff in action.\n>\n> -Peff\n"},{"id":"395149","messageId":"20200409224747.GA4173825@coredump.intra.peff.net","threadId":"53196","inReplyTo":"CAM+g_NuZ3pKAd80+HoR8-_0=N9wV28L-yyb1VhJhTbYH+RS0og@mail.gmail.com","subject":"Re: [git 2.26] stat counts reported by commit and log are different","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T22:47:47Z","receivedAt":"2020-04-09T22:47:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 02:58:25PM -0700, Norbert Kiesel wrote:\n\n> Thanks for the explanation. I still wonder why break-detection is by\n> default enabled for commit but disabled for log. Is there any\n> rationale for this?\n\nIt's enabled for commit, because it was enable for git-status, via\n3eb2a15eb3 (builtin-commit: make summary output consistent with status,\n2007-12-16). It was enabled for status because it makes rename detection\nmore accurate; see the discussion at the end of this thread:\n\n https://lore.kernel.org/git/20071121171235.GA32233@sigill.intra.peff.net/\n\nThat tells us why it _is_ enable for commit, but not why it's not for\nlog. Traditionally rename detection was _not_ enabled by default there.\nBecause it can be expensive, we were quicker to enable it for\nsingle-diff commands like commit (as opposed to log, which is doing a\nbunch of diffs).\n\nMuch later, in 5404c116aa (diff: activate diff.renames by default,\n2016-02-25), we turned on renames by default for git-log. But as far as\nI recall, nobody gave much thought to turning on break detection at the\nsame time.\n\nIt might make sense to do so (and/or to make it possible to enable it by\nconfig like we did for years with diff.renames). But it definitely is\nway more expensive. For a tree-only diff, we don't usually have to look\nat most blobs at all. Even with rename detection, we only have to look\nat inexact rename candidates. But break detection must look at every\nsingle modified file (timings are on git.git):\n\n [no renames at all, pretty fast]\n $ time git log --no-renames --raw >/dev/null\n real\t0m2.231s\n user\t0m2.203s\n sys\t0m0.028s\n\n [adding in renames isn't very expensive]\n $ time git log -M --raw >/dev/null\n real\t0m2.284s\n user\t0m2.224s\n sys\t0m0.060s\n\n [but break detection is]\n $ git log -B -M --raw >/dev/null\n real\t0m33.377s\n user\t0m32.942s\n sys\t0m0.424s\n\nA more fair comparison would actually generate content-level diffs,\nwhich need to look in the blobs, like:\n\n $ time git log -M -p >/dev/null\n real\t0m23.287s\n user\t0m22.854s\n sys\t0m0.432s\n\n $ time git log -B -M -p >/dev/null\n real\t0m49.763s\n user\t0m49.282s\n sys\t0m0.480s\n\nSo not quite as bad percentage-wise, but still pretty expensive. And for\nnot a huge benefit. There are ~261 impacted commits. You can see a\nrecent example with:\n\n git show -B -M --stat --summary ce6521e44\n\nwhere we find that most of builtin/fmt-merge-msg.c was moved to\nfmt-merge-msg.c. It's nice, but it's expensive enough that it probably\nshouldn't be the default.\n\n-Peff\n"},{"id":"395150","messageId":"xmqq8sj4mfq5.fsf@gitster.c.googlers.com","threadId":"53196","inReplyTo":"20200409224747.GA4173825@coredump.intra.peff.net","subject":"Re: [git 2.26] stat counts reported by commit and log are different","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-09T22:55:30Z","receivedAt":"2020-04-09T22:55:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It might make sense to do so (and/or to make it possible to enable it by\n> config like we did for years with diff.renames). But it definitely is\n> way more expensive.\n> ...\n> So not quite as bad percentage-wise, but still pretty expensive. And for\n> not a huge benefit. There are ~261 impacted commits. You can see a\n> recent example with:\n>\n> git show -B -M --stat --summary ce6521e44\n>\n> where we find that most of builtin/fmt-merge-msg.c was moved to\n> fmt-merge-msg.c. It's nice, but it's expensive enough that it probably\n> shouldn't be the default.\n\nNot only that, it can cost correctness-wise. Until this\n\n https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/\n\ngets corrected, it is not advisable to enable -B and -M at the same\ntime.\n\n"},{"id":"395152","messageId":"20200409231253.GA4180345@coredump.intra.peff.net","threadId":"53196","inReplyTo":"xmqq8sj4mfq5.fsf@gitster.c.googlers.com","subject":"Re: [git 2.26] stat counts reported by commit and log are different","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T23:12:53Z","receivedAt":"2020-04-09T23:12:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 03:55:30PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It might make sense to do so (and/or to make it possible to enable it by\n> > config like we did for years with diff.renames). But it definitely is\n> > way more expensive.\n> > ...\n> > So not quite as bad percentage-wise, but still pretty expensive. And for\n> > not a huge benefit. There are ~261 impacted commits. You can see a\n> > recent example with:\n> >\n> > git show -B -M --stat --summary ce6521e44\n> >\n> > where we find that most of builtin/fmt-merge-msg.c was moved to\n> > fmt-merge-msg.c. It's nice, but it's expensive enough that it probably\n> > shouldn't be the default.\n> \n> Not only that, it can cost correctness-wise. Until this\n> \n> https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/\n> \n> gets corrected, it is not advisable to enable -B and -M at the same\n> time.\n\nAh, I forgot about that. I think the resulting diff might still be\nuseful for a human to read, but yeah, it cannot be applied (though even\nthere, it would be nice for it to make a little more clear to humans\nwhat's happening with the destination file).\n\nI wonder if we ought to remove -B from its use in status/commit, but I\nguess there we are never generating a diff to apply, but rather one to\nhelp humans understand what happened.\n\n-Peff\n"}]}