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

From: "Kristoffer Haugsbakk" <[email-masked]>
Date: Tue, 01 Jul 2025 22:35:13 +0200

On Fri, May 23, 2025, at 11:33, Toon Claes wrote:
> 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/[email-masked]/
> [2]:
> https://lore.kernel.org/git/[email-masked]/
> [3]: [email-masked]:ttaylorr/git.git
> [4]: https://lore.kernel.org/git/[email-masked]/
> --
> Cheers,
> Toon
>
> Signed-off-by: Toon Claes <[email-masked]>
> ---
> 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/[email-masked]

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
```