On 9/2/25 09:57, Phillip Wood wrote:

On 01/09/2025 21:43, Colin Stagner wrote:

quoted

The outer loop in git-subtree.sh:983 appears to iterate from the root 
commit forwards… and not from the HEAD backwards.

     git rev-list --topo-order --reverse --parents $rev $unrevs
     #                         ^^^^^^^^^

Since the iteration is ancestor-first, I'm having difficulty seeing 
why `should_ignore_subtree_split_commit()` would want to do an 
ancestor traversal at all. It already sees the commits ancestor- 
first.

check_parents() recurses into process_split_commit() rather than the
loop that call should_ignore_subtree_split_commit(). I think what this
check does do is prune some parents which stops check_parents() from
recursing into other subtrees so the check is in the right place.

I agree. In the original patch [1], Zach indicated that the check 
results in a significant speedup for rejoin-heavy repos. The check is 
clearly doing something.

Performance improvements may be possible. Instead of looking at commits 
one at a time, this operation might be faster as part of a one-shot 
HEAD-to-root traversal:

    git log --grep 'for stuff' --format='%(trailers:...)' $unrev..HEAD

Commits that are deemed "uninteresting" or unnecessary could then be 
provided, in bulk, as negative refs to the `git rev-list` traversal.

But my plan is to make the smallest and most portable maint-2.44 bugfix. 
I think that non-essential performance changes are a task for later.

quoted

I am *much* less certain about the second test:

     test -z "$(git log -1 \
                --grep="git-subtree-dir: $arg_prefix$" $rev)"

If I remove this `test -z`, all the unit tests still pass—including mine. There may not be any test coverage for this line.

I'm not very familiar with git-subtree but I thought this was ensuring 
that we did not exclude the ancestors of a squash or split that involves 
the subtree that we're interested in.

It does, but I am still having problems finding commits that actually 
trigger it.

It appears that `find_existing_splits()` in git-subtree.sh:459 filters 
out the commits that the `test -z` I quoted above would otherwise 
detect. `find_existing_splits()` searches for a previous --rejoin commit 
to use as an `$unrev` stopping point for the rev-walk. It searches for 
commits matching

     git log --grep="^git-subtree-dir: $dir/*\$"

in combination with `git-subtree-mainline:`.

This is essentially the same test as in 
`should_ignore_subtree_split_commit()`.

In --ignore-joins mode, `find_existing_splits()` looks for different 
commits. I experimented a bit with adding --ignore-joins to some of the 
existing unit tests, but I still could not find any instance where this 
`test -z` makes a difference.

That said... I am inclined to keep this second test. The bug I am 
patching is the result of an overzealous prune. The last thing I want to 
do is to inadvertently prune commits we need for the sake of a 
performance boost.

I wouldn't be surprised if the test coverage was lacking.

There don't appear to be any tests at all for --ignore-joins, aside from 
option parsing.


[1]: 98ba49ccc2 (subtree: fix split processing with multiple subtrees 
present, 2023-12-01)