From: Phillip Wood <phillip.wood123@gmail•com>
To: Colin Stagner <ask+git@howdoi•land>,
git@vger•kernel.org, phillip.wood@dunelm•org.uk
Cc: Zach FettersMoore <zach.fetters@apollographql•com>,
Christian Couder <chriscool@tuxfamily•org>,
Patrik Weiskircher <patrik@pspdfkit•com>
Subject: Re: [PATCH v3] contrib/subtree: fix split with squashed subtrees
Date: Wed, 10 Sep 2025 10:39:00 +0100 [thread overview]
Message-ID: <dd71ebee-8629-43c3-aa2a-40124400f262@gmail.com> (raw)
In-Reply-To: <20250910031124.1807856-1-ask+git@howdoi.land>
Hi Colin
Thanks for working on this. I'm not particularly familiar with
git-subtree but as far as I can see this version looks good.
Thanks
Phillip
On 10/09/2025 04:11, 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 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
next prev parent reply other threads:[~2025-09-10 9:39 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-24 19:10 [PATCH] contrib/subtree: fix split with squashed subtrees Colin Stagner
2025-09-01 13:54 ` Phillip Wood
2025-09-01 20:43 ` Colin Stagner
2025-09-02 13:22 ` Phillip Wood
2025-09-02 14:57 ` Phillip Wood
2025-09-04 1:34 ` Colin Stagner
2025-09-05 2:27 ` [PATCH v2] " Colin Stagner
2025-09-08 15:21 ` Phillip Wood
2025-09-10 1:56 ` Colin Stagner
2025-09-10 2:02 ` Junio C Hamano
2025-09-10 3:00 ` Colin Stagner
2025-09-10 15:10 ` Junio C Hamano
2025-09-10 3:11 ` [PATCH v3] " Colin Stagner
2025-09-10 9:39 ` Phillip Wood [this message]
2025-09-11 16:01 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd71ebee-8629-43c3-aa2a-40124400f262@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=ask+git@howdoi$(echo .)land \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=patrik@pspdfkit$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=zach.fetters@apollographql$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox