Re: [PATCH 0/8] Introduce git-blame-tree(1) command

From: Jeff King <peff@peff.net>
Date: Mar 27, 2025, 06:32 UTC
Message-ID: <20250327063243.GB3042475@coredump.intra.peff.net>
In-Reply-To: <Z+Rl43VR3ErE0Bu4@nand.local>

On Wed, Mar 26, 2025 at 04:38:59PM -0400, Taylor Blau wrote:

Show 16 quoted lines

> On Wed, Mar 26, 2025 at 09:18:24PM +0100, Toon Claes wrote:
> > This is yet another attempt to upstream the builtin command
> > `git-blame-tree(1)`. This command is similar to git-blame(1) and shows
> > the most recent modification to paths in a tree..
> >
> > The last attempt (I'm aware of) was made by Ævar in 2023[1]. That
> > series was based of patches by Peff written in 2011[2].
> 
> For what it's worth, the blame-tree implementation that this came from
> has evolved significantly since it was originally written in 2011. Most
> recently Stolee and I worked on a version that uses changed-path Bloom
> filters to narrow the search, passing un-blamed paths to their parents
> at each level of the traversal.
> 
> I wonder if it would be easier to start from scratch with the modern
> implementation rather than land this one and try to build on top of it.

Yeah, I'd suggest starting with that work from you and Stolee (though I do not know if it was ever made public?). It should be much faster and will have been battle-tested in production.

The pathspec-trie stuff is, I think, still a reasonable idea for general use. But IIRC, the rewritten blame-tree you guys worked on does not benefit from it, because it ditches pathspecs entirely (both because they're too slow without the tries, but also because it's important to continually narrow the pathspec while traversing). That trie code was never run in production, I think (and I see there is a patch to narrow the pathspec while traversing; I suspect that likewise was never used).

The max-depth diff code is also in theory a reasonable thing to have in general. But it is awkward to use, and not really necessary for blame-tree. There we really only care about recursing vs not recursing, but the usual "recursive" flag for diffing isn't enough (we have to recurse down to the tree of interest, but may not want to go further). I don't remember how that is handled in your blame-tree rewrites.

So that really mostly leaves the blame-tree scaffolding itself. I remember Junio left a lot of good comments on the original thread on how merges should be handled, but I don't think I ever fixed those bits. I don't recall what your rewritten code does there, but I think it may have improved things.

So yeah. I don't know if all of this is really a very good starting point. Taylor, if you can share the current code that GitHub is running, I think that would be beneficial for the community.

-Peff