Re: [PATCH] contrib/subtree: fix split with squashed subtrees

From: Colin Stagner <ask+git@howdoi.land>
Date: Sep 1, 2025, 20:43 UTC
Message-ID: <ee480c22-0dd3-4c45-a2bd-838c238f1d55@howdoi.land>
In-Reply-To: <00e76b7e-ce4f-44d9-acd9-466c6b14f41b@gmail.com>

On 9/1/25 08:54, Phillip Wood wrote:

Colin Stagner <ask+git@howdoi.land> writes:

>> -    if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
>> +    if test -n "$(git log -1 --grep="git-subtree-dir:" "$rev^!")"
> 
> We could drop the "-1" as we're only considering a single commit.

Concur.

Show 12 quoted lines

>> -        if test -z "$(git log -1 --grep="git-subtree-mainline:" 
>> $rev)" &&
>> -            test -z "$(git log -1 --grep="git-subtree-dir: 
>> $arg_prefix$" $rev)"
>> +        if test -z "$(git log -1 --grep="git-subtree-mainline:" 
>> "$rev^!")" &&
>> +            test -z "$(git log -1 --grep="git-subtree-dir: 
>> $arg_prefix$" "$rev^!")"
>
> I'm less sure about this change. Is the second test checking
> making sure we don't prune this commit if it has an ancestor
> that is a subtree merge for the subtree we're interested in?

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. But there could be a reason that I don't know.

Here is a more long-winded breakdown of these tests. From what I can determine:

     test -z "$(git log -1 --grep="git-subtree-mainline:" "$rev")

excludes squashed commits created from

     git subtree merge --prefix subM --squash srcBranch

The --squash creates two commits:

1. A single-parent "Squashed 'subM/' content from", which
    squashes the changes from srcBranch. This commit's tree
    is like the one on srcBranch. It does not have the `subM/`
    prefix.

2. A merge commit which rewrites the tree in (1) to add
    the `subM/` leading directory, then merge it with the
    current branch. The merge commit doesn't have any
    `git-subtree:` trailers.

We must exclude (1) since the trees aren't actually compatible with HEAD. (They don't have the `subM` prefix). We must keep (2). The above `test -z` appears to do this.

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

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

I think this was intended to keep the mainline portion from a previous `git subtree split --rejoin`. But 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 will probably omit this `test` from v2.

> It would be very helpful if Zach could comment on what was intended here.

Yes, this would aid my understanding a lot.

Show 9 quoted lines

> If it turns out that all three tests only want to consider a single 
> commit then it would be be more efficient to run a single git command 
> and check the output with something like
> 
>      git show -s --format='%(trailers:key=git-subtree-dir,key=git- 
> subtree-mainline' $rev | while read trailer
>          do
>              # check trailers here using case "$trailer"
>          done

This is a cleaner approach, and I'll explore it for v2. Any objection to long options like `--no-patch` instead of `-s`? I find these are better for scripts since there's less hunting around in man pages.

Thanks for your review,

Colin