Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees
From: Phillip Wood <hidden>
Date: 2025-09-08 15:21:45
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}"?
quoted hunk ↗ jump to hunk
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 PARENTSdiff --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
quoted hunk ↗ jump to hunk
+ 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