[go: up one dir, main page]
Skip to content

Introduce git-last-modified(1) command

Toon Claesrequested to merge
toon-blame-tree into master

On many forges the tree view is shown in combination with commit data. In such a view each tree entry is accompanied with the commit message and date that last modified that tree entry. Something similar like:

| README.md   | README: *.txt -> *.adoc fixes                           | 4 months ago |
| RelNotes    | Start 2.51 cycle, the first batch                       | 4 weeks ago  |
| SECURITY.md | SECURITY: describe how to report vulnerabilities        | 4 years      |
| abspath.c   | abspath: move related functions to abspath              | 2 years      |
| abspath.h   | abspath: move related functions to abspath              | 2 years      |
| aclocal.m4  | configure: use AC_LANG_PROGRAM consistently             | 15 years ago |
| add-patch.c | pager: stop using `the_repository`                      | 7 months ago |
| advice.c    | advice: allow disabling default branch name advice      | 4 months ago |
| advice.h    | advice: allow disabling default branch name advice      | 4 months ago |
| alias.h     | rebase -m: fix serialization of strategy options        | 2 years      |
| alloc.h     | git-compat-util: move alloc macros to git-compat-util.h | 2 years ago  |
| apply.c     | apply: only write intents to add for new files          | 8 days ago   |
| archive.c   | Merge branch 'ps/parse-options-integers'                | 3 months ago |
| archive.h   | archive.h: remove unnecessary include                   | 1 year       |
| attr.h      | fuzz: port fuzz-parse-attr-line from OSS-Fuzz           | 9 months ago |
| banned.h    | banned.h: mark `strtok()` and `strtok_r()` as banned    | 2 years      |

This series adds the git-last-modified(1) to feed this view. In the past the subcommand was proposed1 to be named git-blame-tree(1). This version is based on the patches shared by the kind people at GitHub2.

What is different from the series shared by GitHub:

  • Renamed the subcommand from blame-tree to last-modified. There was some consensus5 this name works better, so let's give it a try and see how this name feels.

  • Patches for --max-depth are excluded. I've submitted them as a separate patch series6.

  • The last-modified command isn't recursive by default. If you want recurse into subtrees, you need to pass -r.

  • The patches in 'tb/blame-tree' at Taylor's fork4 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.

  • All the new last-modified machinery is no longer implemented in a library layer (at the root of the project), but directly in the builtin. So far the code is fairly small (little over 300 lines of code) and there are no other users of this code anyway. Also the library level code taken from Taylor's fork required to pass argc and argv into it. It's quite awkward the library code was so tightly coupled with user interaction.

  • 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.

  • 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.

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.

Again thanks to Taylor and the people at GitHub for sharing these patches. I hope we can work together to get this upstreamed.

Changes in v7:

  • Fix case when bloom filters were used and a commit range was given. This bug was uncovered in CI.
  • Rename the long option for -t to --show-trees. This option no longer implies option -r. And resemble these changes in the documentation, with a few other small documentation tweaks.
  • Move prepare_commit_graph() into get_bloom_filter_settings() which no longer requires last-modified to worry about it itself. This is similar to repo_find_commit_pos_in_graph() and lookup_commit_in_graph()
  • Bring back the call to commit_graph_generation() in maybe_changed_path(). This is also called in the same function in blame.c and in check_maybe_different_in_bloom_filter() in revision.c. I couldn't find a test case that triggers this exit condition, but it should not have negative side-effects.
  • No longer call diff_free() on the copy we make when populating the paths of struct last_modified. Because we weren't doing a deep copy, this could clean up fields used later on by the original. Instead only call clear_pathspec(). A comment to clarify this mechanism better is added.
  • Add BUG() call to exit condition that shouldn't happen.
  • Switch some int types to bool types.

Changes in v6:

  • Only the first 3 patches are kept. The last 3 patches worked toward adding an extra option --format. The way it was implemented was heavily debatable and in the end it is not required for a first iteration, so they are dropped.
  • Function prepare_commit_graph() is exported and used in generation_numbers_enabled().
  • Since the library layer was removed and all the code was moved into the builtin, there was still some leftovers from using a callback mechanism to display the results. This is removed (as far as possible) and instead last_modified_emit() always, this function was called show_entry() previously.
  • Code is rebased to use refactoring in the bloom filter API.

Changes in v5:

  • Added a patch to allow for an "extended" format. The name for this option is open for debate (please, all input is welcome). But the main goal of this series is to provide the data needed for the "forge tree view" as demoed at the top of this cover letter. With this extra patch (and the prepatory patch to pretty.[ch]), I hope the use-case because more clear. But because it wasn't included in previous 4 versions I also wouldn't mind sending a separate patch series for it.
  • Removed the call to sort(1) the t8020 tests. This was needed for the tests for --extended.
  • I'm adding a fixup! commit to be compatible with in-flight patches for bloom filter optimizations: https://lore.kernel.org/git/20250712093517.17907-1-yldhome2d2@gmail.com/ This patch can be dropped if current series lands before those.

Changes in v4:

Changes in v3:

  • Updated benchmarks in commit messages.
  • Removed the patches that attempt to increase performance for tree entries that have not been updated in a long time. (see above)
  • Move handling failure in last_modified_init() to the caller.
  • Sorted #include clauses lexicographically.
  • Removed unneeded commit in struct last_modified_entry.
  • Renamed some functions/variables and added some comments to make it easier to understand.
  • Removed unnecessary checking of the commit-graph generation number.
  • Link to v2: https://lore.kernel.org/r/20250523-toon-new-blame-tree-v2-0-101e4ca4c1c9@iotcl.com

Changes in v2:

Edited by Toon Claes

Merge request reports