Web lists-archives.com

Re: [RFC PATCH 0/1] Fuzzy blame

Hi -

On 3/25/19 5:32 AM, Michael Platings wrote:
(resending in plain text mode, sorry for the noise)

Thanks Junio, that's super helpful!

A month or two ago I contacted the author of git-hyper-blame, Matt
Giuca, asking whether anyone had looked into adding the feature to git
blame. I didn't receive a response but maybe that prompted Barret
Rhoden's patch? Or maybe just a weird coincidence!

Weird coincidence.  It's a big company.  =)

I work on a project that needs a major reformatting, and one thing delaying me was the lack of an ability to ignore commits during blame. hyper-blame does that, but I never liked the fact that it wasn't directly in git.

@Barret I see your patches are a nice translation of git-hyper-blame.

Not sure if you've seen my latest version, updated to v4 (2019-02-26) here:


The one Junio has (br/blame-ignore) hasn't been updated - not sure if that's automated, or if it just fell through the cracks.

However could you give me your thoughts on the approach in my patch? A
comment in the git-hyper-blame source [1] says:
# This doesn't work that well if there are a lot of line changes within the
# hunk (demonstrated by GitHyperBlameLineMotionTest.testIntraHunkLineMotion).
# A fuzzy heuristic that takes the text of the new line and tries to find a
# deleted line within the hunk that mostly matches the new line could help.

My patch aims to implement this "fuzzy heuristic" so I'd love to get
your take on it.

This is an interesting idea, and it sounds like it might be complimentary to the blame-ignore work. Both have the flavor of "user knows this commit is special and wants special processing."

In my patch, I didn't try to find the likely original version of a line in a diff hunk. What I did amounted to finding blame based on the parent's image of the file. Example in this message:


Of note, line 12 was blamed on commit b, when it really came from commit a.

For any lines added beyond the size of the parent's image (e.g. the ignored commit added more lines than it removed), those lines were removed from blame contention - marked with all 0s.

In essence, my method did the following:

	for all suspect/child lines 'i' in a diff hunk
		if i <= parent's hunk size
			assign to parent line i (+ offset)
			mark umblamable

Due to the two cases being contiguous, each hunk would be broken up into at most two blame entries. (I actually short-circuit that for loop and just split at i == parent_size immediately).

Having a smart/fuzzy matcher sounds nicer. My patch passes blame to the parent. Yours finds the right *part* of the parent to blame, which means we have a better chance of finding the right original commit to blame.

I think you're doing this:

	for all suspect/child lines 'i' in a diff hunk
		if i matches parent line (say, x)
			assign to parent line x
			assign to child line i

From glancing at your code, it looks like you break up every blame entry into N entries, one for each line, which you need since each parent line X might not be adjacent to the matching lines in the child.

One question I have is under which circumstances do you find that you cannot match a suspect/child line to a parent? One obvious case is a commit that only adds lines - the parent's line set is the empty set. I think you catch that earlier in your code (parent_chunk_length == 0), though I guess there are other cases where the parent has lines, but they are below a certain match threshold?

For those cases where you can't find a match, I could imagine marking them as unblamable. The purpose of 'unblamable' in my patchset was to signal to not bother looking up further commit info for a final blame entry. It was largely so that the user (me!) wouldn't see a commit blamed when I explicitly told git to not tell me about that commit. Both approaches sound fine though.

Another question was whether or not you wanted this to apply per commit or for an entire blame session. It looks like your current code applies it overall, and not for a specific commit. I'd much prefer it to be per-commit, though maybe the -F is just for showing it working in the RFC.

The first thing that comes to mind for me is to plug your fuzzy logic into my patch set. Basically, in my commit near "These go to the parent", we do your line-by-line matching. We might not need my 'delta' check anymore, which was basically the 'if' part of my for loop (in text above). But maybe we need more info for your function.

That way, we'd get the per-commit control of when we ignore a commit from my patch, and we'd get the fuzzy-blaming brains of your patch, such that we try to find the right *part* of the parent to blame.

Anyway, let me know what your thoughts are. We could try to change that part of my code so that it just calls some function that tells it where in the parent to blame, and otherwise marks unblamable. Then your patch can replace the logic?