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

> 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 a single commit REV, but the combination of
> 
>      git log -1 --grep=...
> 
> actually searches all *parent* commits until a `--grep` match is
> discovered.
> 
> Rewrite this method to test only one REV at a time. Extract commit
> information with a single `git` call as opposed to three. The
> `test` conditions for rejecting a commit remain unchanged.
> 
> Unit tests now cover nested subtrees.
> 
> Signed-off-by: Colin Stagner <ask+git@howdoi.land>
> ---
> 
> Notes:
>      This bugfix patch is intended for maint-2.44 and up.
> 
>   contrib/subtree/git-subtree.sh     | 36 +++++++++++----
>   contrib/subtree/t/t7900-subtree.sh | 71 ++++++++++++++++++++++++++++++
>   2 files changed, 99 insertions(+), 8 deletions(-)
> 
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 5dab3f506c..ad9b9b0191 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -783,24 +783,44 @@ ensure_clean () {
>   # Usage: ensure_valid_ref_format REF
>   ensure_valid_ref_format () {
>   	assert test $# = 1
>   	git check-ref-format "refs/heads/$1" ||
>   		die "fatal: '$1' does not look like a ref"
>   }
>   
> -# 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
> +
> -	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"
>   	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..25be40e12b 100755
> --- a/contrib/subtree/t/t7900-subtree.sh
> +++ b/contrib/subtree/t/t7900-subtree.sh
> @@ -9,6 +9,9 @@ This test verifies the basic operation of the add, merge, split, pull,
>   and push subcommands of git subtree.
>   '
>   
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
>   TEST_DIRECTORY=$(pwd)/../../../t
>   . "$TEST_DIRECTORY"/test-lib.sh
>   
> @@ -67,6 +70,33 @@ 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 switch --orphan "$orphan" &&
> +		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 &&
> @@ -425,6 +455,47 @@ 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 &&
> +			test_create_subtree_add \
> +				. mksubtree subA/subB file2 ${is_squashed:+--squash} &&
> +			test_path_is_file subA/file1.t &&
> +			test_path_is_file subA/subB/file2.t &&
> +			git subtree split --prefix=subA --branch=bsplit &&
> +			git checkout bsplit &&
> +			test_path_is_file file1.t &&
> +			test_path_is_file 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_path_is_file subA/subB/file3.t &&
> +			git subtree split --prefix=subA --branch=bsplit &&
> +			git checkout bsplit &&
> +			test_path_is_file file1.t &&
> +			test_path_is_file subB/file2.t &&
> +			test_path_is_file 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