Re: [PATCH v5 0/6] Introduce git-last-modified(1) command

[Toon Claes <toon@iotcl.com>]

Taylor Blau <me@ttaylorr.com> writes:

> On Wed, Jul 16, 2025 at 03:32:00PM +0200, Toon Claes wrote:
>> This series adds the git-last-modified(1) to feed this view. In the past
>> the subcommand was proposed[1] to be named git-blame-tree(1). This
>> version is based on the patches shared by the kind people at GitHub[2].
>
> Sorry for completely dropping this from my review queue. Let me try and
> give it a read...

No worries, we all got work to do ;)

>> What is different from the series shared by GitHub:
>>
>> * Renamed the subcommand from `blame-tree` to `last-modified`. There was
>>   some consensus[5] this name works better, so let's give it a try and
>>   see how this name feels.
>
> Hmmph. I prefer the "blame-tree" name personally, but I am (a) biased,
> and (b) used to it over "last-modified", so I don't think my preference
> or bias should count for much here.

Well, for what it's worth I like "blame-tree" more as well. But didn't
feel strong enough to push it through.

>> * Patches for --max-depth are excluded. I think it's a separate topic to
>>   discuss and I'm not sure it needs to be part of series anyway. The
>>   main patch was submitted in the previous attempt[3] and if people
>>   consider it valuable, I'm happy to discuss that in a separate patch
>>   series.
>
> Yeah, makes sense.

I might be revisiting this, because recently I've integrating the WIP
command in our tech stack I noticed having this option would be
useful/required.

>> * The last-modified command isn't recursive by default. If you want
>>   recurse into subtrees, you need to pass `-r`.
>
> OK.
>
>> * The patches in 'tb/blame-tree' at Taylor's fork[4] implements a
>>   caching layer. This feature reads/writes cached results in
>>   `.git/blame-tree/<hash>.btc`. To keep this series to a reviewable
>>   size, that feature is excluded from this series. I think it's better
>>   to submit this as a separate series.
>
> Makes sense; the caching feature was primarily implemented by Stolee and
> I think for our purposes here can be considered additive and not
> essential to the basic functionality of this new command. For what it's
> worth, I *would* like[^1] to see those features sent to the list at some
> point, but I agree that they are a significant source of additional
> complexity. So punting on them for now seems like the right direction to
> me.
>
> [^1]: My ulterior motive here would be to eventually ditch GitHub's
>   "blame-tree" command entirely and remove it from GitHub's diff to
>   upstream. I'm happy to help however I can with that effort once this
>   series lands.

Obviously. I know how it goes, more code maintained by the community is
better for everyone.

>> * Squashed various commits together. Like they introduced a flag
>>   `--go-faster`, which later became the default and only implementation.
>>   That story was wrapped up in a single commit.
>
> Perfect, thank you. I figured that we would not want to keep temporary
> measures around like the "--go-faster" flag, but I also figured that
> they may be helpful in unpacking the history of this command, hence why
> I sent them in the first place.

I wasn't sure about this. I've had a hard time unraveling what in the 55
patches in your branch was valuable and what could be squashed into
other commits.

>> * Dropped the patches that attempt to increase performance for tree
>>   entries that have not been updated in a long time. In my testing I've
>>   seen both performance improvements *and* degradation with these
>>   changes:
>>
>>   Test                                        HEAD~             HEAD
>>   ------------------------------------------------------------------------------------
>>   8020.1: top-level last-modified             4.52(4.38+0.11)   2.03(1.93+0.08) -55.1%
>>   8020.2: top-level recursive last-modified   5.79(5.64+0.11)   8.34(8.17+0.11) +44.0%
>>   8020.3: subdir last-modified                0.15(0.09+0.06)   0.19(0.14+0.06) +26.7%
>>
>>   Before we include these patches, I want to make sure these changes
>>   have positive impact in all/most scenarios. This can happen in a
>>   separate series.
>
> Hmm. It's been long enough that I honestly don't remember the details
> here, but I agree that this is worth looking into at some point in the
> future.

I've had this patch included in version 2[1]. I'd love to include it,
but it didn't give the results we were expecting. Over time I became
more confortable with these changes. Let me see if I can get more
insights about it.

>> I've set myself as the author and added Based-on-patch-by trailers to
>> credit the original authors. Let me know if you disagree.
>
> I can't speak for the other authors of this command, but I have no issue
> being ~~blamed~~ credited with a "Based-on-patch-by" trailer ;-).
>
>> Again thanks to Taylor and the people at GitHub for sharing these
>> patches. I hope we can work together to get this upstreamed.
>
> Ditto.

<3

[1]: https://lore.kernel.org/git/20250523-toon-new-blame-tree-v2-4-101e4ca4c1c9@iotcl.com/

-- 
Cheers,
Toon