Re: [PATCH] contrib/subtree: fix split with squashed subtrees
From: Phillip Wood <hidden>
Date: 2025-09-01 13:54:15
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
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
then return 0 fi 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 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