[git 2.26] stat counts reported by commit and log are different

Hi,

I just noticed that the numbers reported are different:

% git commit -m 'major code cleanup for SCIMPatchOperationUtil'
[nkiesel/nextrelease/SCIM_2 d4db6f6d83f] major code cleanup for
SCIMPatchOperationUtil
 1 file changed, 2106 insertions(+), 3367 deletions(-)
 rewrite utils/SCIMPatchOperationUtil.java (74%)
% git log --stat -1
commit d4db6f6d83f (HEAD -> nkiesel/nextrelease/SCIM_2)
Author: Norbert Kiesel <nkiesel@metricstream.com>
Date:   Wed Apr 8 18:49:27 2020 -0700

    major code cleanup for SCIMPatchOperationUtil

 utils/SCIMPatchOperationUtil.java | 3807
+++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------------------
 1 file changed, 1273 insertions(+), 2534 deletions(-)
% wc -l utils/SCIMPatchOperationUtil.java
2106 utils/SCIMPatchOperationUtil.java

As you can see, the number of lines in the current file match the number of insertions reported by commit. I suspect some "automated line ending conversion magic" is at fault here (the file has "\r\n" for my Windows-impaired co-developers but I modified it on a Linux system). `git config --get core.autocrlf` on my machine returns `input`. However, I think it would still be better to always report the same numbers, no?

On Wed, Apr 08, 2020 at 07:04:02PM -0700, Norbert Kiesel wrote:

The commit summary diff is done with the break-detection option on. You can tell it kicked in here because of the "rewrite" line. What that means is that the changes to the file were so extensive that Git decided the file had been totally rewritten, and broke it into a separate add/delete. That has two implications:

  - we'd consider it a candidate for rename detection; that didn't
    happen here because there were no other files added or deleted to
    serve as source/dest candidates

  - the diff will be reported as a complete removal of the old content
    and the addition of the new, _even for lines that were the same_

But in git-log break-detection isn't on by default. Try adding "-B" and you will get the other numbers.

Try also adding "-p" to see the whole-diff in action.

-Peff

Thanks for the explanation. I still wonder why break-detection is by default enabled for commit but disabled for log. Is there any rationale for this?

On Thu, Apr 9, 2020 at 7:00 AM Jeff King <peff@peff.net> wrote:

On Thu, Apr 09, 2020 at 02:58:25PM -0700, Norbert Kiesel wrote:

It's enabled for commit, because it was enable for git-status, via 3eb2a15eb3 (builtin-commit: make summary output consistent with status, 2007-12-16). It was enabled for status because it makes rename detection more accurate; see the discussion at the end of this thread:

  https://lore.kernel.org/git/20071121171235.GA32233@sigill.intra.peff.net/

That tells us why it _is_ enable for commit, but not why it's not for log. Traditionally rename detection was _not_ enabled by default there. Because it can be expensive, we were quicker to enable it for single-diff commands like commit (as opposed to log, which is doing a bunch of diffs).

Much later, in 5404c116aa (diff: activate diff.renames by default, 2016-02-25), we turned on renames by default for git-log. But as far as I recall, nobody gave much thought to turning on break detection at the same time.

It might make sense to do so (and/or to make it possible to enable it by config like we did for years with diff.renames). But it definitely is way more expensive. For a tree-only diff, we don't usually have to look at most blobs at all. Even with rename detection, we only have to look at inexact rename candidates. But break detection must look at every single modified file (timings are on git.git):

  [no renames at all, pretty fast]
  $ time git log --no-renames --raw >/dev/null
  real	0m2.231s
  user	0m2.203s
  sys	0m0.028s

  [adding in renames isn't very expensive]
  $ time git log -M --raw >/dev/null
  real	0m2.284s
  user	0m2.224s
  sys	0m0.060s

  [but break detection is]
  $ git log -B -M --raw >/dev/null
  real	0m33.377s
  user	0m32.942s
  sys	0m0.424s

A more fair comparison would actually generate content-level diffs, which need to look in the blobs, like:

  $ time git log -M -p >/dev/null
  real	0m23.287s
  user	0m22.854s
  sys	0m0.432s

  $ time git log -B -M -p >/dev/null
  real	0m49.763s
  user	0m49.282s
  sys	0m0.480s

So not quite as bad percentage-wise, but still pretty expensive. And for not a huge benefit. There are ~261 impacted commits. You can see a recent example with:

  git show -B -M --stat --summary ce6521e44

where we find that most of builtin/fmt-merge-msg.c was moved to fmt-merge-msg.c. It's nice, but it's expensive enough that it probably shouldn't be the default.

-Peff

Jeff King <peff@peff.net> writes:

Not only that, it can cost correctness-wise.  Until this

  https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/

gets corrected, it is not advisable to enable -B and -M at the same time.

On Thu, Apr 09, 2020 at 03:55:30PM -0700, Junio C Hamano wrote:

Ah, I forgot about that. I think the resulting diff might still be useful for a human to read, but yeah, it cannot be applied (though even there, it would be nice for it to make a little more clear to humans what's happening with the destination file).

I wonder if we ought to remove -B from its use in status/commit, but I guess there we are never generating a diff to apply, but rather one to help humans understand what happened.

-Peff