Re: [PATCH RFC v2 0/5] Introduce git-last-modified(1) command

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Jul 1, 2025, 20:35 UTC
Message-ID: <f0c508cc-5c6b-4c4b-a3f3-0cdd8d1071e5@app.fastmail.com>
In-Reply-To: <20250523-toon-new-blame-tree-v2-0-101e4ca4c1c9@iotcl.com>

On Fri, May 23, 2025, at 11:33, Toon Claes wrote:

Show 66 quoted lines

> This is another attempt to upstream the ~~git-blame-tree(1)~~
> git-last-modified(1) subcommand. After my previous attempt[1] the
> people of GitHub shared their version of the subcommand, and this
> version integrates those changes.
>
> What is different from the series shared by GitHub:
>
> * Renamed the subcommand from `blame-tree` to `last-modified`. There was
>   some consensus[4] this name works better, so let's give it a try and
>   see how this name feels.
>
> * 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[2] and if people
>   consider it valuable, I'm happy to discuss that in a separate patch
>   series.
>
> * The patches in 'tb/blame-tree' at Taylor's fork[3] 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.
>
> * 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.
>
> * The last-modified command isn't recursive by default. If you want
>   recurse into subtrees, you need to pass `-r`.
>
> * Fixed all memory leaks, and removed the use of
>   USE_THE_REPOSITORY_VARIABLE.
>
> I've attempted to reuse commit messages as good as possible, but feel
> free to correct me where you think I didn't give proper credit or messed
> up. Although I have no idea what to do with the Signed-off-by trailers.
>
> I didn't modify the benchmark results in the commit messages, simply
> because I didn't get comparable results. In my benchmarks the difference
> between two implementations was negligible, and even in some scenarios
> the performance was worse in the "improved" implementation. As far as I
> can tell, I didn't break anything in my refactoring, because the version
> in these patches acts similar to Taylor's branch. To be honest, I cannot
> explain why...?
>
> Again thanks to Taylor and the people at GitHub for sharing these
> patches. I hope we can work together to get this upstreamed.
>
> [1]:
> https://lore.kernel.org/git/20250326-toon-blame-tree-v1-0-4173133f3786@iotcl.com/
> [2]:
> https://lore.kernel.org/git/20250326-toon-blame-tree-v1-3-4173133f3786@iotcl.com/
> [3]: git@github.com:ttaylorr/git.git
> [4]: https://lore.kernel.org/git/aCbBKj7O9LjO3SMK@pks.im/
> --
> Cheers,
> Toon
>
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> Changes in v2:
> - The subcommand is renamed from `blame-tree` to `last-modified`
> - Documentation is added. Here we mark the command as experimental.
> - Some test cases are added related to merges.
> - Link to v1:
> https://lore.kernel.org/r/20250422-toon-new-blame-tree-v1-0-fdb51b8a394a@iotcl.com

It feels like the command strays a bit from the usual patterns to me. For paths/files that is. I like this:

``` $ git last-modified -r refs.c refs.h 062b914c841329a003f74e1340ea5178391274a6 refs.c 47478802daddf3f9916111307f153c6298ffc0bc refs.h ```

I ask for two files and I get those in the output.

But for individual files in subdirectories:

``` $ git last-modified refs.c refs.h Documentation/git-last-modified.adoc Documentation/git-config.adoc 3691fe72d927658ae77ade7fe967544fc6739e67 Documentation 062b914c841329a003f74e1340ea5178391274a6 refs.c 47478802daddf3f9916111307f153c6298ffc0bc refs.h ```

Same as if I ask for `Documentation`:

``` $ git last-modified refs.c refs.h Documentation/ 3691fe72d927658ae77ade7fe967544fc6739e67 Documentation 062b914c841329a003f74e1340ea5178391274a6 refs.c 47478802daddf3f9916111307f153c6298ffc0bc refs.h ```

But I didn’t ask for the directory first.  I asked for two files.

I have to use `-r` (recurse):

``` $ git last-modified -r refs.c refs.h Documentation/git-last-modified.adoc Documentation/git-config.adoc 3691fe72d927658ae77ade7fe967544fc6739e67 Documentation/git-last-modified.adoc 062b914c841329a003f74e1340ea5178391274a6 refs.c 47478802daddf3f9916111307f153c6298ffc0bc refs.h 0fbe93b36c05bbf4156c157f27998938ce312265 Documentation/git-config.adoc ```

And `-r` with a directory like `Documentation` will recurse through that directory.

As a user I imagine I want `-r` to control whether to, say, only show each directory under `Documentation`. But now you seem to get a special case of allowing directly listing first-level files but not the ones in subdirectories.

I’m more used to being able to use individual files if I want that as well as pathspecs for recursion. But now I just get the directory:

``` git last-modified -- 't/*' 532d9a0984e6464deadb6bdb0287fcce2990adc9 t ```

But I can still use pathspec magic which is nice:

``` $ git last-modified -r -- 't/*' ':^t/t1016*' | grep 1016 <empty> ```

I’m imagining that you may want to feed a specific pathspec to the command and get only the output for those that match. Without having worry about turning on recursion since that may cover some of the things you want but also make it do too much. Well maybe that is just as controllable here but it seems less obvious than for commands where the pathspec is used more directly (?). I appreciate when these commands allow me to express things directly without postprocessing (no excessive pipelining).

Also you get a sort of trailing error if you give a non-existing option:

``` $ git last-modified --recursive error: unknown last-modified argument: --recursive fatal: error setting up last-modified traversal ```