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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Colin

This is looking good. I've left a few comments below, especially on the 
new test.

On 05/09/2025 03:27, Colin Stagner wrote:
>   
> -# Usage: check if a commit from another subtree should be
> +# Usage: should_ignore_subtree_split_commit REV
> +#
> +# Check if REV is a commit from another subtree and should be
>   # ignored from processing for splits
>   should_ignore_subtree_split_commit () {
>   	assert test $# = 1
> -	local rev="$1"
> +
> +	git show \
> +		--no-patch \
> +		--no-show-signature \
> +		--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \
> +		"$1" |
> +	(
> +	have_mainline=
> +	subtree_dir=
> +
> +	while read -r trailer val
> +	do
> +		case "$trailer" in
> +		(git-subtree-dir:)
> +			subtree_dir="${val%/}" ;;
> +		(git-subtree-mainline:)
> +			have_mainline=y ;;
> +		esac
> +	done

This looks good, we run git log once and then parse the trailers. We do 
not use the optional '(' in case statements in our code though.

> -	if test -n "$(git log -1 --grep="git-subtree-dir:" $rev)"
> +	if test -n "${subtree_dir:-}" &&
> +		test -z "${have_mainline:-}" &&
> +		test "${subtree_dir}" != "$arg_prefix"

If we have a git-subtree-dir: trailer whose value does not match the 
subtree we're interested in and there is no git-subtree-mainline: 
trailer then we skip this commit - good. What's the idea behind using 
"${var:-}" rather than "{var}"?

>   	then
> -		if test -z "$(git log -1 --grep="git-subtree-mainline:" $rev)" &&
> -			test -z "$(git log -1 --grep="git-subtree-dir: $arg_prefix$" $rev)"
> -		then
> -			return 0
> -		fi
> +		return 0
>   	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 ca4df5be83..8bd45e7be7 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -67,6 +67,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 . &&

If you use "git switch --orphan" that clears the worktree for you

> +		test_commit "$filename" &&
> +		git checkout "$last" &&

I think this could be "git checkout @{-1}" and then we'd avoid having to 
run "git branch" above

> +		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 &&
> @@ -425,6 +453,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';

no need for ';' at the end of the line

> +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 &&

For tests that depend on the default branch name you can add

	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

to the start of the file before it sources test-lib.sh. Or we could 
change subtree_create_repo() to pass all its arguments along to 
test_create_repo() and use "subtree_create_repo --initial-branch=main 
$test_count"

> +			test_create_subtree_add \
> +				. mksubtree subA/subB file2 ${is_squashed:+--squash} &&
> +			test -e subA/file1.t &&

We have test_path_is_file() for this which prints a useful diagnostic 
message it it fails

Thanks

Phillip

> +			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: 09669c729af92144fde84e97d358759b5b42b555