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

From: Phillip Wood <[email-masked]>
Date: Mon, 1 Sep 2025 14:54:11 +0100

Hi Colin

On 24/08/2025 20:10, Colin Stagner wrote:
> 98ba49ccc2 (subtree: fix split processing with multiple subtrees
> present, 2023-12-01) increases the performance of
> 
>      git subtree split --prefix=subA
> 
> by ignoring subtree merges which are outside of `subA/`. It also
> introduces a regression. Subtree merges that should be retained
> are incorrectly ignored if they:
> 
> 1. are nested under `subA/`; and
> 2. are merged with `--squash`.
> 
> For example, a subtree merged like:
> 
>      git subtree merge --squash --prefix=subA/subB "$rev"
>      #                 ^^^^^^^^          ^^^^
> 
> is erroneously ignored during a split of `subA`. This causes
> missing tree files and different commit hashes starting in
> git v2.44.0-rc0.
> 
> The method:
> 
>      should_ignore_subtree_split_commit REV
> 
> should test only if REV is a subtree commit, but the combination of
> 
>      git log -1 --grep=...
> 
> actually searches all *parent* commits until a `--grep` match is
> discovered. Limit these checks to the current REV only.

Thanks for the clear explanation of the problem and the proposed solution.

> Tests now cover nested subtrees.

Great
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 3fddba797c..139049351d 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -788,17 +788,17 @@ ensure_valid_ref_format () {
>   # Usage: check if a commit from another subtree should be
>   # ignored from processing for splits
>   should_ignore_subtree_split_commit () {
>   	assert test $# = 1
>   	local rev="$1"
> -	if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
> +	if test -n "$(git log -1 --grep="git-subtree-dir:" "$rev^!")"

This makes sense as we only want to grep the current commit. We could 
drop the "-1" as we're only considering a single commit.

>   	then
> -		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? It would be very helpful if Zach 
could comment on what was intended here.

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

Thanks

Phillip

>   		then
>   			return 0
>   		fi
>   	fi
>   	return 1
>   }
>   
>   # Usage: process_split_commit REV PARENTS
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> index 3edbb33af4..1ddc213621 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -68,6 +68,34 @@ test_create_pre2_32_repo () {
>   	git -C "$1-clone" replace HEAD^2 $new_commit
>   }
>   
> +# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...
> +#
> +# Create a simple subtree on a new branch named ORPHAN in REPO.
> +# The subtree is then merged into the current branch of REPO,
> +# under PREFIX. The generated subtree has has one commit
> +# with subject and tag FILENAME with a single file "FILENAME.t"
> +#
> +# When this method returns:
> +# - the current branch of REPO will have file PREFIX/FILENAME.t
> +# - REPO will have a branch named ORPHAN with subtree history
> +#
> +# additional arguments are forwarded to "subtree add"
> +test_create_subtree_add () {
> +	(
> +		cd "$1" &&
> +		orphan="$2" &&
> +		prefix="$3" &&
> +		filename="$4" &&
> +		shift 4 &&
> +		last="$(git branch --show-current)" &&
> +		git checkout --orphan "$orphan" &&
> +		git rm -rf . &&
> +		test_commit "$filename" &&
> +		git checkout "$last" &&
> +		git subtree add --prefix="$prefix" "$@" "$orphan"
> +	)
> +}
> +
>   test_expect_success 'shows short help text for -h' '
>   	test_expect_code 129 git subtree -h >out 2>err &&
>   	test_must_be_empty err &&
> @@ -426,6 +454,48 @@ test_expect_success 'split with multiple subtrees' '
>   		--squash --rejoin -d -m "Sub B Split 1" 2>&1 | grep -w "\[1\]")" = ""
>   '
>   
> +# When subtree split-ing a directory that has other subtree
> +# *merges* underneath it, the split must include those subtrees.
> +# This test creates a nested subtree, `subA/subB`, and tests
> +# that the tree is correct after a subtree split of `subA/`.
> +# The test covers:
> +# - An initial `subtree add`; and
> +# - A follow-up `subtree merge`
> +# both with and without `--squashed`.
> +for is_squashed in '' 'y';
> +do
> +	test_expect_success "split keeps nested ${is_squashed:+--squash }subtrees that are part of the split" '
> +		subtree_test_create_repo "$test_count" &&
> +		(
> +			cd "$test_count" &&
> +			mkdir subA &&
> +			test_commit subA/file1 &&
> +			git branch -m main &&
> +			test_create_subtree_add \
> +				. mksubtree subA/subB file2 ${is_squashed:+--squash} &&
> +			test -e subA/file1.t &&
> +			test -e subA/subB/file2.t &&
> +			git subtree split --prefix=subA --branch=bsplit &&
> +			git checkout bsplit &&
> +			test -e file1.t &&
> +			test -e subB/file2.t &&
> +			git checkout mksubtree &&
> +			git branch -D bsplit &&
> +			test_commit file3 &&
> +			git checkout main &&
> +			git subtree merge \
> +				${is_squashed:+--squash} \
> +				--prefix=subA/subB mksubtree &&
> +			test -e subA/subB/file3.t &&
> +			git subtree split --prefix=subA --branch=bsplit &&
> +			git checkout bsplit &&
> +			test -e file1.t &&
> +			test -e subB/file2.t &&
> +			test -e subB/file3.t
> +		)
> +	'
> +done
> +
>   test_expect_success 'split sub dir/ with --rejoin from scratch' '
>   	subtree_test_create_repo "$test_count" &&
>   	test_create_commit "$test_count" main1 &&
> 
> base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0