{"thread":{"id":"64025","subject":"[PATCH] contrib/subtree: fix split with squashed subtrees","startedAt":"2025-08-24T19:11:59Z","lastAt":"2025-09-11T16:01:36Z","messageCount":15,"participants":["Colin Stagner","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"524849","messageId":"20250824191048.1938340-1-ask+git@howdoi.land","threadId":"64025","inReplyTo":null,"subject":"[PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-08-24T19:10:48Z","receivedAt":"2025-08-24T19:11:59Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"98ba49ccc2 (subtree: fix split processing with multiple subtrees\npresent, 2023-12-01) increases the performance of\n\n    git subtree split --prefix=subA\n\nby ignoring subtree merges which are outside of `subA/`. It also\nintroduces a regression. Subtree merges that should be retained\nare incorrectly ignored if they:\n\n1. are nested under `subA/`; and\n2. are merged with `--squash`.\n\nFor example, a subtree merged like:\n\n    git subtree merge --squash --prefix=subA/subB \"$rev\"\n    #                 ^^^^^^^^          ^^^^\n\nis erroneously ignored during a split of `subA`. This causes\nmissing tree files and different commit hashes starting in\ngit v2.44.0-rc0.\n\nThe method:\n\n    should_ignore_subtree_split_commit REV\n\nshould test only if REV is a subtree commit, but the combination of\n\n    git log -1 --grep=...\n\nactually searches all *parent* commits until a `--grep` match is\ndiscovered. Limit these checks to the current REV only.\n\nTests now cover nested subtrees.\n\nSigned-off-by: Colin Stagner <ask+git@howdoi.land>\n---\n\nNotes:\n    The unit test changes in t7900-subtree.sh demonstrate the bug.\n    \n    See also:\n    \n    * <pull.1587.v5.git.1701206267300.gitgitgadget@gmail.com>\n    * <c9e8f54f-2594-4092-ae41-f1da73e97f6e@howdoi.land>\n\n contrib/subtree/git-subtree.sh     |  6 +--\n contrib/subtree/t/t7900-subtree.sh | 70 ++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 3fddba797c..139049351d 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -788,17 +788,17 @@ ensure_valid_ref_format () {\n # Usage: check if a commit from another subtree should be\n # ignored from processing for splits\n should_ignore_subtree_split_commit () {\n \tassert test $# = 1\n \tlocal rev=\"$1\"\n-\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n+\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" \"$rev^!\")\"\n \tthen\n-\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n-\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n+\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \"$rev^!\")\" &&\n+\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" \"$rev^!\")\"\n \t\tthen\n \t\t\treturn 0\n \t\tfi\n \tfi\n \treturn 1\n }\n \n # Usage: process_split_commit REV PARENTS\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 3edbb33af4..1ddc213621 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -68,6 +68,34 @@ test_create_pre2_32_repo () {\n \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n }\n \n+# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n+#\n+# Create a simple subtree on a new branch named ORPHAN in REPO.\n+# The subtree is then merged into the current branch of REPO,\n+# under PREFIX. The generated subtree has has one commit\n+# with subject and tag FILENAME with a single file \"FILENAME.t\"\n+#\n+# When this method returns:\n+# - the current branch of REPO will have file PREFIX/FILENAME.t\n+# - REPO will have a branch named ORPHAN with subtree history\n+#\n+# additional arguments are forwarded to \"subtree add\"\n+test_create_subtree_add () {\n+\t(\n+\t\tcd \"$1\" &&\n+\t\torphan=\"$2\" &&\n+\t\tprefix=\"$3\" &&\n+\t\tfilename=\"$4\" &&\n+\t\tshift 4 &&\n+\t\tlast=\"$(git branch --show-current)\" &&\n+\t\tgit checkout --orphan \"$orphan\" &&\n+\t\tgit rm -rf . &&\n+\t\ttest_commit \"$filename\" &&\n+\t\tgit checkout \"$last\" &&\n+\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n+\t)\n+}\n+\n test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n@@ -426,6 +454,48 @@ test_expect_success 'split with multiple subtrees' '\n \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n '\n \n+# When subtree split-ing a directory that has other subtree\n+# *merges* underneath it, the split must include those subtrees.\n+# This test creates a nested subtree, `subA/subB`, and tests\n+# that the tree is correct after a subtree split of `subA/`.\n+# The test covers:\n+# - An initial `subtree add`; and\n+# - A follow-up `subtree merge`\n+# both with and without `--squashed`.\n+for is_squashed in '' 'y';\n+do\n+\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n+\t\tsubtree_test_create_repo \"$test_count\" &&\n+\t\t(\n+\t\t\tcd \"$test_count\" &&\n+\t\t\tmkdir subA &&\n+\t\t\ttest_commit subA/file1 &&\n+\t\t\tgit branch -m main &&\n+\t\t\ttest_create_subtree_add \\\n+\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n+\t\t\ttest -e subA/file1.t &&\n+\t\t\ttest -e subA/subB/file2.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest -e file1.t &&\n+\t\t\ttest -e subB/file2.t &&\n+\t\t\tgit checkout mksubtree &&\n+\t\t\tgit branch -D bsplit &&\n+\t\t\ttest_commit file3 &&\n+\t\t\tgit checkout main &&\n+\t\t\tgit subtree merge \\\n+\t\t\t\t${is_squashed:+--squash} \\\n+\t\t\t\t--prefix=subA/subB mksubtree &&\n+\t\t\ttest -e subA/subB/file3.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest -e file1.t &&\n+\t\t\ttest -e subB/file2.t &&\n+\t\t\ttest -e subB/file3.t\n+\t\t)\n+\t'\n+done\n+\n test_expect_success 'split sub dir/ with --rejoin from scratch' '\n \tsubtree_test_create_repo \"$test_count\" &&\n \ttest_create_commit \"$test_count\" main1 &&\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \n2.43.0\n\n"},{"id":"525278","messageId":"00e76b7e-ce4f-44d9-acd9-466c6b14f41b@gmail.com","threadId":"64025","inReplyTo":"20250824191048.1938340-1-ask+git@howdoi.land","subject":"Re: [PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-01T13:54:11Z","receivedAt":"2025-09-01T13:54:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Colin\n\nOn 24/08/2025 20:10, Colin Stagner wrote:\n> 98ba49ccc2 (subtree: fix split processing with multiple subtrees\n> present, 2023-12-01) increases the performance of\n> \n>      git subtree split --prefix=subA\n> \n> by ignoring subtree merges which are outside of `subA/`. It also\n> introduces a regression. Subtree merges that should be retained\n> are incorrectly ignored if they:\n> \n> 1. are nested under `subA/`; and\n> 2. are merged with `--squash`.\n> \n> For example, a subtree merged like:\n> \n>      git subtree merge --squash --prefix=subA/subB \"$rev\"\n>      #                 ^^^^^^^^          ^^^^\n> \n> is erroneously ignored during a split of `subA`. This causes\n> missing tree files and different commit hashes starting in\n> git v2.44.0-rc0.\n> \n> The method:\n> \n>      should_ignore_subtree_split_commit REV\n> \n> should test only if REV is a subtree commit, but the combination of\n> \n>      git log -1 --grep=...\n> \n> actually searches all *parent* commits until a `--grep` match is\n> discovered. Limit these checks to the current REV only.\n\nThanks for the clear explanation of the problem and the proposed solution.\n\n> Tests now cover nested subtrees.\n\nGreat\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 3fddba797c..139049351d 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -788,17 +788,17 @@ ensure_valid_ref_format () {\n>   # Usage: check if a commit from another subtree should be\n>   # ignored from processing for splits\n>   should_ignore_subtree_split_commit () {\n>   \tassert test $# = 1\n>   \tlocal rev=\"$1\"\n> -\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n> +\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" \"$rev^!\")\"\n\nThis makes sense as we only want to grep the current commit. We could \ndrop the \"-1\" as we're only considering a single commit.\n\n>   \tthen\n> -\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n> -\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n> +\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \"$rev^!\")\" &&\n> +\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" \"$rev^!\")\"\n\nI'm less sure about this change. Is the second test checking making sure \nwe don't prune this commit if it has an ancestor that is a subtree merge \nfor the subtree we're interested in? It would be very helpful if Zach \ncould comment on what was intended here.\n\nIf it turns out that all three tests only want to consider a single \ncommit then it would be be more efficient to run a single git command \nand check the output with something like\n\n\tgit show -s \n--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline' $rev \n| while read trailer\n\t\tdo\n\t\t\t# check trailers here using case \"$trailer\"\n\t\tdone\n\nThanks\n\nPhillip\n\n>   \t\tthen\n>   \t\t\treturn 0\n>   \t\tfi\n>   \tfi\n>   \treturn 1\n>   }\n>   \n>   # Usage: process_split_commit REV PARENTS\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 3edbb33af4..1ddc213621 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -68,6 +68,34 @@ test_create_pre2_32_repo () {\n>   \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n>   }\n>   \n> +# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n> +#\n> +# Create a simple subtree on a new branch named ORPHAN in REPO.\n> +# The subtree is then merged into the current branch of REPO,\n> +# under PREFIX. The generated subtree has has one commit\n> +# with subject and tag FILENAME with a single file \"FILENAME.t\"\n> +#\n> +# When this method returns:\n> +# - the current branch of REPO will have file PREFIX/FILENAME.t\n> +# - REPO will have a branch named ORPHAN with subtree history\n> +#\n> +# additional arguments are forwarded to \"subtree add\"\n> +test_create_subtree_add () {\n> +\t(\n> +\t\tcd \"$1\" &&\n> +\t\torphan=\"$2\" &&\n> +\t\tprefix=\"$3\" &&\n> +\t\tfilename=\"$4\" &&\n> +\t\tshift 4 &&\n> +\t\tlast=\"$(git branch --show-current)\" &&\n> +\t\tgit checkout --orphan \"$orphan\" &&\n> +\t\tgit rm -rf . &&\n> +\t\ttest_commit \"$filename\" &&\n> +\t\tgit checkout \"$last\" &&\n> +\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n> +\t)\n> +}\n> +\n>   test_expect_success 'shows short help text for -h' '\n>   \ttest_expect_code 129 git subtree -h >out 2>err &&\n>   \ttest_must_be_empty err &&\n> @@ -426,6 +454,48 @@ test_expect_success 'split with multiple subtrees' '\n>   \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n>   '\n>   \n> +# When subtree split-ing a directory that has other subtree\n> +# *merges* underneath it, the split must include those subtrees.\n> +# This test creates a nested subtree, `subA/subB`, and tests\n> +# that the tree is correct after a subtree split of `subA/`.\n> +# The test covers:\n> +# - An initial `subtree add`; and\n> +# - A follow-up `subtree merge`\n> +# both with and without `--squashed`.\n> +for is_squashed in '' 'y';\n> +do\n> +\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n> +\t\tsubtree_test_create_repo \"$test_count\" &&\n> +\t\t(\n> +\t\t\tcd \"$test_count\" &&\n> +\t\t\tmkdir subA &&\n> +\t\t\ttest_commit subA/file1 &&\n> +\t\t\tgit branch -m main &&\n> +\t\t\ttest_create_subtree_add \\\n> +\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n> +\t\t\ttest -e subA/file1.t &&\n> +\t\t\ttest -e subA/subB/file2.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest -e file1.t &&\n> +\t\t\ttest -e subB/file2.t &&\n> +\t\t\tgit checkout mksubtree &&\n> +\t\t\tgit branch -D bsplit &&\n> +\t\t\ttest_commit file3 &&\n> +\t\t\tgit checkout main &&\n> +\t\t\tgit subtree merge \\\n> +\t\t\t\t${is_squashed:+--squash} \\\n> +\t\t\t\t--prefix=subA/subB mksubtree &&\n> +\t\t\ttest -e subA/subB/file3.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest -e file1.t &&\n> +\t\t\ttest -e subB/file2.t &&\n> +\t\t\ttest -e subB/file3.t\n> +\t\t)\n> +\t'\n> +done\n> +\n>   test_expect_success 'split sub dir/ with --rejoin from scratch' '\n>   \tsubtree_test_create_repo \"$test_count\" &&\n>   \ttest_create_commit \"$test_count\" main1 &&\n> \n> base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n\n"},{"id":"525289","messageId":"ee480c22-0dd3-4c45-a2bd-838c238f1d55@howdoi.land","threadId":"64025","inReplyTo":"00e76b7e-ce4f-44d9-acd9-466c6b14f41b@gmail.com","subject":"Re: [PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-01T20:43:19Z","receivedAt":"2025-09-01T21:38:17Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"On 9/1/25 08:54, Phillip Wood wrote:\n\nColin Stagner <ask+git@howdoi.land> writes:\n\n>> -    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n>> +    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" \"$rev^!\")\"\n> \n> We could drop the \"-1\" as we're only considering a single commit.\n\nConcur.\n\n>> -        if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \n>> $rev)\" &&\n>> -            test -z \"$(git log -1 --grep=\"git-subtree-dir: \n>> $arg_prefix$\" $rev)\"\n>> +        if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \n>> \"$rev^!\")\" &&\n>> +            test -z \"$(git log -1 --grep=\"git-subtree-dir: \n>> $arg_prefix$\" \"$rev^!\")\"\n>\n> I'm less sure about this change. Is the second test checking\n> making sure we don't prune this commit if it has an ancestor\n> that is a subtree merge for the subtree we're interested in?\n\nThe outer loop in git-subtree.sh:983 appears to iterate from the root \ncommit forwards… and not from the HEAD backwards.\n\n     git rev-list --topo-order --reverse --parents $rev $unrevs\n     #                         ^^^^^^^^^\n\nSince the iteration is ancestor-first, I'm having difficulty seeing why \n`should_ignore_subtree_split_commit()` would want to do an ancestor \ntraversal at all. It already sees the commits ancestor-first. But there \ncould be a reason that I don't know.\n\n\nHere is a more long-winded breakdown of these tests. From what I can \ndetermine:\n\n     test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \"$rev\")\n\nexcludes squashed commits created from\n\n     git subtree merge --prefix subM --squash srcBranch\n\nThe --squash creates two commits:\n\n1. A single-parent \"Squashed 'subM/' content from\", which\n    squashes the changes from srcBranch. This commit's tree\n    is like the one on srcBranch. It does not have the `subM/`\n    prefix.\n\n2. A merge commit which rewrites the tree in (1) to add\n    the `subM/` leading directory, then merge it with the\n    current branch. The merge commit doesn't have any\n    `git-subtree:` trailers.\n\nWe must exclude (1) since the trees aren't actually compatible with \nHEAD. (They don't have the `subM` prefix). We must keep (2). The above \n`test -z` appears to do this.\n\n\nI am *much* less certain about the second test:\n\n    test -z \"$(git log -1 \\\n               --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n\nI think this was intended to keep the mainline portion from a previous \n`git subtree split --rejoin`. But if I remove this `test -z`, all the \nunit tests still pass—including mine. There may not be any test coverage \nfor this line. I will probably omit this `test` from v2.\n\n\n> It would be very helpful if Zach could comment on what was intended here.\n\nYes, this would aid my understanding a lot.\n\n\n> If it turns out that all three tests only want to consider a single \n> commit then it would be be more efficient to run a single git command \n> and check the output with something like\n> \n>      git show -s --format='%(trailers:key=git-subtree-dir,key=git- \n> subtree-mainline' $rev | while read trailer\n>          do\n>              # check trailers here using case \"$trailer\"\n>          done\n\nThis is a cleaner approach, and I'll explore it for v2. Any objection to \nlong options like `--no-patch` instead of `-s`? I find these are better \nfor scripts since there's less hunting around in man pages.\n\nThanks for your review,\n\nColin\n\n"},{"id":"525341","messageId":"773ed81e-34b4-4116-88de-7e4307b6c679@gmail.com","threadId":"64025","inReplyTo":"ee480c22-0dd3-4c45-a2bd-838c238f1d55@howdoi.land","subject":"Re: [PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-02T13:22:34Z","receivedAt":"2025-09-02T13:22:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/09/2025 21:43, Colin Stagner wrote:\n> On 9/1/25 08:54, Phillip Wood wrote:\n> Colin Stagner <ask+git@howdoi.land> writes:\n>>> -    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n>>> +    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" \"$rev^!\")\"\n>>\n>> We could drop the \"-1\" as we're only considering a single commit.\n> \n> Concur.\n> \n>>> -        if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \n>>> $rev)\" &&\n>>> -            test -z \"$(git log -1 --grep=\"git-subtree-dir: \n>>> $arg_prefix$\" $rev)\"\n>>> +        if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \n>>> \"$rev^!\")\" &&\n>>> +            test -z \"$(git log -1 --grep=\"git-subtree-dir: \n>>> $arg_prefix$\" \"$rev^!\")\"\n>>\n>> I'm less sure about this change. Is the second test checking\n>> making sure we don't prune this commit if it has an ancestor\n>> that is a subtree merge for the subtree we're interested in?\n> \n> The outer loop in git-subtree.sh:983 appears to iterate from the root \n> commit forwards… and not from the HEAD backwards.\n> \n>      git rev-list --topo-order --reverse --parents $rev $unrevs\n>      #                         ^^^^^^^^^\n> \n> Since the iteration is ancestor-first, I'm having difficulty seeing why \n> `should_ignore_subtree_split_commit()` would want to do an ancestor \n> traversal at all. It already sees the commits ancestor-first. But there \n> could be a reason that I don't know.\n\nAh, I was only looking at this patch, not how it was called. That begs \nthe question \"what's the point of these checks if we've already visited \nall the ancestors anyway\". I think the answer is that it is pruning the \nrecursion that happens in check_parent() and checking the commits that \ncome from that rev-list command is pointless. The regression test \nintroduced with this function only looks at $extracount which comes from \nthe recursion. I haven't looked too closely but it would be nice if we \ncould move this check so it is only run when check_parents() is recursing.\n\n> Here is a more long-winded breakdown of these tests. From what I can \n> determine:\n> \n>      test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" \"$rev\")\n> \n> excludes squashed commits created from\n> \n>      git subtree merge --prefix subM --squash srcBranch\n > > The --squash creates two commits:\n> \n> 1. A single-parent \"Squashed 'subM/' content from\", which\n>     squashes the changes from srcBranch. This commit's tree\n>     is like the one on srcBranch. It does not have the `subM/`\n>     prefix.\nYes, and the commit message comes from squash_msg() which adds the \n\"git-subtree-dir:\" and \"git-subtree-split:\" trailers but not \n\"git-subtree-mainline:\".\n\n> 2. A merge commit which rewrites the tree in (1) to add\n>     the `subM/` leading directory, then merge it with the\n>     current branch. The merge commit doesn't have any\n>     `git-subtree:` trailers.\n\nIndeed. Running\n\n\tgit subtree split --squash --rejoin --prefix subM\n\nwill create a squash commit as above and a merge with a commit message \ncreated by rejoin_msg() and contains the \"git-subtree-dir:\", \n\"git-subtree-split:\" and \"git-subtree-mainline:\" trailers.\n\n> We must exclude (1) since the trees aren't actually compatible with \n> HEAD. (They don't have the `subM` prefix). We must keep (2). The above \n> `test -z` appears to do this.\n\nSo we'll exclude squashed merges but not squashed splits? I think you're \nright that we don't want \"git log\" to walk the history here.\n\n> I am *much* less certain about the second test:\n> \n>     test -z \"$(git log -1 \\\n>                --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n> \n> I think this was intended to keep the mainline portion from a previous \n> `git subtree split --rejoin`. But if I remove this `test -z`, all the \n> unit tests still pass—including mine. There may not be any test coverage \n> for this line. I will probably omit this `test` from v2.\n\nI'm not very familiar with git-subtree but I thought this was ensuring \nthat we did not exclude the ancestors of a squash or split that involves \nthe subtree that we're interested in. I wouldn't be surprised if the \ntest coverage was lacking.\n>> It would be very helpful if Zach could comment on what was intended here.\n> \n> Yes, this would aid my understanding a lot.\n\nand mine too.\n\n>> If it turns out that all three tests only want to consider a single \n>> commit then it would be be more efficient to run a single git command \n>> and check the output with something like\n>>\n>>      git show -s --format='%(trailers:key=git-subtree-dir,key=git- \n>> subtree-mainline' $rev | while read trailer\n>>          do\n>>              # check trailers here using case \"$trailer\"\n>>          done\n> \n> This is a cleaner approach, and I'll explore it for v2. Any objection to \n> long options like `--no-patch` instead of `-s`? I find these are better \n> for scripts since there's less hunting around in man pages.\n\nSure, I used '-s' out of habit but '--no-patch' would be clearer \n(especially as I can't see any link between the short and long option \nnames in this case)\n\nThanks\n\nPhillip\n\n"},{"id":"525344","messageId":"62b50f7e-7ee3-420b-9de3-6d9df611b6b6@gmail.com","threadId":"64025","inReplyTo":"773ed81e-34b4-4116-88de-7e4307b6c679@gmail.com","subject":"Re: [PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-02T14:57:03Z","receivedAt":"2025-09-02T14:57:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 02/09/2025 14:22, Phillip Wood wrote:\n> On 01/09/2025 21:43, Colin Stagner wrote:\n>> On 9/1/25 08:54, Phillip Wood wrote:\n>> Colin Stagner <ask+git@howdoi.land> writes:\n>>\n>> The outer loop in git-subtree.sh:983 appears to iterate from the root \n>> commit forwards… and not from the HEAD backwards.\n>>\n>>      git rev-list --topo-order --reverse --parents $rev $unrevs\n>>      #                         ^^^^^^^^^\n>>\n>> Since the iteration is ancestor-first, I'm having difficulty seeing \n>> why `should_ignore_subtree_split_commit()` would want to do an \n>> ancestor traversal at all. It already sees the commits ancestor-first. \n>> But there could be a reason that I don't know.\n> \n> Ah, I was only looking at this patch, not how it was called. That begs \n> the question \"what's the point of these checks if we've already visited \n> all the ancestors anyway\". I think the answer is that it is pruning the \n> recursion that happens in check_parent() and checking the commits that \n> come from that rev-list command is pointless. The regression test \n> introduced with this function only looks at $extracount which comes from \n> the recursion. I haven't looked too closely but it would be nice if we \n> could move this check so it is only run when check_parents() is recursing.\n\nSorry that's not quite right. check_parents() recurses into \nprocess_split_commit() rather than the loop that call \nshould_ignore_subtree_split_commit(). I think what this check does do is \nprune some parents which stops check_parents() from recursing into other \nsubtrees so the check is in the right place.\n\nThanks\n\nPhillip\n\n"},{"id":"525477","messageId":"b8bf66c1-39c4-419d-ac78-e5f847d9ff90@howdoi.land","threadId":"64025","inReplyTo":"62b50f7e-7ee3-420b-9de3-6d9df611b6b6@gmail.com","subject":"Re: [PATCH] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-04T01:34:23Z","receivedAt":"2025-09-04T02:10:18Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"On 9/2/25 09:57, Phillip Wood wrote:\n> On 01/09/2025 21:43, Colin Stagner wrote:\n>>\n>> The outer loop in git-subtree.sh:983 appears to iterate from the root \n>> commit forwards… and not from the HEAD backwards.\n>>\n>>      git rev-list --topo-order --reverse --parents $rev $unrevs\n>>      #                         ^^^^^^^^^\n>>\n>> Since the iteration is ancestor-first, I'm having difficulty seeing \n>> why `should_ignore_subtree_split_commit()` would want to do an \n>> ancestor traversal at all. It already sees the commits ancestor- \n>> first.\n> check_parents() recurses into process_split_commit() rather than the\n> loop that call should_ignore_subtree_split_commit(). I think what this\n> check does do is prune some parents which stops check_parents() from\n> recursing into other subtrees so the check is in the right place.\n\nI agree. In the original patch [1], Zach indicated that the check \nresults in a significant speedup for rejoin-heavy repos. The check is \nclearly doing something.\n\nPerformance improvements may be possible. Instead of looking at commits \none at a time, this operation might be faster as part of a one-shot \nHEAD-to-root traversal:\n\n    git log --grep 'for stuff' --format='%(trailers:...)' $unrev..HEAD\n\nCommits that are deemed \"uninteresting\" or unnecessary could then be \nprovided, in bulk, as negative refs to the `git rev-list` traversal.\n\nBut my plan is to make the smallest and most portable maint-2.44 bugfix. \nI think that non-essential performance changes are a task for later.\n\n\n>> I am *much* less certain about the second test:\n>> \n>>      test -z \"$(git log -1 \\\n>>                 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n>> \n>> If I remove this `test -z`, all the unit tests still pass—including mine. There may not be any test coverage for this line.\n> I'm not very familiar with git-subtree but I thought this was ensuring \n> that we did not exclude the ancestors of a squash or split that involves \n> the subtree that we're interested in.\n\nIt does, but I am still having problems finding commits that actually \ntrigger it.\n\nIt appears that `find_existing_splits()` in git-subtree.sh:459 filters \nout the commits that the `test -z` I quoted above would otherwise \ndetect. `find_existing_splits()` searches for a previous --rejoin commit \nto use as an `$unrev` stopping point for the rev-walk. It searches for \ncommits matching\n\n     git log --grep=\"^git-subtree-dir: $dir/*\\$\"\n\nin combination with `git-subtree-mainline:`.\n\nThis is essentially the same test as in \n`should_ignore_subtree_split_commit()`.\n\nIn --ignore-joins mode, `find_existing_splits()` looks for different \ncommits. I experimented a bit with adding --ignore-joins to some of the \nexisting unit tests, but I still could not find any instance where this \n`test -z` makes a difference.\n\nThat said... I am inclined to keep this second test. The bug I am \npatching is the result of an overzealous prune. The last thing I want to \ndo is to inadvertently prune commits we need for the sake of a \nperformance boost.\n\n\n> I wouldn't be surprised if the test coverage was lacking.\n\nThere don't appear to be any tests at all for --ignore-joins, aside from \noption parsing.\n\n\n[1]: 98ba49ccc2 (subtree: fix split processing with multiple subtrees \npresent, 2023-12-01)\n\n"},{"id":"525584","messageId":"20250905022728.940664-1-ask+git@howdoi.land","threadId":"64025","inReplyTo":"20250824191048.1938340-1-ask+git@howdoi.land","subject":"[PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-05T02:27:28Z","receivedAt":"2025-09-05T02:28:49Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"98ba49ccc2 (subtree: fix split processing with multiple subtrees\npresent, 2023-12-01) increases the performance of\n\n    git subtree split --prefix=subA\n\nby ignoring subtree merges which are outside of `subA/`. It also\nintroduces a regression. Subtree merges that should be retained\nare incorrectly ignored if they:\n\n1. are nested under `subA/`; and\n2. are merged with `--squash`.\n\nFor example, a subtree merged like:\n\n    git subtree merge --squash --prefix=subA/subB \"$rev\"\n    #                 ^^^^^^^^          ^^^^\n\nis erroneously ignored during a split of `subA`. This causes\nmissing tree files and different commit hashes starting in\ngit v2.44.0-rc0.\n\nThe method:\n\n    should_ignore_subtree_split_commit REV\n\nshould test only a single commit REV, but the combination of\n\n    git log -1 --grep=...\n\nactually searches all *parent* commits until a `--grep` match is\ndiscovered.\n\nRewrite this method to test only one REV at a time. Extract commit\ninformation with a single `git` call as opposed to three. The\n`test` conditions for rejecting a commit remain unchanged.\n\nUnit tests now cover nested subtrees.\n\nSigned-off-by: Colin Stagner <ask+git@howdoi.land>\n---\n\nNotes:\n    This bugfix patch is intended for maint-2.44 and up.\n    \n    v2 rewrites `should_ignore_subtree_split_commit()` completely. In\n    addition to the review comments, v2 also:\n    \n    * adds `--no-show-signature` to align with\n      8841b5222c (subtree: fix add and pull for GPG-signed commits,\n      2018-02-23)\n    \n    * removes use of `local` from `should_ignore_subtree_split_commit`.\n      `local` is not part of POSIX sh. Other uses of `local` remain in\n      untouched code.\n    \n    * unit tests are unchanged since v1.\n\n contrib/subtree/git-subtree.sh     | 36 +++++++++++----\n contrib/subtree/t/t7900-subtree.sh | 70 ++++++++++++++++++++++++++++++\n 2 files changed, 98 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 5dab3f506c..c3cd60d341 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -783,24 +783,44 @@ ensure_clean () {\n # Usage: ensure_valid_ref_format REF\n ensure_valid_ref_format () {\n \tassert test $# = 1\n \tgit check-ref-format \"refs/heads/$1\" ||\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n-# Usage: check if a commit from another subtree should be\n+# Usage: should_ignore_subtree_split_commit REV\n+#\n+# Check if REV is a commit from another subtree and should be\n # ignored from processing for splits\n should_ignore_subtree_split_commit () {\n \tassert test $# = 1\n-\tlocal rev=\"$1\"\n+\n+\tgit show \\\n+\t\t--no-patch \\\n+\t\t--no-show-signature \\\n+\t\t--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \\\n+\t\t\"$1\" |\n+\t(\n+\thave_mainline=\n+\tsubtree_dir=\n+\n+\twhile read -r trailer val\n+\tdo\n+\t\tcase \"$trailer\" in\n+\t\t(git-subtree-dir:)\n+\t\t\tsubtree_dir=\"${val%/}\" ;;\n+\t\t(git-subtree-mainline:)\n+\t\t\thave_mainline=y ;;\n+\t\tesac\n+\tdone\n+\n-\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n+\tif test -n \"${subtree_dir:-}\" &&\n+\t\ttest -z \"${have_mainline:-}\" &&\n+\t\ttest \"${subtree_dir}\" != \"$arg_prefix\"\n \tthen\n-\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n-\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n-\t\tthen\n-\t\t\treturn 0\n-\t\tfi\n+\t\treturn 0\n \tfi\n \treturn 1\n+\t)\n }\n \n # Usage: process_split_commit REV PARENTS\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex ca4df5be83..8bd45e7be7 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -67,6 +67,34 @@ test_create_pre2_32_repo () {\n \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n }\n \n+# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n+#\n+# Create a simple subtree on a new branch named ORPHAN in REPO.\n+# The subtree is then merged into the current branch of REPO,\n+# under PREFIX. The generated subtree has has one commit\n+# with subject and tag FILENAME with a single file \"FILENAME.t\"\n+#\n+# When this method returns:\n+# - the current branch of REPO will have file PREFIX/FILENAME.t\n+# - REPO will have a branch named ORPHAN with subtree history\n+#\n+# additional arguments are forwarded to \"subtree add\"\n+test_create_subtree_add () {\n+\t(\n+\t\tcd \"$1\" &&\n+\t\torphan=\"$2\" &&\n+\t\tprefix=\"$3\" &&\n+\t\tfilename=\"$4\" &&\n+\t\tshift 4 &&\n+\t\tlast=\"$(git branch --show-current)\" &&\n+\t\tgit checkout --orphan \"$orphan\" &&\n+\t\tgit rm -rf . &&\n+\t\ttest_commit \"$filename\" &&\n+\t\tgit checkout \"$last\" &&\n+\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n+\t)\n+}\n+\n test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n@@ -425,6 +453,48 @@ test_expect_success 'split with multiple subtrees' '\n \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n '\n \n+# When subtree split-ing a directory that has other subtree\n+# *merges* underneath it, the split must include those subtrees.\n+# This test creates a nested subtree, `subA/subB`, and tests\n+# that the tree is correct after a subtree split of `subA/`.\n+# The test covers:\n+# - An initial `subtree add`; and\n+# - A follow-up `subtree merge`\n+# both with and without `--squashed`.\n+for is_squashed in '' 'y';\n+do\n+\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n+\t\tsubtree_test_create_repo \"$test_count\" &&\n+\t\t(\n+\t\t\tcd \"$test_count\" &&\n+\t\t\tmkdir subA &&\n+\t\t\ttest_commit subA/file1 &&\n+\t\t\tgit branch -m main &&\n+\t\t\ttest_create_subtree_add \\\n+\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n+\t\t\ttest -e subA/file1.t &&\n+\t\t\ttest -e subA/subB/file2.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest -e file1.t &&\n+\t\t\ttest -e subB/file2.t &&\n+\t\t\tgit checkout mksubtree &&\n+\t\t\tgit branch -D bsplit &&\n+\t\t\ttest_commit file3 &&\n+\t\t\tgit checkout main &&\n+\t\t\tgit subtree merge \\\n+\t\t\t\t${is_squashed:+--squash} \\\n+\t\t\t\t--prefix=subA/subB mksubtree &&\n+\t\t\ttest -e subA/subB/file3.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest -e file1.t &&\n+\t\t\ttest -e subB/file2.t &&\n+\t\t\ttest -e subB/file3.t\n+\t\t)\n+\t'\n+done\n+\n test_expect_success 'split sub dir/ with --rejoin from scratch' '\n \tsubtree_test_create_repo \"$test_count\" &&\n \ttest_create_commit \"$test_count\" main1 &&\n\nbase-commit: 09669c729af92144fde84e97d358759b5b42b555\n-- \n2.43.0\n\n"},{"id":"525832","messageId":"b78639ee-021d-49fc-8b8d-0140ed8fc010@gmail.com","threadId":"64025","inReplyTo":"20250905022728.940664-1-ask+git@howdoi.land","subject":"Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-08T15:21:38Z","receivedAt":"2025-09-08T15:21:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Colin\n\nThis is looking good. I've left a few comments below, especially on the \nnew test.\n\nOn 05/09/2025 03:27, Colin Stagner wrote:\n>   \n> -# Usage: check if a commit from another subtree should be\n> +# Usage: should_ignore_subtree_split_commit REV\n> +#\n> +# Check if REV is a commit from another subtree and should be\n>   # ignored from processing for splits\n>   should_ignore_subtree_split_commit () {\n>   \tassert test $# = 1\n> -\tlocal rev=\"$1\"\n> +\n> +\tgit show \\\n> +\t\t--no-patch \\\n> +\t\t--no-show-signature \\\n> +\t\t--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \\\n> +\t\t\"$1\" |\n> +\t(\n> +\thave_mainline=\n> +\tsubtree_dir=\n> +\n> +\twhile read -r trailer val\n> +\tdo\n> +\t\tcase \"$trailer\" in\n> +\t\t(git-subtree-dir:)\n> +\t\t\tsubtree_dir=\"${val%/}\" ;;\n> +\t\t(git-subtree-mainline:)\n> +\t\t\thave_mainline=y ;;\n> +\t\tesac\n> +\tdone\n\nThis looks good, we run git log once and then parse the trailers. We do \nnot use the optional '(' in case statements in our code though.\n\n> -\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n> +\tif test -n \"${subtree_dir:-}\" &&\n> +\t\ttest -z \"${have_mainline:-}\" &&\n> +\t\ttest \"${subtree_dir}\" != \"$arg_prefix\"\n\nIf we have a git-subtree-dir: trailer whose value does not match the \nsubtree we're interested in and there is no git-subtree-mainline: \ntrailer then we skip this commit - good. What's the idea behind using \n\"${var:-}\" rather than \"{var}\"?\n\n>   \tthen\n> -\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n> -\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n> -\t\tthen\n> -\t\t\treturn 0\n> -\t\tfi\n> +\t\treturn 0\n>   \tfi\n>   \treturn 1\n> +\t)\n>   }\n>   \n>   # Usage: process_split_commit REV PARENTS\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index ca4df5be83..8bd45e7be7 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -67,6 +67,34 @@ test_create_pre2_32_repo () {\n>   \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n>   }\n>   \n> +# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n> +#\n> +# Create a simple subtree on a new branch named ORPHAN in REPO.\n> +# The subtree is then merged into the current branch of REPO,\n> +# under PREFIX. The generated subtree has has one commit\n> +# with subject and tag FILENAME with a single file \"FILENAME.t\"\n> +#\n> +# When this method returns:\n> +# - the current branch of REPO will have file PREFIX/FILENAME.t\n> +# - REPO will have a branch named ORPHAN with subtree history\n> +#\n> +# additional arguments are forwarded to \"subtree add\"\n> +test_create_subtree_add () {\n> +\t(\n> +\t\tcd \"$1\" &&\n> +\t\torphan=\"$2\" &&\n> +\t\tprefix=\"$3\" &&\n> +\t\tfilename=\"$4\" &&\n> +\t\tshift 4 &&\n> +\t\tlast=\"$(git branch --show-current)\" &&\n> +\t\tgit checkout --orphan \"$orphan\" &&\n> +\t\tgit rm -rf . &&\n\nIf you use \"git switch --orphan\" that clears the worktree for you\n\n> +\t\ttest_commit \"$filename\" &&\n> +\t\tgit checkout \"$last\" &&\n\nI think this could be \"git checkout @{-1}\" and then we'd avoid having to \nrun \"git branch\" above\n\n> +\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n> +\t)\n> +}\n> +\n>   test_expect_success 'shows short help text for -h' '\n>   \ttest_expect_code 129 git subtree -h >out 2>err &&\n>   \ttest_must_be_empty err &&\n> @@ -425,6 +453,48 @@ test_expect_success 'split with multiple subtrees' '\n>   \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n>   '\n>   \n> +# When subtree split-ing a directory that has other subtree\n> +# *merges* underneath it, the split must include those subtrees.\n> +# This test creates a nested subtree, `subA/subB`, and tests\n> +# that the tree is correct after a subtree split of `subA/`.\n> +# The test covers:\n> +# - An initial `subtree add`; and\n> +# - A follow-up `subtree merge`\n> +# both with and without `--squashed`.\n> +for is_squashed in '' 'y';\n\nno need for ';' at the end of the line\n\n> +do\n> +\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n> +\t\tsubtree_test_create_repo \"$test_count\" &&\n> +\t\t(\n> +\t\t\tcd \"$test_count\" &&\n> +\t\t\tmkdir subA &&\n> +\t\t\ttest_commit subA/file1 &&\n> +\t\t\tgit branch -m main &&\n\nFor tests that depend on the default branch name you can add\n\n\tGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n\texport GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\nto the start of the file before it sources test-lib.sh. Or we could \nchange subtree_create_repo() to pass all its arguments along to \ntest_create_repo() and use \"subtree_create_repo --initial-branch=main \n$test_count\"\n\n> +\t\t\ttest_create_subtree_add \\\n> +\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n> +\t\t\ttest -e subA/file1.t &&\n\nWe have test_path_is_file() for this which prints a useful diagnostic \nmessage it it fails\n\nThanks\n\nPhillip\n\n> +\t\t\ttest -e subA/subB/file2.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest -e file1.t &&\n> +\t\t\ttest -e subB/file2.t &&\n> +\t\t\tgit checkout mksubtree &&\n> +\t\t\tgit branch -D bsplit &&\n> +\t\t\ttest_commit file3 &&\n> +\t\t\tgit checkout main &&\n> +\t\t\tgit subtree merge \\\n> +\t\t\t\t${is_squashed:+--squash} \\\n> +\t\t\t\t--prefix=subA/subB mksubtree &&\n> +\t\t\ttest -e subA/subB/file3.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest -e file1.t &&\n> +\t\t\ttest -e subB/file2.t &&\n> +\t\t\ttest -e subB/file3.t\n> +\t\t)\n> +\t'\n> +done\n> +\n>   test_expect_success 'split sub dir/ with --rejoin from scratch' '\n>   \tsubtree_test_create_repo \"$test_count\" &&\n>   \ttest_create_commit \"$test_count\" main1 &&\n> \n> base-commit: 09669c729af92144fde84e97d358759b5b42b555\n\n"},{"id":"526001","messageId":"8d341a51-2135-4c62-9df1-5be351e73275@howdoi.land","threadId":"64025","inReplyTo":"b78639ee-021d-49fc-8b8d-0140ed8fc010@gmail.com","subject":"Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-10T01:56:07Z","receivedAt":"2025-09-10T01:56:15Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"Phillip,\n\nHello again! I have adopted your recommendations everywhere except for \n`git checkout @{-1}`. Details below.\n\nOn 9/8/25 10:21, Phillip Wood wrote:\n> On 05/09/2025 03:27, Colin Stagner wrote:\n\n>> +    while read -r trailer val\n>> +    do\n>> +        case \"$trailer\" in\n>> +        (git-subtree-dir:)\n>> +            subtree_dir=\"${val%/}\" ;;\n>> +        (git-subtree-mainline:)\n>> +            have_mainline=y ;;\n>> +        esac\n>> +    done\n> \n> We do not use the optional '(' in case statements\n\nWill fix in v3.\n\n> \n>> -    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n>> +    if test -n \"${subtree_dir:-}\" &&\n>> +        test -z \"${have_mainline:-}\" &&\n>> +        test \"${subtree_dir}\" != \"$arg_prefix\"\n> \n> What's the idea behind using \"${var:-}\" rather than \"{var}\"?\n\nI write a lot of shell scripts that run \"set -u\" (aka \"set -o nounset\"), \nso I do this a lot when testing for empty vars. In this case, it's not \nactually necessary since `have_mainline` is explicitly defined above. \nAnd we don't run `set -u` anyway.\n\nWill remove from v3.\n\n\n>> +test_create_subtree_add () {\n>> +    (\n>> +        cd \"$1\" &&\n>> +        orphan=\"$2\" &&\n>> +        prefix=\"$3\" &&\n>> +        filename=\"$4\" &&\n>> +        shift 4 &&\n>> +        last=\"$(git branch --show-current)\" &&\n>> +        git checkout --orphan \"$orphan\" &&\n>> +        git rm -rf . &&\n> \n> If you use \"git switch --orphan\" that clears the worktree for you\n\nVery useful. I'll start using it in v3.\n\n\n>> +        test_commit \"$filename\" &&\n>> +        git checkout \"$last\" &&\n> \n> I think this could be \"git checkout @{-1}\" and then we'd avoid having to \n> run \"git branch\" above\n\nI experimented with this, but I couldn't get it to work on git 2.44. \nAlthough the reflog shows the refs I expect, using\n\n     git switch '@{-1}'\n\ndies with\n\n     fatal: invalid reference: @{-1}\n\ncheckout doesn't work either. Perhaps there is something about --orphan \nthat is messing up the history.\n\nI could make `test_create_subtree_add` take a mainline branch name, \nbut... unless there's something unsound about v2, I think we should just \nkeep v2. `git branch --show-current` looks like well-defined porcelain.\n\nAny other ideas?\n\n\n>> +# The test covers:\n>> +# - An initial `subtree add`; and\n>> +# - A follow-up `subtree merge`\n>> +# both with and without `--squashed`.\n>> +for is_squashed in '' 'y';\n> \n> no need for ';' at the end of the line\n\nFixed for v3.\n\n>> +        subtree_test_create_repo \"$test_count\" &&\n>> +        (\n>> +            cd \"$test_count\" &&\n>> +            mkdir subA &&\n>> +            test_commit subA/file1 &&\n>> +            git branch -m main &&\n> \n> For tests that depend on the default branch name you can add\n> \n>      GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>      export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> to the start of the file before it sources test-lib.sh.\n\nGIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main looks very common, so I'll go \nwith that for v3.\n\n\n>> +            test_create_subtree_add \\\n>> +                . mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n>> +            test -e subA/file1.t &&\n> \n> We have test_path_is_file() for this which prints a useful diagnostic \n> message\n\nFixed all occurrences in v3.\n\n\nColin\n\n"},{"id":"526002","messageId":"xmqqbjnjt67y.fsf@gitster.g","threadId":"64025","inReplyTo":"8d341a51-2135-4c62-9df1-5be351e73275@howdoi.land","subject":"Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-10T02:02:41Z","receivedAt":"2025-09-10T02:02:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Stagner <ask+git@howdoi.land> writes:\n\n>>> -    if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n>>> +    if test -n \"${subtree_dir:-}\" &&\n>>> +        test -z \"${have_mainline:-}\" &&\n>>> +        test \"${subtree_dir}\" != \"$arg_prefix\"\n>> What's the idea behind using \"${var:-}\" rather than \"{var}\"?\n>\n> I write a lot of shell scripts that run \"set -u\" (aka \"set -o\n> nounset\"), so I do this a lot when testing for empty vars. In this\n> case, it's not actually necessary since `have_mainline` is explicitly\n> defined above. And we don't run `set -u` anyway.\n\nBesides, \"if test -n ${subtree_dir-}\" without colon would be the\nmore proper way for those who care about \"set -u\", wouldn't it?  It\nis not that you want to substitute with an empty string that comes\nbetween that \"-\" and \"}\" when subtree_dir is unset or set to empty.\nYou are preparing for the case where the variable is truly not set,\nand the variable being set to an empty string is not something you\nare worried about.  THe same for ${have_mainline:-}.\n\n>> If you use \"git switch --orphan\" that clears the worktree for you\n>\n> Very useful. I'll start using it in v3.\n\nExcellent suggestion.\n\n"},{"id":"526004","messageId":"641aaa9b-2b23-4faf-a13e-f6205e9ef5a2@howdoi.land","threadId":"64025","inReplyTo":"xmqqbjnjt67y.fsf@gitster.g","subject":"Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-10T03:00:18Z","receivedAt":"2025-09-10T03:00:42Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"On 9/9/25 21:02, Junio C Hamano wrote:\n> Besides, \"if test -n ${subtree_dir-}\" without colon would be the\n> more proper way for those who care about \"set -u\", wouldn't it?  It\n> is not that you want to substitute with an empty string that comes\n> between that \"-\" and \"}\" when subtree_dir is unset or set to empty.\n> You are preparing for the case where the variable is truly not set,\n> and the variable being set to an empty string is not something you\n> are worried about.\nYes, \"test -n ${subtree_dir-}\" is definitely the more correct expression.\n\nAt the very real risk of embarrassing myself in public today... in the \nparticular case of a \"test -n,\" is there actually an appreciable \ndifference? Either way, the output of the substitution is empty if the \ninput is empty or undefined. Here, \"test -n ${subtree_dir:-}\" is merely \nless efficient. Right?\n\nThe difference between \"${x:-}\" vs \"${x-}\" really starts to matter if \nyou want to permit the empty string (or not). It also matters if you \ncall a command that has side effects.\n\n(And in the context of this patch, neither are necessary.)\n\n"},{"id":"526005","messageId":"20250910031124.1807856-1-ask+git@howdoi.land","threadId":"64025","inReplyTo":"20250824191048.1938340-1-ask+git@howdoi.land","subject":"[PATCH v3] contrib/subtree: fix split with squashed subtrees","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-09-10T03:11:24Z","receivedAt":"2025-09-10T03:12:12Z","isPatch":true,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"98ba49ccc2 (subtree: fix split processing with multiple subtrees\npresent, 2023-12-01) increases the performance of\n\n    git subtree split --prefix=subA\n\nby ignoring subtree merges which are outside of `subA/`. It also\nintroduces a regression. Subtree merges that should be retained\nare incorrectly ignored if they:\n\n1. are nested under `subA/`; and\n2. are merged with `--squash`.\n\nFor example, a subtree merged like:\n\n    git subtree merge --squash --prefix=subA/subB \"$rev\"\n    #                 ^^^^^^^^          ^^^^\n\nis erroneously ignored during a split of `subA`. This causes\nmissing tree files and different commit hashes starting in\ngit v2.44.0-rc0.\n\nThe method:\n\n    should_ignore_subtree_split_commit REV\n\nshould test only a single commit REV, but the combination of\n\n    git log -1 --grep=...\n\nactually searches all *parent* commits until a `--grep` match is\ndiscovered.\n\nRewrite this method to test only one REV at a time. Extract commit\ninformation with a single `git` call as opposed to three. The\n`test` conditions for rejecting a commit remain unchanged.\n\nUnit tests now cover nested subtrees.\n\nSigned-off-by: Colin Stagner <ask+git@howdoi.land>\n---\n\nNotes:\n    This bugfix patch is intended for maint-2.44 and up.\n\n contrib/subtree/git-subtree.sh     | 36 +++++++++++----\n contrib/subtree/t/t7900-subtree.sh | 71 ++++++++++++++++++++++++++++++\n 2 files changed, 99 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 5dab3f506c..ad9b9b0191 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -783,24 +783,44 @@ ensure_clean () {\n # Usage: ensure_valid_ref_format REF\n ensure_valid_ref_format () {\n \tassert test $# = 1\n \tgit check-ref-format \"refs/heads/$1\" ||\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n-# Usage: check if a commit from another subtree should be\n+# Usage: should_ignore_subtree_split_commit REV\n+#\n+# Check if REV is a commit from another subtree and should be\n # ignored from processing for splits\n should_ignore_subtree_split_commit () {\n \tassert test $# = 1\n-\tlocal rev=\"$1\"\n+\n+\tgit show \\\n+\t\t--no-patch \\\n+\t\t--no-show-signature \\\n+\t\t--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \\\n+\t\t\"$1\" |\n+\t(\n+\thave_mainline=\n+\tsubtree_dir=\n+\n+\twhile read -r trailer val\n+\tdo\n+\t\tcase \"$trailer\" in\n+\t\tgit-subtree-dir:)\n+\t\t\tsubtree_dir=\"${val%/}\" ;;\n+\t\tgit-subtree-mainline:)\n+\t\t\thave_mainline=y ;;\n+\t\tesac\n+\tdone\n+\n-\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n+\tif test -n \"${subtree_dir}\" &&\n+\t\ttest -z \"${have_mainline}\" &&\n+\t\ttest \"${subtree_dir}\" != \"$arg_prefix\"\n \tthen\n-\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n-\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n-\t\tthen\n-\t\t\treturn 0\n-\t\tfi\n+\t\treturn 0\n \tfi\n \treturn 1\n+\t)\n }\n \n # Usage: process_split_commit REV PARENTS\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex ca4df5be83..25be40e12b 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -9,6 +9,9 @@ This test verifies the basic operation of the add, merge, split, pull,\n and push subcommands of git subtree.\n '\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_DIRECTORY=$(pwd)/../../../t\n . \"$TEST_DIRECTORY\"/test-lib.sh\n \n@@ -67,6 +70,33 @@ test_create_pre2_32_repo () {\n \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n }\n \n+# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n+#\n+# Create a simple subtree on a new branch named ORPHAN in REPO.\n+# The subtree is then merged into the current branch of REPO,\n+# under PREFIX. The generated subtree has has one commit\n+# with subject and tag FILENAME with a single file \"FILENAME.t\"\n+#\n+# When this method returns:\n+# - the current branch of REPO will have file PREFIX/FILENAME.t\n+# - REPO will have a branch named ORPHAN with subtree history\n+#\n+# additional arguments are forwarded to \"subtree add\"\n+test_create_subtree_add () {\n+\t(\n+\t\tcd \"$1\" &&\n+\t\torphan=\"$2\" &&\n+\t\tprefix=\"$3\" &&\n+\t\tfilename=\"$4\" &&\n+\t\tshift 4 &&\n+\t\tlast=\"$(git branch --show-current)\" &&\n+\t\tgit switch --orphan \"$orphan\" &&\n+\t\ttest_commit \"$filename\" &&\n+\t\tgit checkout \"$last\" &&\n+\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n+\t)\n+}\n+\n test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n@@ -425,6 +455,47 @@ test_expect_success 'split with multiple subtrees' '\n \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n '\n \n+# When subtree split-ing a directory that has other subtree\n+# *merges* underneath it, the split must include those subtrees.\n+# This test creates a nested subtree, `subA/subB`, and tests\n+# that the tree is correct after a subtree split of `subA/`.\n+# The test covers:\n+# - An initial `subtree add`; and\n+# - A follow-up `subtree merge`\n+# both with and without `--squashed`.\n+for is_squashed in '' 'y'\n+do\n+\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n+\t\tsubtree_test_create_repo \"$test_count\" &&\n+\t\t(\n+\t\t\tcd \"$test_count\" &&\n+\t\t\tmkdir subA &&\n+\t\t\ttest_commit subA/file1 &&\n+\t\t\ttest_create_subtree_add \\\n+\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n+\t\t\ttest_path_is_file subA/file1.t &&\n+\t\t\ttest_path_is_file subA/subB/file2.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest_path_is_file file1.t &&\n+\t\t\ttest_path_is_file subB/file2.t &&\n+\t\t\tgit checkout mksubtree &&\n+\t\t\tgit branch -D bsplit &&\n+\t\t\ttest_commit file3 &&\n+\t\t\tgit checkout main &&\n+\t\t\tgit subtree merge \\\n+\t\t\t\t${is_squashed:+--squash} \\\n+\t\t\t\t--prefix=subA/subB mksubtree &&\n+\t\t\ttest_path_is_file subA/subB/file3.t &&\n+\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n+\t\t\tgit checkout bsplit &&\n+\t\t\ttest_path_is_file file1.t &&\n+\t\t\ttest_path_is_file subB/file2.t &&\n+\t\t\ttest_path_is_file subB/file3.t\n+\t\t)\n+\t'\n+done\n+\n test_expect_success 'split sub dir/ with --rejoin from scratch' '\n \tsubtree_test_create_repo \"$test_count\" &&\n \ttest_create_commit \"$test_count\" main1 &&\n\nbase-commit: 09669c729af92144fde84e97d358759b5b42b555\n-- \n2.43.0\n\n"},{"id":"526019","messageId":"dd71ebee-8629-43c3-aa2a-40124400f262@gmail.com","threadId":"64025","inReplyTo":"20250910031124.1807856-1-ask+git@howdoi.land","subject":"Re: [PATCH v3] contrib/subtree: fix split with squashed subtrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-09-10T09:39:00Z","receivedAt":"2025-09-10T09:39:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Colin\n\nThanks for working on this. I'm not particularly familiar with \ngit-subtree but as far as I can see this version looks good.\n\nThanks\n\nPhillip\n\nOn 10/09/2025 04:11, Colin Stagner wrote:\n> 98ba49ccc2 (subtree: fix split processing with multiple subtrees\n> present, 2023-12-01) increases the performance of\n> \n>      git subtree split --prefix=subA\n> \n> by ignoring subtree merges which are outside of `subA/`. It also\n> introduces a regression. Subtree merges that should be retained\n> are incorrectly ignored if they:\n> \n> 1. are nested under `subA/`; and\n> 2. are merged with `--squash`.\n> \n> For example, a subtree merged like:\n> \n>      git subtree merge --squash --prefix=subA/subB \"$rev\"\n>      #                 ^^^^^^^^          ^^^^\n> \n> is erroneously ignored during a split of `subA`. This causes\n> missing tree files and different commit hashes starting in\n> git v2.44.0-rc0.\n> \n> The method:\n> \n>      should_ignore_subtree_split_commit REV\n> \n> should test only a single commit REV, but the combination of\n> \n>      git log -1 --grep=...\n> \n> actually searches all *parent* commits until a `--grep` match is\n> discovered.\n> \n> Rewrite this method to test only one REV at a time. Extract commit\n> information with a single `git` call as opposed to three. The\n> `test` conditions for rejecting a commit remain unchanged.\n> \n> Unit tests now cover nested subtrees.\n> \n> Signed-off-by: Colin Stagner <ask+git@howdoi.land>\n> ---\n> \n> Notes:\n>      This bugfix patch is intended for maint-2.44 and up.\n> \n>   contrib/subtree/git-subtree.sh     | 36 +++++++++++----\n>   contrib/subtree/t/t7900-subtree.sh | 71 ++++++++++++++++++++++++++++++\n>   2 files changed, 99 insertions(+), 8 deletions(-)\n> \n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 5dab3f506c..ad9b9b0191 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -783,24 +783,44 @@ ensure_clean () {\n>   # Usage: ensure_valid_ref_format REF\n>   ensure_valid_ref_format () {\n>   \tassert test $# = 1\n>   \tgit check-ref-format \"refs/heads/$1\" ||\n>   \t\tdie \"fatal: '$1' does not look like a ref\"\n>   }\n>   \n> -# Usage: check if a commit from another subtree should be\n> +# Usage: should_ignore_subtree_split_commit REV\n> +#\n> +# Check if REV is a commit from another subtree and should be\n>   # ignored from processing for splits\n>   should_ignore_subtree_split_commit () {\n>   \tassert test $# = 1\n> -\tlocal rev=\"$1\"\n> +\n> +\tgit show \\\n> +\t\t--no-patch \\\n> +\t\t--no-show-signature \\\n> +\t\t--format='%(trailers:key=git-subtree-dir,key=git-subtree-mainline)' \\\n> +\t\t\"$1\" |\n> +\t(\n> +\thave_mainline=\n> +\tsubtree_dir=\n> +\n> +\twhile read -r trailer val\n> +\tdo\n> +\t\tcase \"$trailer\" in\n> +\t\tgit-subtree-dir:)\n> +\t\t\tsubtree_dir=\"${val%/}\" ;;\n> +\t\tgit-subtree-mainline:)\n> +\t\t\thave_mainline=y ;;\n> +\t\tesac\n> +\tdone\n> +\n> -\tif test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $rev)\"\n> +\tif test -n \"${subtree_dir}\" &&\n> +\t\ttest -z \"${have_mainline}\" &&\n> +\t\ttest \"${subtree_dir}\" != \"$arg_prefix\"\n>   \tthen\n> -\t\tif test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $rev)\" &&\n> -\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $rev)\"\n> -\t\tthen\n> -\t\t\treturn 0\n> -\t\tfi\n> +\t\treturn 0\n>   \tfi\n>   \treturn 1\n> +\t)\n>   }\n>   \n>   # Usage: process_split_commit REV PARENTS\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index ca4df5be83..25be40e12b 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -9,6 +9,9 @@ This test verifies the basic operation of the add, merge, split, pull,\n>   and push subcommands of git subtree.\n>   '\n>   \n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n>   TEST_DIRECTORY=$(pwd)/../../../t\n>   . \"$TEST_DIRECTORY\"/test-lib.sh\n>   \n> @@ -67,6 +70,33 @@ test_create_pre2_32_repo () {\n>   \tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n>   }\n>   \n> +# test_create_subtree_add REPO ORPHAN PREFIX FILENAME ...\n> +#\n> +# Create a simple subtree on a new branch named ORPHAN in REPO.\n> +# The subtree is then merged into the current branch of REPO,\n> +# under PREFIX. The generated subtree has has one commit\n> +# with subject and tag FILENAME with a single file \"FILENAME.t\"\n> +#\n> +# When this method returns:\n> +# - the current branch of REPO will have file PREFIX/FILENAME.t\n> +# - REPO will have a branch named ORPHAN with subtree history\n> +#\n> +# additional arguments are forwarded to \"subtree add\"\n> +test_create_subtree_add () {\n> +\t(\n> +\t\tcd \"$1\" &&\n> +\t\torphan=\"$2\" &&\n> +\t\tprefix=\"$3\" &&\n> +\t\tfilename=\"$4\" &&\n> +\t\tshift 4 &&\n> +\t\tlast=\"$(git branch --show-current)\" &&\n> +\t\tgit switch --orphan \"$orphan\" &&\n> +\t\ttest_commit \"$filename\" &&\n> +\t\tgit checkout \"$last\" &&\n> +\t\tgit subtree add --prefix=\"$prefix\" \"$@\" \"$orphan\"\n> +\t)\n> +}\n> +\n>   test_expect_success 'shows short help text for -h' '\n>   \ttest_expect_code 129 git subtree -h >out 2>err &&\n>   \ttest_must_be_empty err &&\n> @@ -425,6 +455,47 @@ test_expect_success 'split with multiple subtrees' '\n>   \t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n>   '\n>   \n> +# When subtree split-ing a directory that has other subtree\n> +# *merges* underneath it, the split must include those subtrees.\n> +# This test creates a nested subtree, `subA/subB`, and tests\n> +# that the tree is correct after a subtree split of `subA/`.\n> +# The test covers:\n> +# - An initial `subtree add`; and\n> +# - A follow-up `subtree merge`\n> +# both with and without `--squashed`.\n> +for is_squashed in '' 'y'\n> +do\n> +\ttest_expect_success \"split keeps nested ${is_squashed:+--squash }subtrees that are part of the split\" '\n> +\t\tsubtree_test_create_repo \"$test_count\" &&\n> +\t\t(\n> +\t\t\tcd \"$test_count\" &&\n> +\t\t\tmkdir subA &&\n> +\t\t\ttest_commit subA/file1 &&\n> +\t\t\ttest_create_subtree_add \\\n> +\t\t\t\t. mksubtree subA/subB file2 ${is_squashed:+--squash} &&\n> +\t\t\ttest_path_is_file subA/file1.t &&\n> +\t\t\ttest_path_is_file subA/subB/file2.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest_path_is_file file1.t &&\n> +\t\t\ttest_path_is_file subB/file2.t &&\n> +\t\t\tgit checkout mksubtree &&\n> +\t\t\tgit branch -D bsplit &&\n> +\t\t\ttest_commit file3 &&\n> +\t\t\tgit checkout main &&\n> +\t\t\tgit subtree merge \\\n> +\t\t\t\t${is_squashed:+--squash} \\\n> +\t\t\t\t--prefix=subA/subB mksubtree &&\n> +\t\t\ttest_path_is_file subA/subB/file3.t &&\n> +\t\t\tgit subtree split --prefix=subA --branch=bsplit &&\n> +\t\t\tgit checkout bsplit &&\n> +\t\t\ttest_path_is_file file1.t &&\n> +\t\t\ttest_path_is_file subB/file2.t &&\n> +\t\t\ttest_path_is_file subB/file3.t\n> +\t\t)\n> +\t'\n> +done\n> +\n>   test_expect_success 'split sub dir/ with --rejoin from scratch' '\n>   \tsubtree_test_create_repo \"$test_count\" &&\n>   \ttest_create_commit \"$test_count\" main1 &&\n> \n> base-commit: 09669c729af92144fde84e97d358759b5b42b555\n\n"},{"id":"526037","messageId":"xmqq7by6tkan.fsf@gitster.g","threadId":"64025","inReplyTo":"641aaa9b-2b23-4faf-a13e-f6205e9ef5a2@howdoi.land","subject":"Re: [PATCH v2] contrib/subtree: fix split with squashed subtrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-10T15:10:56Z","receivedAt":"2025-09-10T15:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Colin Stagner <ask+git@howdoi.land> writes:\n\n> On 9/9/25 21:02, Junio C Hamano wrote:\n>> Besides, \"if test -n ${subtree_dir-}\" without colon would be the\n>> more proper way for those who care about \"set -u\", wouldn't it?  It\n>> is not that you want to substitute with an empty string that comes\n>> between that \"-\" and \"}\" when subtree_dir is unset or set to empty.\n>> You are preparing for the case where the variable is truly not set,\n>> and the variable being set to an empty string is not something you\n>> are worried about.\n> Yes, \"test -n ${subtree_dir-}\" is definitely the more correct expression.\n>\n> At the very real risk of embarrassing myself in public today... in the\n> particular case of a \"test -n,\" is there actually an appreciable\n> difference? Either way, the output of the substitution is empty if the\n> input is empty or undefined. Here, \"test -n ${subtree_dir:-}\" is\n> merely less efficient. Right?\n>\n> The difference between \"${x:-}\" vs \"${x-}\" really starts to matter if\n> you want to permit the empty string (or not). It also matters if you\n> call a command that has side effects.\n>\n> (And in the context of this patch, neither are necessary.)\n\nCorrect.  There is no practical difference.\n\nYour explanation for using the \"default values\" parameter expansion\nin this script, knowing that \"set -u\" is not in use, being it is out\nof inertia, I would have expected them to be written in a way that\nis suitable when \"set -u\" is in use, which is without colon.  Doing\nsomething \"different\" on a variable that is set but set to an empty\nstring is not something you would want to do to deal with \"set -u\",\nso I found it strange to see the colon there.\n\nThere is no practical difference, since the \"default value\"\nspecified is an empty string, so a variable set to an empty string\nwill use the empty string between \":-\" and \"}\" instead of its value\nthat is another empty string, and you can tell these two empty\nstrings apart in the result ;-)\n\n"},{"id":"526133","messageId":"xmqqms71ou5f.fsf@gitster.g","threadId":"64025","inReplyTo":"dd71ebee-8629-43c3-aa2a-40124400f262@gmail.com","subject":"Re: [PATCH v3] contrib/subtree: fix split with squashed subtrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-11T16:01:32Z","receivedAt":"2025-09-11T16:01:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Hi Colin\n>\n> Thanks for working on this. I'm not particularly familiar with\n> git-subtree but as far as I can see this version looks good.\n>\n> Thanks\n>\n> Phillip\n\nThanks, both of you.  Queued.\n\n"}]}