Re: [PATCH v5 0/6] Introduce git-last-modified(1) command
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Jul 17, 2025, 23:39 UTC
- Message-ID
- <aHmJmwvYmAyi3Nlh@nand.local>
- In-Reply-To
- <20250716133206.1787549-1-toon@iotcl.com>
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...
Show 5 quoted lines
> 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.
Show 5 quoted lines
> * 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.
> * The last-modified command isn't recursive by default. If you want > recurse into subtrees, you need to pass `-r`.
OK.
Show 5 quoted lines
> * 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.
> * 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.
Show 14 quoted lines
> * 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 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.
Thanks, Taylor