git-blame not tracking copies

Hello,

During the discussion on the ffmpeg list about potential migration to git the following came up. It seems like a bug to me, so I said I would raise it here.

This is the output of a test script (which I can supply if wanted, but you can guess the content from the output.

Initialized empty Git repository in .git/
----- echo ABC to commit 1
Created initial commit beb7140: 1
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 newtest
----- echo DEF to commit 2
Created commit 207f5a3: 2
 1 files changed, 1 insertions(+), 0 deletions(-)
----- echo ghijk to commit 3
Created commit 14abf8c: 3
 1 files changed, 1 insertions(+), 0 deletions(-)
----- Blame 1...
^beb7140 (Andy Parkins 2007-05-02 20:25:27 +0100 1) ABC
207f5a35 (Andy Parkins 2007-05-02 20:25:27 +0100 2) DEF
14abf8ce (Andy Parkins 2007-05-02 20:25:27 +0100 3) ghijk
----- Copy newtest to newtest2, commit 4
Created commit 48861ce: 4
 1 files changed, 3 insertions(+), 0 deletions(-)
 create mode 100644 newtest2
----- Blame 2...
48861ced (Andy Parkins 2007-05-02 20:25:27 +0100 1) ABC
48861ced (Andy Parkins 2007-05-02 20:25:27 +0100 2) DEF
48861ced (Andy Parkins 2007-05-02 20:25:27 +0100 3) ghijk
----- Edit newtest2, commit 5
Created commit 2d2ec0f: 5
 1 files changed, 1 insertions(+), 0 deletions(-)
----- Blame 3...
^beb7140 newtest  (Andy Parkins 2007-05-02 20:25:27 +0100 1) ABC
207f5a35 newtest  (Andy Parkins 2007-05-02 20:25:27 +0100 2) DEF
2d2ec0f0 newtest2 (Andy Parkins 2007-05-02 20:25:27 +0100 3) XXXX
48861ced newtest2 (Andy Parkins 2007-05-02 20:25:27 +0100 4) ghijk

All git-blame commands are "git-blame -C1 -C1"

The issues are

 - Blame2 says all the lines come from commit 4, when actually they
   come from commits 1, 2 and 3.  It was pointed out that this is
   particularly annoying because the file is an exact copy and so the
   copy has the same hash as the original so should be easy to spot

 - The output isn't stable, even if blame2 had a good reason for not
   assigning lines 1 and 2 to their correct commits, why isn't the same
   true in blame3?

 - Blame3 incorrectly ascribes line 4 to commit 4, when it should have
   remained as it was in blame1 - to commit 3.

Andy

Andy Parkins <andyparkins@gmail.com> writes:

I am sick and not functioning well today, so will not be able to review what is happening with your example deeply, but here are some comments to get you started digging.

There is a built-in sanity valve in git-blame that refuses to pass down the blame via -M/-C for really trivial hunks. Without such safety, all the empty lines in the latest revision would be attributed to a random empty line in a random file in the root commit ;-).

By default, the sanity valve is set to refuse a hunk that has 20 or 40 alnum characters. These values seem to be appropriate for real life projects, but obviously the real-world case would be very different from a made-up "each commit changes one-line" test case and it would be understandable that the command would behave differently.

Also with real-life projects, probably depending on the coding style and merge patterns, I would not be surprised if there are rooms to tweak the heuristics.

On Thursday 2007 May 03, Junio C Hamano wrote:

I'm sorry to hear that.  I hope you feel better soon.

That sounds like an excellent feature. However - in the case of a full file-to-file copy, surely that valve may be safely disabled for that one step?

The copy is easily detectable because the hash is the same, so can't git-blame just continue having flipped itself on to the original filename? I suppose in essence a move is the same, the only difference between a move and a copy is that in one case the copy is spatial in the other it is temporal.

Please excuse the fact that in the above I've completely minimised the fact the job git-blame does is flippin' hard; I don't mean to imply that "any fule could fix it" - because I can't :-)

Andy

Andy Parkins <andyparkins@gmail.com> writes:

This turns out to be a simple and stupid boundary error.

The algorithm passes the blame by (ab)using diff to find the common section. We run an equivalent of "diff -u0", notice the lines that come out of it -- the lines that do not appear are common ones. Simply put, if we have this hunk at the beginning:

	@@ -4,1 +4,2 @@
        -a
        +A
        +B

the only information we are interested in this hunk are (1) that the hunk begins at line 4 in the postimage, so lines 1,2,3 in the postimage are the same as the preimage; and (2) that the different part ends just before line 6 (the hunk has two lines in the postimage which we know do not match the preimage). So if the next hunk from the diff look like this...

	@@ -6,1 +8,1 @@
        -e
        +E

then we know lines 6 and 7 in the postimage are the same as the preimage.

We need to use the information (2) from the last hunk in the patch and blame all the remaining lines to the parent.

When we are assigning blame from one file in the "current" commit down to one (possibly different) file in its parent commit in pass_blame_to_parent() function, we grab patch between the two, and we have the final "the rest are the same" call to blame_chunk(). However, the code to assign blame for only part of a file to unrelated file in its parent (i.e. -M/-C), the corresponding code in find_copy_in_blob() to use the diff output was missing this "the rest are the same" handling.

A 3-patch series follows.