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:
https://public-inbox.org/git/20190226170648.211847-1-brho@xxxxxxxxxx/
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:
https://public-inbox.org/git/20190226170648.211847-4-brho@xxxxxxxxxx/
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)
else
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
else
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?
Barret
