{"thread":{"id":"60241","subject":"[PATCH] subtree: fix split processing with multiple subtrees present","startedAt":"2023-09-18T20:05:23Z","lastAt":"2025-08-21T03:51:57Z","messageCount":30,"participants":["Zach FettersMoore via GitGitGadget","Junio C Hamano","Zach FettersMoore","Christian Couder","Colin Stagner"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481973","messageId":"pull.1587.git.1695067516192.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":null,"subject":"[PATCH] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-18T20:05:16Z","receivedAt":"2023-09-18T20:05:23Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nIn the diagram below, 'M' represents the mainline repo branch, 'A'\nrepresents one subtree, and 'B' represents another. M3 and B1 represent\na split commit for subtree B that was created from commit M4. M2 and A1\nrepresent a split commit made from subtree A that was also created\nbased on changes back to and including M4. M1 represents new changes to\nthe repo, in this scenario if you try to run a 'git subtree split\n--rejoin' for subtree B, commits M1, M2, and A1, will be included in\nthe processing of changes for the new split commit since the last\nsplit/rejoin for subtree B was at M3. The issue is that by having A1\nincluded in this processing the command ends up needing to processing\nevery commit down tree A even though none of that is needed or relevant\nto the current command and result.\n\nM1\n |\t \\\t \\\nM2\t |\t |\n | \t A1\t |\nM3\t |\t |\n |\t |\t B1\nM4\t |\t |\n\nSo this commit makes a change to the processing of commits for the split\ncommand in order to ignore non-mainline commits from other subtrees such\nas A1 in the diagram by adding a new function\n'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to still\nfunction as expected but removes all of the unnecessary processing that\ntakes place currently which greatly inflates the processing time.\n\nSigned-off-by: Zach FettersMoore \n---\n subtree: fix split processing with multiple subtrees present\n \n When there are multiple subtrees in a repo and git subtree split\n --rejoin is being used for the subtrees, the processing of commits for a\n new split can take a significant (and constantly growing) amount of time\n because the split commits from other subtrees cause the processing to\n have to scan the entire history of the other subtree(s). This patch\n filters out the other subtree split commits that are unnecessary for the\n split commit processing.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\n contrib/subtree/git-subtree.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..e9250dfb019 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n+# Usage: check if a commit from another subtree should be ignored from processing for splits\n+should_ignore_subtree_commit () {\n+ if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n+ then\n+ if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n+ then\n+ return 0\n+ fi\n+ fi\n+ return 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n \tlocal rev=\"$1\"\n \tlocal parents=\"$2\"\n \n+ if should_ignore_subtree_commit $rev\n+ then\n+\t return\n+ fi\n+\n \tif test $indent -eq 0\n \tthen\n \t\trevcount=$(($revcount + 1))\n\nbase-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n-- \ngitgitgadget\n"},{"id":"481997","messageId":"xmqq34zbjbxl.fsf@gitster.g","threadId":"60241","inReplyTo":"pull.1587.git.1695067516192.gitgitgadget@gmail.com","subject":"Re: [PATCH] subtree: fix split processing with multiple subtrees present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-18T23:31:34Z","receivedAt":"2023-09-18T23:31:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Zach FettersMoore via GitGitGadget\" \nwrites:\n\n> contrib/subtree/git-subtree.sh | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index e0c5d3b0de6..e9250dfb019 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n> \t\tdie \"fatal: '$1' does not look like a ref\"\n> }\n> \n> +# Usage: check if a commit from another subtree should be ignored from processing for splits\n> +should_ignore_subtree_commit () {\n> + if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n> + then\n> + if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n> + then\n> + return 0\n> + fi\n> + fi\n> + return 1\n> +}\n>\n> # Usage: process_split_commit REV PARENTS\n> process_split_commit () {\n> \tassert test $# = 2\n> \tlocal rev=\"$1\"\n> \tlocal parents=\"$2\"\n> \n> + if should_ignore_subtree_commit $rev\n> + then\n> +\t return\n> + fi\n> +\n\nPlease do not violate Documentation/CodingGuidelines for our shell\nscripted Porcelain, even if it is a script in contrib/ and also\nplease avoid bash-isms.\n\nAlso doesn't \"subtree\" have its own test? If this change is a fix\nfor some problem(s), can we have a test or two that demonstrate how\nthe current code without the patch is broken?\n\nThanks.\n\n"},{"id":"481998","messageId":"xmqqpm2fht2x.fsf@gitster.g","threadId":"60241","inReplyTo":"pull.1587.git.1695067516192.gitgitgadget@gmail.com","subject":"Re: [PATCH] subtree: fix split processing with multiple subtrees present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-19T01:04:06Z","receivedAt":"2023-09-19T01:04:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Zach FettersMoore via GitGitGadget\" \nwrites:\n\n> In the diagram below, 'M' represents the mainline repo branch, 'A'\n> represents one subtree, and 'B' represents another. M3 and B1 represent\n> a split commit for subtree B that was created from commit M4. M2 and A1\n> represent a split commit made from subtree A that was also created\n> based on changes back to and including M4. M1 represents new changes to\n> the repo, in this scenario if you try to run a 'git subtree split\n> --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n> the processing of changes for the new split commit since the last\n> split/rejoin for subtree B was at M3. The issue is that by having A1\n> included in this processing the command ends up needing to processing\n> every commit down tree A even though none of that is needed or relevant\n> to the current command and result.\n>\n> M1\n> | \\ \\\n> M2 | |\n> | A1 |\n> M3 | |\n> | | B1\n> M4 | |\n\nThe above paragraph explains which different things you drew in the\ndiagram are representing, but it is not clear how they relate to\neach other. Do they for example depict parent-child commit\nrelationship? What are the wide gaps between these three tracks and\nwhat are the short angled lines leaning to the left near the tip?\nIs the time/topology flowing from bottom to top?\n\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index e0c5d3b0de6..e9250dfb019 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n> \t\tdie \"fatal: '$1' does not look like a ref\"\n> }\n> \n> +# Usage: check if a commit from another subtree should be ignored from processing for splits\n\nWay overlong line. Please split them accordingly. I won't comment\non what CodingGuidelines tells us already, in this review, but have\na few comments here:\n\n> +should_ignore_subtree_commit () {\n> + if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n> + then\n> + if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n\nHere $dir is a free variable that comes from outside. The caller\ndoes not supply it as a parameter to this function (and the caller\ndoes not receive it as its parameter from its caller). Yet the file\nas a whole seems to liberally make assignments to it (\"git grep dir=\"\non the file counts 7 assignments). Are we sure we are looking for\nthe right $dir in this particular grep?\n\n\tSide note: I am not familiar with this part of the code at\n\tall, so do not take it as \"here is a bug\", but more as \"this\n\tsmells error prone.\"\n\nAlso can $dir have regular expressions special characters? \"The\nexisting code and new code alike, git-subtree is not prepared to \nhandle directory names with RE special characters well at all, so\ndo not use them if you do not want your history broken\" is an\nacceptable answer.\n\nThe caller of this function process_split_commit is cmd_split and\nprocess_split_commit (hence this function) is called repeatedly\ninside a loop. This function makes a traversal over the entire\nhistory for each and every iteration in \"good\" cases where there is\nno 'mainline' or 'subtree-dir' commits for the given $dir.\n\nI wonder if it is more efficient to enumerate all commits that hits\nthese grep criteria in the cmd_split before it starts to call\nprocess_split_commit repeatedly. If it knows which commit can be\nignored beforehand, it can skip and not call process_split_commit,\nno?\n\n> + then\n> + return 0\n> + fi\n> + fi\n> + return 1\n> +}\n> +\n> # Usage: process_split_commit REV PARENTS\n> process_split_commit () {\n> \tassert test $# = 2\n> \tlocal rev=\"$1\"\n> \tlocal parents=\"$2\"\n\nThese seem to assume that $1 and $2 can have $IFS in them, so\nshouldn't ...\n\n> + if should_ignore_subtree_commit $rev\n\n... this call too enclose $rev inside a pair of double-quotes for\nconsistency? We know the loop in the cmd_split that calls this\nfunction is reading from \"rev-list --parents\" and $rev is a 40-hex\ncommit object name (and $parents can have more than one 40-hex\ncommit object names separated with SP), so it is safe to leave $rev\nunquoted, but it pays to be consistent to help make the code more\nreadable.\n\n> + then\n> +\t return\n> + fi\n> +\n> \tif test $indent -eq 0\n> \tthen\n> \t\trevcount=$(($revcount + 1))\n>\n> base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n"},{"id":"482147","messageId":"d6811daf7cf7f1460877307575e4cbc363ae851a.1695399920.git.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v2.git.1695399920.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] subtree: changing location of commit ignore processing","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-22T16:25:20Z","receivedAt":"2023-09-22T16:25:29Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nBased on feedback from original commit:\n\n-Updated the location of check whether a commit should\nbe ignored during split processing\n\n-Updated code to better fit coding guidelines\n\nSigned-off-by: Zach FettersMoore \n---\n contrib/subtree/git-subtree.sh | 30 ++++++++++++++++++++----------\n 1 file changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e9250dfb019..e69991a9d80 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,11 +778,13 @@ ensure_valid_ref_format () {\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n-# Usage: check if a commit from another subtree should be ignored from processing for splits\n-should_ignore_subtree_commit () {\n- if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n+# Usage: check if a commit from another subtree should be\n+# ignored from processing for splits\n+should_ignore_subtree_split_commit () {\n+ if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n then\n- if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n+ if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n+\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n then\n return 0\n fi\n@@ -796,11 +798,6 @@ process_split_commit () {\n \tlocal rev=\"$1\"\n \tlocal parents=\"$2\"\n \n- if should_ignore_subtree_commit $rev\n- then\n-\t return\n- fi\n-\n \tif test $indent -eq 0\n \tthen\n \t\trevcount=$(($revcount + 1))\n@@ -980,7 +977,20 @@ cmd_split () {\n \teval \"$grl\" |\n \twhile read rev parents\n \tdo\n-\t\tprocess_split_commit \"$rev\" \"$parents\"\n+\t\tif should_ignore_subtree_split_commit \"$rev\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\t\tparsedParents=''\n+\t\tfor parent in $parents\n+\t\tdo\n+\t\t\tshould_ignore_subtree_split_commit \"$parent\"\n+\t\t\tif test $? -eq 1\n+\t\t\tthen\n+\t\t\t\tparsedParents+=\"$parent \"\n+\t\t\tfi\n+\t\tdone\n+\t\tprocess_split_commit \"$rev\" \"$parsedParents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\n-- \ngitgitgadget\n"},{"id":"482148","messageId":"43175154a82ea04eec995f1d47771881a981bda6.1695399920.git.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v2.git.1695399920.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-22T16:25:19Z","receivedAt":"2023-09-22T16:25:31Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nIn the diagram below, 'M' represents the mainline repo branch, 'A'\nrepresents one subtree, and 'B' represents another. M3 and B1 represent\na split commit for subtree B that was created from commit M4. M2 and A1\nrepresent a split commit made from subtree A that was also created\nbased on changes back to and including M4. M1 represents new changes to\nthe repo, in this scenario if you try to run a 'git subtree split\n--rejoin' for subtree B, commits M1, M2, and A1, will be included in\nthe processing of changes for the new split commit since the last\nsplit/rejoin for subtree B was at M3. The issue is that by having A1\nincluded in this processing the command ends up needing to processing\nevery commit down tree A even though none of that is needed or relevant\nto the current command and result.\n\nM1\n |\t \\\t \\\nM2\t |\t |\n | \t A1\t |\nM3\t |\t |\n |\t |\t B1\nM4\t |\t |\n\nSo this commit makes a change to the processing of commits for the split\ncommand in order to ignore non-mainline commits from other subtrees such\nas A1 in the diagram by adding a new function\n'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to still\nfunction as expected but removes all of the unnecessary processing that\ntakes place currently which greatly inflates the processing time.\n\nSigned-off-by: Zach FettersMoore \n---\n contrib/subtree/git-subtree.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..e9250dfb019 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n+# Usage: check if a commit from another subtree should be ignored from processing for splits\n+should_ignore_subtree_commit () {\n+ if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n+ then\n+ if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n+ then\n+ return 0\n+ fi\n+ fi\n+ return 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n \tlocal rev=\"$1\"\n \tlocal parents=\"$2\"\n \n+ if should_ignore_subtree_commit $rev\n+ then\n+\t return\n+ fi\n+\n \tif test $indent -eq 0\n \tthen\n \t\trevcount=$(($revcount + 1))\n-- \ngitgitgadget\n\n"},{"id":"482149","messageId":"pull.1587.v2.git.1695399920.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.git.1695067516192.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-22T16:25:18Z","receivedAt":"2023-09-22T16:25:32Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"When there are multiple subtrees in a repo and git subtree split --rejoin is\nbeing used for the subtrees, the processing of commits for a new split can\ntake a significant (and constantly growing) amount of time because the split\ncommits from other subtrees cause the processing to have to scan the entire\nhistory of the other subtree(s). This patch filters out the other subtree\nsplit commits that are unnecessary for the split commit processing.\n\nZach FettersMoore (2):\n subtree: fix split processing with multiple subtrees present\n subtree: changing location of commit ignore processing\n\n contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++++++++++-\n 1 file changed, 28 insertions(+), 1 deletion(-)\n\n\nbase-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\nRange-diff vs v1:\n\n 1: 43175154a82 = 1: 43175154a82 subtree: fix split processing with multiple subtrees present\n -: ----------- > 2: d6811daf7cf subtree: changing location of commit ignore processing\n\n-- \ngitgitgadget\n"},{"id":"482453","messageId":"pull.1587.v3.git.1696019580.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v2.git.1695399920.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-29T20:32:57Z","receivedAt":"2023-09-29T20:33:16Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"When there are multiple subtrees in a repo and git subtree split --rejoin is\nbeing used for the subtrees, the processing of commits for a new split can\ntake a significant (and constantly growing) amount of time because the split\ncommits from other subtrees cause the processing to have to scan the entire\nhistory of the other subtree(s). This patch filters out the other subtree\nsplit commits that are unnecessary for the split commit processing.\n\nZach FettersMoore (3):\n subtree: fix split processing with multiple subtrees present\n subtree: changing location of commit ignore processing\n subtree: adding test to validate fix\n\n contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-\n contrib/subtree/t/t7900-subtree.sh | 41 ++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 1 deletion(-)\n\n\nbase-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\nRange-diff vs v2:\n\n 1: 43175154a82 = 1: 43175154a82 subtree: fix split processing with multiple subtrees present\n 2: d6811daf7cf = 2: d6811daf7cf subtree: changing location of commit ignore processing\n -: ----------- > 3: eff8bfcc042 subtree: adding test to validate fix\n\n-- \ngitgitgadget\n"},{"id":"482454","messageId":"d6811daf7cf7f1460877307575e4cbc363ae851a.1696019580.git.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v3.git.1696019580.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] subtree: changing location of commit ignore processing","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-29T20:32:59Z","receivedAt":"2023-09-29T20:33:18Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nBased on feedback from original commit:\n\n-Updated the location of check whether a commit should\nbe ignored during split processing\n\n-Updated code to better fit coding guidelines\n\nSigned-off-by: Zach FettersMoore \n---\n contrib/subtree/git-subtree.sh | 30 ++++++++++++++++++++----------\n 1 file changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e9250dfb019..e69991a9d80 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,11 +778,13 @@ ensure_valid_ref_format () {\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n-# Usage: check if a commit from another subtree should be ignored from processing for splits\n-should_ignore_subtree_commit () {\n- if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n+# Usage: check if a commit from another subtree should be\n+# ignored from processing for splits\n+should_ignore_subtree_split_commit () {\n+ if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n then\n- if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n+ if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n+\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n then\n return 0\n fi\n@@ -796,11 +798,6 @@ process_split_commit () {\n \tlocal rev=\"$1\"\n \tlocal parents=\"$2\"\n \n- if should_ignore_subtree_commit $rev\n- then\n-\t return\n- fi\n-\n \tif test $indent -eq 0\n \tthen\n \t\trevcount=$(($revcount + 1))\n@@ -980,7 +977,20 @@ cmd_split () {\n \teval \"$grl\" |\n \twhile read rev parents\n \tdo\n-\t\tprocess_split_commit \"$rev\" \"$parents\"\n+\t\tif should_ignore_subtree_split_commit \"$rev\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\t\tparsedParents=''\n+\t\tfor parent in $parents\n+\t\tdo\n+\t\t\tshould_ignore_subtree_split_commit \"$parent\"\n+\t\t\tif test $? -eq 1\n+\t\t\tthen\n+\t\t\t\tparsedParents+=\"$parent \"\n+\t\t\tfi\n+\t\tdone\n+\t\tprocess_split_commit \"$rev\" \"$parsedParents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\n-- \ngitgitgadget\n\n"},{"id":"482455","messageId":"eff8bfcc04278eeae658ffbff8317f822edb9b20.1696019580.git.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v3.git.1696019580.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] subtree: adding test to validate fix","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-29T20:33:00Z","receivedAt":"2023-09-29T20:33:19Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nAdding a test to validate that the proposed fix\nsolves the issue.\n\nThe test accomplishes this by checking the output\nof the split command to ensure the output from\nthe progress of 'process_split_commit' function\nthat represents the 'extracount' of commits\nprocessed does not increment.\n\nThis was tested against the original functionality\nto show the test failed, and then with this fix\nto show the test passes.\n\nThis illustrated that when using multiple subtrees,\nA and B, when doing a split on subtree B, the\nprocessing does not traverse the entire history\nof subtree A which is unnecessary and would cause\nthe 'extracount' of processed commits to climb\nbased on the number of commits in the history of\nsubtree A.\n\nSigned-off-by: Zach FettersMoore \n---\n contrib/subtree/t/t7900-subtree.sh | 41 ++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 49a21dd7c9c..57c12e9f924 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -385,6 +385,47 @@ test_expect_success 'split sub dir/ with --rejoin' '\n \t)\n '\n \n+test_expect_success 'split with multiple subtrees' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/subA\" &&\n+\tsubtree_test_create_repo \"$test_count/subB\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/subA\" subA1 &&\n+\ttest_create_commit \"$test_count/subA\" subA2 &&\n+\ttest_create_commit \"$test_count/subA\" subA3 &&\n+\ttest_create_commit \"$test_count/subB\" subB1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./subA HEAD &&\n+\t\tgit subtree add --prefix=subADir FETCH_HEAD\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./subB HEAD &&\n+\t\tgit subtree add --prefix=subBDir FETCH_HEAD\n+\t) &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA1 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 1\"\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subBDir --squash --rejoin -m \"Sub B Split 1\"\n+\t) &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA2 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB2 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 2\"\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\ttest \"$(git subtree split --prefix=subBDir --squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n+\t)\n+'\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-- \ngitgitgadget\n"},{"id":"482456","messageId":"43175154a82ea04eec995f1d47771881a981bda6.1696019580.git.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v3.git.1696019580.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-29T20:32:58Z","receivedAt":"2023-09-29T20:33:20Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nIn the diagram below, 'M' represents the mainline repo branch, 'A'\nrepresents one subtree, and 'B' represents another. M3 and B1 represent\na split commit for subtree B that was created from commit M4. M2 and A1\nrepresent a split commit made from subtree A that was also created\nbased on changes back to and including M4. M1 represents new changes to\nthe repo, in this scenario if you try to run a 'git subtree split\n--rejoin' for subtree B, commits M1, M2, and A1, will be included in\nthe processing of changes for the new split commit since the last\nsplit/rejoin for subtree B was at M3. The issue is that by having A1\nincluded in this processing the command ends up needing to processing\nevery commit down tree A even though none of that is needed or relevant\nto the current command and result.\n\nM1\n |\t \\\t \\\nM2\t |\t |\n | \t A1\t |\nM3\t |\t |\n |\t |\t B1\nM4\t |\t |\n\nSo this commit makes a change to the processing of commits for the split\ncommand in order to ignore non-mainline commits from other subtrees such\nas A1 in the diagram by adding a new function\n'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to still\nfunction as expected but removes all of the unnecessary processing that\ntakes place currently which greatly inflates the processing time.\n\nSigned-off-by: Zach FettersMoore \n---\n contrib/subtree/git-subtree.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..e9250dfb019 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n \t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n+# Usage: check if a commit from another subtree should be ignored from processing for splits\n+should_ignore_subtree_commit () {\n+ if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n+ then\n+ if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n+ then\n+ return 0\n+ fi\n+ fi\n+ return 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n \tlocal rev=\"$1\"\n \tlocal parents=\"$2\"\n \n+ if should_ignore_subtree_commit $rev\n+ then\n+\t return\n+ fi\n+\n \tif test $indent -eq 0\n \tthen\n \t\trevcount=$(($revcount + 1))\n-- \ngitgitgadget\n\n"},{"id":"483929","messageId":"pull.1587.v4.git.1698347871200.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v3.git.1696019580.gitgitgadget@gmail.com","subject":"[PATCH v4] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-26T19:17:51Z","receivedAt":"2023-10-26T19:17:56Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nIn the diagram below, 'M' represents the mainline repo branch, 'A'\nrepresents one subtree, and 'B' represents another. M3 and B1 represent\na split commit for subtree B that was created from commit M4. M2 and A1\nrepresent a split commit made from subtree A that was also created\nbased on changes back to and including M4. M1 represents new changes to\nthe repo, in this scenario if you try to run a 'git subtree split\n--rejoin' for subtree B, commits M1, M2, and A1, will be included in\nthe processing of changes for the new split commit since the last\nsplit/rejoin for subtree B was at M3. The issue is that by having A1\nincluded in this processing the command ends up needing to processing\nevery commit down tree A even though none of that is needed or relevant\nto the current command and result.\n\nM1\n |\t \\\t \\\nM2\t |\t |\n | \t A1\t |\nM3\t |\t |\n |\t |\t B1\nM4\t |\t |\n\nSo this commit makes a change to the processing of commits for the split\ncommand in order to ignore non-mainline commits from other subtrees such\nas A1 in the diagram by adding a new function\n'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to still\nfunction as expected but removes all of the unnecessary processing that\ntakes place currently which greatly inflates the processing time.\n\nAdded a test to validate that the proposed fix\nsolves the issue.\n\nThe test accomplishes this by checking the output\nof the split command to ensure the output from\nthe progress of 'process_split_commit' function\nthat represents the 'extracount' of commits\nprocessed does not increment.\n\nThis was tested against the original functionality\nto show the test failed, and then with this fix\nto show the test passes.\n\nThis illustrated that when using multiple subtrees,\nA and B, when doing a split on subtree B, the\nprocessing does not traverse the entire history\nof subtree A which is unnecessary and would cause\nthe 'extracount' of processed commits to climb\nbased on the number of commits in the history of\nsubtree A.\n\nSigned-off-by: Zach FettersMoore \n---\n subtree: fix split processing with multiple subtrees present\n \n When there are multiple subtrees in a repo and git subtree split\n --rejoin is being used for the subtrees, the processing of commits for a\n new split can take a significant (and constantly growing) amount of time\n because the split commits from other subtrees cause the processing to\n have to scan the entire history of the other subtree(s). This patch\n filters out the other subtree split commits that are unnecessary for the\n split commit processing.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\nRange-diff vs v3:\n\n 1: 43175154a82 < -: ----------- subtree: fix split processing with multiple subtrees present\n 2: d6811daf7cf < -: ----------- subtree: changing location of commit ignore processing\n 3: eff8bfcc042 ! 1: 353152910eb subtree: adding test to validate fix\n @@ Metadata\n Author: Zach FettersMoore \n \n ## Commit message ##\n - subtree: adding test to validate fix\n + subtree: fix split processing with multiple subtrees present\n \n - Adding a test to validate that the proposed fix\n + When there are multiple subtrees present in a repository and they are\n + all using 'git subtree split', the 'split' command can take a\n + significant (and constantly growing) amount of time to run even when\n + using the '--rejoin' flag. This is due to the fact that when processing\n + commits to determine the last known split to start from when looking\n + for changes, if there has been a split/merge done from another subtree\n + there will be 2 split commits, one mainline and one subtree, for the\n + second subtree that are part of the processing. The non-mainline\n + subtree split commit will cause the processing to always need to search\n + the entire history of the given subtree as part of its processing even\n + though those commits are totally irrelevant to the current subtree\n + split being run.\n +\n + In the diagram below, 'M' represents the mainline repo branch, 'A'\n + represents one subtree, and 'B' represents another. M3 and B1 represent\n + a split commit for subtree B that was created from commit M4. M2 and A1\n + represent a split commit made from subtree A that was also created\n + based on changes back to and including M4. M1 represents new changes to\n + the repo, in this scenario if you try to run a 'git subtree split\n + --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n + the processing of changes for the new split commit since the last\n + split/rejoin for subtree B was at M3. The issue is that by having A1\n + included in this processing the command ends up needing to processing\n + every commit down tree A even though none of that is needed or relevant\n + to the current command and result.\n +\n + M1\n + | \\ \\\n + M2 | |\n + | A1 |\n + M3 | |\n + | | B1\n + M4 | |\n +\n + So this commit makes a change to the processing of commits for the split\n + command in order to ignore non-mainline commits from other subtrees such\n + as A1 in the diagram by adding a new function\n + 'should_ignore_subtree_commit' which is called during\n + 'process_split_commit'. This allows the split/rejoin processing to still\n + function as expected but removes all of the unnecessary processing that\n + takes place currently which greatly inflates the processing time.\n +\n + Added a test to validate that the proposed fix\n solves the issue.\n \n The test accomplishes this by checking the output\n @@ Commit message\n \n Signed-off-by: Zach FettersMoore \n \n + ## contrib/subtree/git-subtree.sh ##\n +@@ contrib/subtree/git-subtree.sh: ensure_valid_ref_format () {\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 ++# ignored from processing for splits\n ++should_ignore_subtree_split_commit () {\n ++ if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n ++ then\n ++ if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n ++\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n ++ then\n ++ return 0\n ++ fi\n ++ fi\n ++ return 1\n ++}\n ++\n + # Usage: process_split_commit REV PARENTS\n + process_split_commit () {\n + \tassert test $# = 2\n +@@ contrib/subtree/git-subtree.sh: cmd_split () {\n + \teval \"$grl\" |\n + \twhile read rev parents\n + \tdo\n +-\t\tprocess_split_commit \"$rev\" \"$parents\"\n ++\t\tif should_ignore_subtree_split_commit \"$rev\"\n ++\t\tthen\n ++\t\t\tcontinue\n ++\t\tfi\n ++\t\tparsedParents=''\n ++\t\tfor parent in $parents\n ++\t\tdo\n ++\t\t\tshould_ignore_subtree_split_commit \"$parent\"\n ++\t\t\tif test $? -eq 1\n ++\t\t\tthen\n ++\t\t\t\tparsedParents+=\"$parent \"\n ++\t\t\tfi\n ++\t\tdone\n ++\t\tprocess_split_commit \"$rev\" \"$parsedParents\"\n + \tdone || exit $?\n + \n + \tlatest_new=$(cache_get latest_new) || exit $?\n +\n ## contrib/subtree/t/t7900-subtree.sh ##\n @@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --rejoin' '\n \t)\n @@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r\n +\t) &&\n +\t(\n +\t\tcd \"$test_count\" &&\n -+\t\ttest \"$(git subtree split --prefix=subBDir --squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n ++\t\ttest \"$(git subtree split --prefix=subBDir --squash --rejoin \\\n ++\t\t -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n +\t)\n +'\n +\n\n\n contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-\n contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..e69991a9d80 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,6 +778,20 @@ ensure_valid_ref_format () {\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+# ignored from processing for splits\n+should_ignore_subtree_split_commit () {\n+ if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n+ then\n+ if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n+\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n+ then\n+ return 0\n+ fi\n+ fi\n+ return 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n@@ -963,7 +977,20 @@ cmd_split () {\n \teval \"$grl\" |\n \twhile read rev parents\n \tdo\n-\t\tprocess_split_commit \"$rev\" \"$parents\"\n+\t\tif should_ignore_subtree_split_commit \"$rev\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\t\tparsedParents=''\n+\t\tfor parent in $parents\n+\t\tdo\n+\t\t\tshould_ignore_subtree_split_commit \"$parent\"\n+\t\t\tif test $? -eq 1\n+\t\t\tthen\n+\t\t\t\tparsedParents+=\"$parent \"\n+\t\t\tfi\n+\t\tdone\n+\t\tprocess_split_commit \"$rev\" \"$parsedParents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 49a21dd7c9c..87d59afd761 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '\n \t)\n '\n \n+test_expect_success 'split with multiple subtrees' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/subA\" &&\n+\tsubtree_test_create_repo \"$test_count/subB\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/subA\" subA1 &&\n+\ttest_create_commit \"$test_count/subA\" subA2 &&\n+\ttest_create_commit \"$test_count/subA\" subA3 &&\n+\ttest_create_commit \"$test_count/subB\" subB1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./subA HEAD &&\n+\t\tgit subtree add --prefix=subADir FETCH_HEAD\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit fetch ./subB HEAD &&\n+\t\tgit subtree add --prefix=subBDir FETCH_HEAD\n+\t) &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA1 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB1 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 1\"\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subBDir --squash --rejoin -m \"Sub B Split 1\"\n+\t) &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA2 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB2 &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 2\"\n+\t) &&\n+\t(\n+\t\tcd \"$test_count\" &&\n+\t\ttest \"$(git subtree split --prefix=subBDir --squash --rejoin \\\n+\t\t -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n+\t)\n+'\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: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n-- \ngitgitgadget\n"},{"id":"483930","messageId":"CAEWN6q3HfeU1Uj4TPiiVW8PO13xwxgqihPH5-cm8T1oK5tHVTQ@mail.gmail.com","threadId":"60241","inReplyTo":"xmqqpm2fht2x.fsf@gitster.g","subject":"Re: [PATCH] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore","fromEmail":"zach.fetters@apollographql.com","sentAt":"2023-10-26T19:59:42Z","receivedAt":"2023-10-26T19:59:58Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"> Please do not violate Documentation/CodingGuidelines for our shell\n> scripted Porcelain, even if it is a script in contrib/ and also\n>please avoid bash-isms.\n\nI believe I have resolved the CodingGuidelines issues.\n\n> Also doesn't \"subtree\" have its own test? If this change is a fix\n> for some problem(s), can we have a test or two that demonstrate how\n> the current code without the patch is broken?\n\nI was able to add a test that validates against some of the metrics that\nare tracked when running a split for processing commits. Validated that\nbefore my fix the test fails, and after my fix the test passes.\n\n>> In the diagram below, 'M' represents the mainline repo branch, 'A'\n>> represents one subtree, and 'B' represents another. M3 and B1 represent\n>> a split commit for subtree B that was created from commit M4. M2 and A1\n>> represent a split commit made from subtree A that was also created\n>> based on changes back to and including M4. M1 represents new changes to\n>> the repo, in this scenario if you try to run a 'git subtree split\n>> --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n>> the processing of changes for the new split commit since the last\n>> split/rejoin for subtree B was at M3. The issue is that by having A1\n>> included in this processing the command ends up needing to processing\n>> every commit down tree A even though none of that is needed or relevant\n>> to the current command and result.\n>>\n>> M1\n>> | \\ \\\n>> M2 | |\n>> | A1 |\n>> M3 | |\n>> | | B1\n>> M4 | |\n\n> The above paragraph explains which different things you drew in the\n> diagram are representing, but it is not clear how they relate to\n> each other. Do they for example depict parent-child commit\n> relationship? What are the wide gaps between these three tracks and\n> what are the short angled lines leaning to the left near the tip?\n> Is the time/topology flowing from bottom to top?\n\nI am realizing I made a few mistakes with trying to illustrate the diagram\nwhich I will attempt to make more clear below. As for the 3 columns in the\ndiagram, 'M' represents the mainline branch of the repo being developed in,\nwhile column 'A' represents the history of a subtree 'A' included in the\nrepo, and column 'B' also represents the history of a subtree 'B' in the\nrepo. The diagram attempts to illustrate when a 'git subtree split --rejoin'\nis used, that there is a commit made in the subtrees history, and that is\nthen merged into the mainline repo branch.\n\nM1\n |\n |\nM2 --- |\n | A1\n | |\nM3 ---------- |\n | | B1\nM4 | |\n\nHopefully that helps better illustrate the state of the repo before the new\n'git subtree split --rejoin' attempt and why it results in the described issue.\n\n>> +should_ignore_subtree_commit () {\n>> + if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n>> + then\n>> + if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n>\n> Here $dir is a free variable that comes from outside. The caller\n> does not supply it as a parameter to this function (and the caller\n> does not receive it as its parameter from its caller). Yet the file\n> as a whole seems to liberally make assignments to it (\"git grep dir=\"\n> on the file counts 7 assignments). Are we sure we are looking for\n> the right $dir in this particular grep?\n>\n> Side note: I am not familiar with this part of the code at\n> all, so do not take it as \"here is a bug\", but more as \"this\n> smells error prone.\"\n\nFrom my testing and what I see for '$dir' usage in the 'cmd_split'\nfunction which leads to this code it is the correct '$dir', although\nI see your point about it being reassigned in different places which\nmakes it error prone. I switched this to use the command\nline argument '$arg_prefix' since the subtree prefix passed into\nthe command is what we actually want in this case so we can filter\nout commits from other subtrees.\n\n> Also can $dir have regular expressions special characters? \"The\n> existing code and new code alike, git-subtree is not prepared to\n> handle directory names with RE special characters well at all, so\n> do not use them if you do not want your history broken\" is an\n> acceptable answer.\n\nAs far as I can tell from looking at the code (which I only recently\nstarted using) the '$dir' which is based on the subtree prefix is\nnot setup to handle this.\n\n> The caller of this function process_split_commit is cmd_split and\n> process_split_commit (hence this function) is called repeatedly\n> inside a loop. This function makes a traversal over the entire\n> history for each and every iteration in \"good\" cases where there is\n> no 'mainline' or 'subtree-dir' commits for the given $dir.\n>\n> I wonder if it is more efficient to enumerate all commits that hits\n> these grep criteria in the cmd_split before it starts to call\n> process_split_commit repeatedly. If it knows which commit can be\n> ignored beforehand, it can skip and not call process_split_commit,\n> no?\n\nMoved this functionality into the 'cmd_split' function as suggested.\n\n>> + then\n>> + return 0\n>> + fi\n>> + fi\n>> + return 1\n>> +}\n>> +\n>> # Usage: process_split_commit REV PARENTS\n>> process_split_commit () {\n>> assert test $# = 2\n>> local rev=\"$1\"\n>> local parents=\"$2\"\n>\n> These seem to assume that $1 and $2 can have $IFS in them, so\n> shouldn't ...\n>\n>> + if should_ignore_subtree_commit $rev\n>\n> ... this call too enclose $rev inside a pair of double-quotes for\n> consistency? We know the loop in the cmd_split that calls this\n> function is reading from \"rev-list --parents\" and $rev is a 40-hex\n> commit object name (and $parents can have more than one 40-hex\n> commit object names separated with SP), so it is safe to leave $rev\n> unquoted, but it pays to be consistent to help make the code more\n> readable.\n\nUpdated this for consistency\n\n\nOn Mon, Sep 18, 2023 at 9:04 PM Junio C Hamano wrote:\n>\n> \"Zach FettersMoore via GitGitGadget\" \n> writes:\n>\n> > In the diagram below, 'M' represents the mainline repo branch, 'A'\n> > represents one subtree, and 'B' represents another. M3 and B1 represent\n> > a split commit for subtree B that was created from commit M4. M2 and A1\n> > represent a split commit made from subtree A that was also created\n> > based on changes back to and including M4. M1 represents new changes to\n> > the repo, in this scenario if you try to run a 'git subtree split\n> > --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n> > the processing of changes for the new split commit since the last\n> > split/rejoin for subtree B was at M3. The issue is that by having A1\n> > included in this processing the command ends up needing to processing\n> > every commit down tree A even though none of that is needed or relevant\n> > to the current command and result.\n> >\n> > M1\n> > | \\ \\\n> > M2 | |\n> > | A1 |\n> > M3 | |\n> > | | B1\n> > M4 | |\n>\n> The above paragraph explains which different things you drew in the\n> diagram are representing, but it is not clear how they relate to\n> each other. Do they for example depict parent-child commit\n> relationship? What are the wide gaps between these three tracks and\n> what are the short angled lines leaning to the left near the tip?\n> Is the time/topology flowing from bottom to top?\n>\n> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> > index e0c5d3b0de6..e9250dfb019 100755\n> > --- a/contrib/subtree/git-subtree.sh\n> > +++ b/contrib/subtree/git-subtree.sh\n> > @@ -778,12 +778,29 @@ ensure_valid_ref_format () {\n> > die \"fatal: '$1' does not look like a ref\"\n> > }\n> >\n> > +# Usage: check if a commit from another subtree should be ignored from processing for splits\n>\n> Way overlong line. Please split them accordingly. I won't comment\n> on what CodingGuidelines tells us already, in this review, but have\n> a few comments here:\n>\n> > +should_ignore_subtree_commit () {\n> > + if [ \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\" ]\n> > + then\n> > + if [[ -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" && -z \"$(git log -1 --grep=\"git-subtree-dir: $dir$\" $1)\" ]]\n>\n> Here $dir is a free variable that comes from outside. The caller\n> does not supply it as a parameter to this function (and the caller\n> does not receive it as its parameter from its caller). Yet the file\n> as a whole seems to liberally make assignments to it (\"git grep dir=\"\n> on the file counts 7 assignments). Are we sure we are looking for\n> the right $dir in this particular grep?\n>\n> Side note: I am not familiar with this part of the code at\n> all, so do not take it as \"here is a bug\", but more as \"this\n> smells error prone.\"\n>\n> Also can $dir have regular expressions special characters? \"The\n> existing code and new code alike, git-subtree is not prepared to\n> handle directory names with RE special characters well at all, so\n> do not use them if you do not want your history broken\" is an\n> acceptable answer.\n>\n> The caller of this function process_split_commit is cmd_split and\n> process_split_commit (hence this function) is called repeatedly\n> inside a loop. This function makes a traversal over the entire\n> history for each and every iteration in \"good\" cases where there is\n> no 'mainline' or 'subtree-dir' commits for the given $dir.\n>\n> I wonder if it is more efficient to enumerate all commits that hits\n> these grep criteria in the cmd_split before it starts to call\n> process_split_commit repeatedly. If it knows which commit can be\n> ignored beforehand, it can skip and not call process_split_commit,\n> no?\n>\n> > + then\n> > + return 0\n> > + fi\n> > + fi\n> > + return 1\n> > +}\n> > +\n> > # Usage: process_split_commit REV PARENTS\n> > process_split_commit () {\n> > assert test $# = 2\n> > local rev=\"$1\"\n> > local parents=\"$2\"\n>\n> These seem to assume that $1 and $2 can have $IFS in them, so\n> shouldn't ...\n>\n> > + if should_ignore_subtree_commit $rev\n>\n> ... this call too enclose $rev inside a pair of double-quotes for\n> consistency? We know the loop in the cmd_split that calls this\n> function is reading from \"rev-list --parents\" and $rev is a 40-hex\n> commit object name (and $parents can have more than one 40-hex\n> commit object names separated with SP), so it is safe to leave $rev\n> unquoted, but it pays to be consistent to help make the code more\n> readable.\n>\n> > + then\n> > + return\n> > + fi\n> > +\n> > if test $indent -eq 0\n> > then\n> > revcount=$(($revcount + 1))\n> >\n> > base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n"},{"id":"485003","messageId":"CAP8UFD18Hh=m8HQibAgZW1KNAn6zg_rxe9asg0ViC5z27W=Smw@mail.gmail.com","threadId":"60241","inReplyTo":"pull.1587.v4.git.1698347871200.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-11-18T11:28:42Z","receivedAt":"2023-11-18T11:28:58Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 26, 2023 at 9:18 PM Zach FettersMoore via GitGitGadget\n wrote:\n>\n> From: Zach FettersMoore \n>\n> When there are multiple subtrees present in a repository and they are\n> all using 'git subtree split', the 'split' command can take a\n> significant (and constantly growing) amount of time to run even when\n> using the '--rejoin' flag. This is due to the fact that when processing\n> commits to determine the last known split to start from when looking\n> for changes, if there has been a split/merge done from another subtree\n> there will be 2 split commits, one mainline and one subtree, for the\n> second subtree that are part of the processing. The non-mainline\n> subtree split commit will cause the processing to always need to search\n> the entire history of the given subtree as part of its processing even\n> though those commits are totally irrelevant to the current subtree\n> split being run.\n\nThanks for your continued work on this!\n\nI am not familiar with git subtree so I might miss obvious things. On\nthe other hand, my comments might help increase a bit the number of\npeople who could review this patch.\n\n> In the diagram below, 'M' represents the mainline repo branch, 'A'\n> represents one subtree, and 'B' represents another. M3 and B1 represent\n> a split commit for subtree B that was created from commit M4. M2 and A1\n> represent a split commit made from subtree A that was also created\n> based on changes back to and including M4. M1 represents new changes to\n> the repo, in this scenario if you try to run a 'git subtree split\n> --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n> the processing of changes for the new split commit since the last\n> split/rejoin for subtree B was at M3. The issue is that by having A1\n> included in this processing the command ends up needing to processing\n> every commit down tree A even though none of that is needed or relevant\n> to the current command and result.\n>\n> M1\n> | \\ \\\n> M2 | |\n> | A1 |\n> M3 | |\n> | | B1\n> M4 | |\n\nAbout the above, Junio already commented the following:\n\n-> The above paragraph explains which different things you drew in the\n-> diagram are representing, but it is not clear how they relate to\n-> each other. Do they for example depict parent-child commit\n-> relationship? What are the wide gaps between these three tracks and\n-> what are the short angled lines leaning to the left near the tip?\n-> Is the time/topology flowing from bottom to top?\n\nand it doesn't look like you have addressed that comment.\n\nWhen you say \"M3 and B1 represent a split commit for subtree B that\nwas created from commit M4.\" I am not sure what it means exactly.\nCould you give example commands that could have created the M3 and B1\ncommits?\n\n> So this commit makes a change to the processing of commits for the split\n> command in order to ignore non-mainline commits from other subtrees such\n> as A1 in the diagram by adding a new function\n> 'should_ignore_subtree_commit' which is called during\n> 'process_split_commit'. This allows the split/rejoin processing to still\n> function as expected but removes all of the unnecessary processing that\n> takes place currently which greatly inflates the processing time.\n\nCould you tell a bit more what kind of processing time reduction is or\nwould be possible on what kind of repo? Have you benchmark-ed or just\ntimed this somehow on one of your repos or better on an open source\nrepo (so that we could reproduce if we wanted)?\n\n> Added a test to validate that the proposed fix\n> solves the issue.\n>\n> The test accomplishes this by checking the output\n> of the split command to ensure the output from\n> the progress of 'process_split_commit' function\n> that represents the 'extracount' of commits\n> processed does not increment.\n\nDoes not increment compared to what?\n\n> This was tested against the original functionality\n> to show the test failed, and then with this fix\n> to show the test passes.\n>\n> This illustrated that when using multiple subtrees,\n> A and B, when doing a split on subtree B, the\n> processing does not traverse the entire history\n> of subtree A which is unnecessary and would cause\n> the 'extracount' of processed commits to climb\n> based on the number of commits in the history of\n> subtree A.\n\nDoes this mean that the test checks that the extracount is the same\nwhen subtree A exists as when it doesn't exist?\n\n[...]\n\n> contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-\n> contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++\n> 2 files changed, 70 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index e0c5d3b0de6..e69991a9d80 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -778,6 +778,20 @@ ensure_valid_ref_format () {\n> die \"fatal: '$1' does not look like a ref\"\n> }\n>\n> +# Usage: check if a commit from another subtree should be\n> +# ignored from processing for splits\n> +should_ignore_subtree_split_commit () {\n\nMaybe adding:\n\n assert test $# = 1\n local rev=\"$1\"\n\nhere, and using $rev instead of $1 in this function could make things\na bit clearer and similar to what is done in other functions.\n\n> + if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n> + then\n> + if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n> + test -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n> + then\n> + return 0\n> + fi\n> + fi\n> + return 1\n> +}\n\nThe above doesn't seem to be properly indented. We use tabs not spaces.\n\n> # Usage: process_split_commit REV PARENTS\n> process_split_commit () {\n> assert test $# = 2\n> @@ -963,7 +977,20 @@ cmd_split () {\n> eval \"$grl\" |\n> while read rev parents\n> do\n> - process_split_commit \"$rev\" \"$parents\"\n> + if should_ignore_subtree_split_commit \"$rev\"\n> + then\n> + continue\n> + fi\n> + parsedParents=''\n\nIt seems to me that we name variables \"parsed_parents\" (or sometimes\n\"parsedparents\") rather than \"parsedParents\".\n\n> + for parent in $parents\n> + do\n> + should_ignore_subtree_split_commit \"$parent\"\n> + if test $? -eq 1\n\nI think the 2 lines above could be replaced by:\n\n+ if ! should_ignore_subtree_split_commit \"$parent\"\n\n> + then\n> + parsedParents+=\"$parent \"\n\nIt doesn't seem to me that we use \"+=\" much in our shell scripts.\nhttps://www.shellcheck.net/ emits the following:\n\n(warning): In POSIX sh, += is undefined.\n\nso I guess we don't use it because it's not available in some usual shells.\n\n(I haven't checked the script with https://www.shellcheck.net/ before\nand after your patch, but it might help avoid bash-isms and such\nissues.)\n\n> + fi\n> + done\n> + process_split_commit \"$rev\" \"$parsedParents\"\n> done || exit $?\n\nIt looks like we use \"exit $?\" a lot in git-subtree.sh while we use\njust \"exit\" most often elsewhere. Not sure why.\n\n> latest_new=$(cache_get latest_new) || exit $?\n> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> index 49a21dd7c9c..87d59afd761 100755\n> --- a/contrib/subtree/t/t7900-subtree.sh\n> +++ b/contrib/subtree/t/t7900-subtree.sh\n> @@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '\n> )\n> '\n>\n> +test_expect_success 'split with multiple subtrees' '\n> + subtree_test_create_repo \"$test_count\" &&\n> + subtree_test_create_repo \"$test_count/subA\" &&\n> + subtree_test_create_repo \"$test_count/subB\" &&\n> + test_create_commit \"$test_count\" main1 &&\n> + test_create_commit \"$test_count/subA\" subA1 &&\n> + test_create_commit \"$test_count/subA\" subA2 &&\n> + test_create_commit \"$test_count/subA\" subA3 &&\n> + test_create_commit \"$test_count/subB\" subB1 &&\n> + (\n> + cd \"$test_count\" &&\n> + git fetch ./subA HEAD &&\n> + git subtree add --prefix=subADir FETCH_HEAD\n> + ) &&\n> + (\n> + cd \"$test_count\" &&\n> + git fetch ./subB HEAD &&\n> + git subtree add --prefix=subBDir FETCH_HEAD\n> + ) &&\n> + test_create_commit \"$test_count\" subADir/main-subA1 &&\n> + test_create_commit \"$test_count\" subBDir/main-subB1 &&\n> + (\n> + cd \"$test_count\" &&\n> + git subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 1\"\n> + ) &&\n\nNot sure why there are so many sub-shells used, and why the -C option\nis not used instead to tell Git to work in a subdirectory. I guess you\ncopied what most existing (old) tests in this test script do.\n\nFor example perhaps the 4 line above could be replaced by just:\n\n+ git -C \"$test_count\" subtree split --prefix=subADir\n--squash --rejoin -m \"Sub A Split 1\" &&\n\n> + (\n> + cd \"$test_count\" &&\n> + git subtree split --prefix=subBDir --squash --rejoin -m \"Sub B Split 1\"\n> + ) &&\n> + test_create_commit \"$test_count\" subADir/main-subA2 &&\n> + test_create_commit \"$test_count\" subBDir/main-subB2 &&\n> + (\n> + cd \"$test_count\" &&\n> + git subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 2\"\n> + ) &&\n> + (\n> + cd \"$test_count\" &&\n> + test \"$(git subtree split --prefix=subBDir --squash --rejoin \\\n> + -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n> + )\n> +'\n\nIt's not clear to me what the test is doing. Maybe you could split it\ninto 2 tests. Perhaps one setting up a repo with multiple subtrees and\none checking that a new split ignores other subtree split commits.\nPerhaps adding a few comments would help too.\n\nBest,\nChristian.\n"},{"id":"485223","messageId":"CAEWN6q3BHECpJtfr-ZiGoJtpt8n65h2r+DKsu8Yg2ZWb8_SgKQ@mail.gmail.com","threadId":"60241","inReplyTo":"CAP8UFD18Hh=m8HQibAgZW1KNAn6zg_rxe9asg0ViC5z27W=Smw@mail.gmail.com","subject":"Re: [PATCH v4] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore","fromEmail":"zach.fetters@apollographql.com","sentAt":"2023-11-28T21:04:28Z","receivedAt":"2023-11-28T21:04:40Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":">> In the diagram below, 'M' represents the mainline repo branch, 'A'\n>> represents one subtree, and 'B' represents another. M3 and B1 represent\n>> a split commit for subtree B that was created from commit M4. M2 and A1\n>> represent a split commit made from subtree A that was also created\n>> based on changes back to and including M4. M1 represents new changes to\n>> the repo, in this scenario if you try to run a 'git subtree split\n>> --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n>> the processing of changes for the new split commit since the last\n>> split/rejoin for subtree B was at M3. The issue is that by having A1\n>> included in this processing the command ends up needing to processing\n>> every commit down tree A even though none of that is needed or relevant\n>> to the current command and result.\n>>\n>> M1\n>> | \\ \\\n>> M2 | |\n>> | A1 |\n>> M3 | |\n>> | | B1\n>> M4 | |\n\n> About the above, Junio already commented the following:\n>\n> -> The above paragraph explains which different things you drew in the\n> -> diagram are representing, but it is not clear how they relate to\n> -> each other. Do they for example depict parent-child commit\n> -> relationship? What are the wide gaps between these three tracks and\n> -> what are the short angled lines leaning to the left near the tip?\n> -> Is the time/topology flowing from bottom to top?\n>\n> and it doesn't look like you have addressed that comment.\n>\n> When you say \"M3 and B1 represent a split commit for subtree B that\n> was created from commit M4.\" I am not sure what it means exactly.\n> Could you give example commands that could have created the M3 and B1\n> commits?\n\nI removed the diagram from the commit message since it seems a little\nunclear, and in its place I added an example of an open source repo\n(which I am currently using the fix in) and the commands to replicate\nthe issue. Hopefully that better illustrates how I came across the issue\nand what it is.\n\n>> So this commit makes a change to the processing of commits for the split\n>> command in order to ignore non-mainline commits from other subtrees such\n>> as A1 in the diagram by adding a new function\n>> 'should_ignore_subtree_commit' which is called during\n>> 'process_split_commit'. This allows the split/rejoin processing to still\n>> function as expected but removes all of the unnecessary processing that\n>> takes place currently which greatly inflates the processing time.\n\n> Could you tell a bit more what kind of processing time reduction is or\n> would be possible on what kind of repo? Have you benchmark-ed or just\n> timed this somehow on one of your repos or better on an open source\n> repo (so that we could reproduce if we wanted)?\n\nI added some extra info for this to the commit message as well, but to\nanswer your question yes I discovered and benchmarked this issue in a\nrepo I help maintain. I was seeing splits take upwards of 12 minutes\nbefore the fix, and after they were taking only seconds. Also provided\ninfor on the repo and how to reproduce in the updated commit message.\n\n>> Added a test to validate that the proposed fix\n>> solves the issue.\n>>\n>> The test accomplishes this by checking the output\n>> of the split command to ensure the output from\n>> the progress of 'process_split_commit' function\n>> that represents the 'extracount' of commits\n>> processed does not increment.\n\n> Does not increment compared to what?\n\nI reworded this to say the 'extracount' remains at 0 since\nthere should be no extra processed commits from the second subtree\nin the test.\n\n>> This was tested against the original functionality\n>> to show the test failed, and then with this fix\n>> to show the test passes.\n>>\n>> This illustrated that when using multiple subtrees,\n>> A and B, when doing a split on subtree B, the\n>> processing does not traverse the entire history\n>> of subtree A which is unnecessary and would cause\n>> the 'extracount' of processed commits to climb\n>> based on the number of commits in the history of\n>> subtree A.\n\n> Does this mean that the test checks that the extracount is the same\n> when subtree A exists as when it doesn't exist?\n\nThis means the test is checking that the 'extracount' remains at\n0, because anything above 0 would mean commits from subtree A were\nbeing processed, which is where the issue stems from.\n\n>> contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-\n>> contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++\n>> 2 files changed, 70 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree=\n>.sh\n>> index e0c5d3b0de6..e69991a9d80 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -778,6 +778,20 @@ ensure_valid_ref_format () {\n>> die \"fatal: '$1' does not look like a ref\"\n>> }\n>>\n>> +# Usage: check if a commit from another subtree should be\n>> +# ignored from processing for splits\n>> +should_ignore_subtree_split_commit () {\n\n> Maybe adding:\n>\n> assert test $# =3D 1\n> local rev=3D\"$1\"\n>\n> here, and using $rev instead of $1 in this function could make things\n> a bit clearer and similar to what is done in other functions.\n\nUpdated.\n\n>> + if test -n \"$(git log -1 --grep=3D\"git-subtree-dir:\" $1)\"\n>> + then\n>> + if test -z \"$(git log -1 --grep=3D\"git-subtree-mainline:\" $1)\" &&\n>> + test -z \"$(git log -1 --grep=3D\"git-subtree-dir: =\n>> $arg_prefix$\" $1)\"\n>> + then\n>> + return 0\n>> + fi\n>> + fi\n>> + return 1\n>> +}\n\n> The above doesn't seem to be properly indented. We use tabs not spaces.\n\nFixed.\n\n>> # Usage: process_split_commit REV PARENTS\n>> process_split_commit () {\n>> assert test $# =3D 2\n>> @@ -963,7 +977,20 @@ cmd_split () {\n>> eval \"$grl\" |\n>> while read rev parents\n>> do\n>> - process_split_commit \"$rev\" \"$parents\"\n>> + if should_ignore_subtree_split_commit \"$rev\"\n>> + then\n>> + continue\n>> + fi\n>> + parsedParents=3D''\n\n> It seems to me that we name variables \"parsed_parents\" (or sometimes\n> \"parsedparents\") rather than \"parsedParents\".\n\nFixed.\n\n>> + for parent in $parents\n>> + do\n>> + should_ignore_subtree_split_commit \"$parent\"\n>> + if test $? -eq 1\n\n> I think the 2 lines above could be replaced by:\n>\n> + if ! should_ignore_subtree_split_commit \"$parent\"\n\nUpdated.\n\n>> + then\n>> + parsedParents+=3D\"$parent \"\n\n> It doesn't seem to me that we use \"+=3D\" much in our shell scripts.\n> https://www.shellcheck.net/ emits the following:\n>\n> (warning): In POSIX sh, +=3D is undefined.\n>\n> so I guess we don't use it because it's not available in some usual shells.\n>\n> (I haven't checked the script with https://www.shellcheck.net/ before\n> and after your patch, but it might help avoid bash-isms and such\n> issues.)\n\nUpdated this to remove the '+=' usage.\n\n>> + fi\n>> + done\n>> + process_split_commit \"$rev\" \"$parsedParents\"\n>> done || exit $?\n\n> It looks like we use \"exit $?\" a lot in git-subtree.sh while we use\n> just \"exit\" most often elsewhere. Not sure why.\n\nYea I am unsure of the reasoning of that, I was just trying to follow the\nwhat the existing script was already doing.\n\n>> latest_new=3D$(cache_get latest_new) || exit $?\n>> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900=\n>> -subtree.sh\n>> index 49a21dd7c9c..87d59afd761 100755\n>> --- a/contrib/subtree/t/t7900-subtree.sh\n>> +++ b/contrib/subtree/t/t7900-subtree.sh\n>> @@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '\n>> )\n>> '\n>>\n>> +test_expect_success 'split with multiple subtrees' '\n>> + subtree_test_create_repo \"$test_count\" &&\n>> + subtree_test_create_repo \"$test_count/subA\" &&\n>> + subtree_test_create_repo \"$test_count/subB\" &&\n>> + test_create_commit \"$test_count\" main1 &&\n>> + test_create_commit \"$test_count/subA\" subA1 &&\n>> + test_create_commit \"$test_count/subA\" subA2 &&\n>> + test_create_commit \"$test_count/subA\" subA3 &&\n>> + test_create_commit \"$test_count/subB\" subB1 &&\n>> + (\n>> + cd \"$test_count\" &&\n>> + git fetch ./subA HEAD &&\n>> + git subtree add --prefix=3DsubADir FETCH_HEAD\n>> + ) &&\n>> + (\n>> + cd \"$test_count\" &&\n>> + git fetch ./subB HEAD &&\n>> + git subtree add --prefix=3DsubBDir FETCH_HEAD\n>> + ) &&\n>> + test_create_commit \"$test_count\" subADir/main-subA1 &&\n>> + test_create_commit \"$test_count\" subBDir/main-subB1 &&\n>> + (\n>> + cd \"$test_count\" &&\n>> + git subtree split --prefix=3DsubADir --squash --rejoin -m=\n>> \"Sub A Split 1\"\n>> + ) &&\n\n> Not sure why there are so many sub-shells used, and why the -C option\n> is not used instead to tell Git to work in a subdirectory. I guess you\n> copied what most existing (old) tests in this test script do.\n>\n> For example perhaps the 4 line above could be replaced by just:\n>\n> + git -C \"$test_count\" subtree split --prefix=3DsubADir\n> --squash --rejoin -m \"Sub A Split 1\" &&\n\nYea I was following what was being done in other existing tests, although\nthis seems like a better way to do this so I updated the test to remove\nthe extra sub-shells.\n\n>> + (\n>> + cd \"$test_count\" &&\n>> + git subtree split --prefix=3DsubBDir --squash --rejoin -m=\n>> \"Sub B Split 1\"\n>> + ) &&\n>> + test_create_commit \"$test_count\" subADir/main-subA2 &&\n>> + test_create_commit \"$test_count\" subBDir/main-subB2 &&\n>> + (\n>> + cd \"$test_count\" &&\n>> + git subtree split --prefix=3DsubADir --squash --rejoin -m=\n>> \"Sub A Split 2\"\n>> + ) &&\n>> + (\n>> + cd \"$test_count\" &&\n>> + test \"$(git subtree split --prefix=3DsubBDir --squash --r=\n>> ejoin \\\n>> + -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" =3D \"\"\n>> + )\n>> +'\n\n> It's not clear to me what the test is doing. Maybe you could split it\n> into 2 tests. Perhaps one setting up a repo with multiple subtrees and\n> one checking that a new split ignores other subtree split commits.\n> Perhaps adding a few comments would help too.\n\nAdded some comments before the test to describe the steps the test is taking in\norder to verify the desired behavior.\n\n\nOn Sat, Nov 18, 2023 at 6:28 AM Christian Couder\n wrote:\n>\n> On Thu, Oct 26, 2023 at 9:18 PM Zach FettersMoore via GitGitGadget\n> wrote:\n> >\n> > From: Zach FettersMoore \n> >\n> > When there are multiple subtrees present in a repository and they are\n> > all using 'git subtree split', the 'split' command can take a\n> > significant (and constantly growing) amount of time to run even when\n> > using the '--rejoin' flag. This is due to the fact that when processing\n> > commits to determine the last known split to start from when looking\n> > for changes, if there has been a split/merge done from another subtree\n> > there will be 2 split commits, one mainline and one subtree, for the\n> > second subtree that are part of the processing. The non-mainline\n> > subtree split commit will cause the processing to always need to search\n> > the entire history of the given subtree as part of its processing even\n> > though those commits are totally irrelevant to the current subtree\n> > split being run.\n>\n> Thanks for your continued work on this!\n>\n> I am not familiar with git subtree so I might miss obvious things. On\n> the other hand, my comments might help increase a bit the number of\n> people who could review this patch.\n>\n> > In the diagram below, 'M' represents the mainline repo branch, 'A'\n> > represents one subtree, and 'B' represents another. M3 and B1 represent\n> > a split commit for subtree B that was created from commit M4. M2 and A1\n> > represent a split commit made from subtree A that was also created\n> > based on changes back to and including M4. M1 represents new changes to\n> > the repo, in this scenario if you try to run a 'git subtree split\n> > --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n> > the processing of changes for the new split commit since the last\n> > split/rejoin for subtree B was at M3. The issue is that by having A1\n> > included in this processing the command ends up needing to processing\n> > every commit down tree A even though none of that is needed or relevant\n> > to the current command and result.\n> >\n> > M1\n> > | \\ \\\n> > M2 | |\n> > | A1 |\n> > M3 | |\n> > | | B1\n> > M4 | |\n>\n> About the above, Junio already commented the following:\n>\n> -> The above paragraph explains which different things you drew in the\n> -> diagram are representing, but it is not clear how they relate to\n> -> each other. Do they for example depict parent-child commit\n> -> relationship? What are the wide gaps between these three tracks and\n> -> what are the short angled lines leaning to the left near the tip?\n> -> Is the time/topology flowing from bottom to top?\n>\n> and it doesn't look like you have addressed that comment.\n>\n> When you say \"M3 and B1 represent a split commit for subtree B that\n> was created from commit M4.\" I am not sure what it means exactly.\n> Could you give example commands that could have created the M3 and B1\n> commits?\n>\n> > So this commit makes a change to the processing of commits for the split\n> > command in order to ignore non-mainline commits from other subtrees such\n> > as A1 in the diagram by adding a new function\n> > 'should_ignore_subtree_commit' which is called during\n> > 'process_split_commit'. This allows the split/rejoin processing to still\n> > function as expected but removes all of the unnecessary processing that\n> > takes place currently which greatly inflates the processing time.\n>\n> Could you tell a bit more what kind of processing time reduction is or\n> would be possible on what kind of repo? Have you benchmark-ed or just\n> timed this somehow on one of your repos or better on an open source\n> repo (so that we could reproduce if we wanted)?\n>\n> > Added a test to validate that the proposed fix\n> > solves the issue.\n> >\n> > The test accomplishes this by checking the output\n> > of the split command to ensure the output from\n> > the progress of 'process_split_commit' function\n> > that represents the 'extracount' of commits\n> > processed does not increment.\n>\n> Does not increment compared to what?\n>\n> > This was tested against the original functionality\n> > to show the test failed, and then with this fix\n> > to show the test passes.\n> >\n> > This illustrated that when using multiple subtrees,\n> > A and B, when doing a split on subtree B, the\n> > processing does not traverse the entire history\n> > of subtree A which is unnecessary and would cause\n> > the 'extracount' of processed commits to climb\n> > based on the number of commits in the history of\n> > subtree A.\n>\n> Does this mean that the test checks that the extracount is the same\n> when subtree A exists as when it doesn't exist?\n>\n> [...]\n>\n> > contrib/subtree/git-subtree.sh | 29 ++++++++++++++++++++-\n> > contrib/subtree/t/t7900-subtree.sh | 42 ++++++++++++++++++++++++++++++\n> > 2 files changed, 70 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> > index e0c5d3b0de6..e69991a9d80 100755\n> > --- a/contrib/subtree/git-subtree.sh\n> > +++ b/contrib/subtree/git-subtree.sh\n> > @@ -778,6 +778,20 @@ ensure_valid_ref_format () {\n> > die \"fatal: '$1' does not look like a ref\"\n> > }\n> >\n> > +# Usage: check if a commit from another subtree should be\n> > +# ignored from processing for splits\n> > +should_ignore_subtree_split_commit () {\n>\n> Maybe adding:\n>\n> assert test $# = 1\n> local rev=\"$1\"\n>\n> here, and using $rev instead of $1 in this function could make things\n> a bit clearer and similar to what is done in other functions.\n>\n> > + if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n> > + then\n> > + if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n> > + test -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n> > + then\n> > + return 0\n> > + fi\n> > + fi\n> > + return 1\n> > +}\n>\n> The above doesn't seem to be properly indented. We use tabs not spaces.\n>\n> > # Usage: process_split_commit REV PARENTS\n> > process_split_commit () {\n> > assert test $# = 2\n> > @@ -963,7 +977,20 @@ cmd_split () {\n> > eval \"$grl\" |\n> > while read rev parents\n> > do\n> > - process_split_commit \"$rev\" \"$parents\"\n> > + if should_ignore_subtree_split_commit \"$rev\"\n> > + then\n> > + continue\n> > + fi\n> > + parsedParents=''\n>\n> It seems to me that we name variables \"parsed_parents\" (or sometimes\n> \"parsedparents\") rather than \"parsedParents\".\n>\n> > + for parent in $parents\n> > + do\n> > + should_ignore_subtree_split_commit \"$parent\"\n> > + if test $? -eq 1\n>\n> I think the 2 lines above could be replaced by:\n>\n> + if ! should_ignore_subtree_split_commit \"$parent\"\n>\n> > + then\n> > + parsedParents+=\"$parent \"\n>\n> It doesn't seem to me that we use \"+=\" much in our shell scripts.\n> https://www.shellcheck.net/ emits the following:\n>\n> (warning): In POSIX sh, += is undefined.\n>\n> so I guess we don't use it because it's not available in some usual shells.\n>\n> (I haven't checked the script with https://www.shellcheck.net/ before\n> and after your patch, but it might help avoid bash-isms and such\n> issues.)\n>\n> > + fi\n> > + done\n> > + process_split_commit \"$rev\" \"$parsedParents\"\n> > done || exit $?\n>\n> It looks like we use \"exit $?\" a lot in git-subtree.sh while we use\n> just \"exit\" most often elsewhere. Not sure why.\n>\n> > latest_new=$(cache_get latest_new) || exit $?\n> > diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\n> > index 49a21dd7c9c..87d59afd761 100755\n> > --- a/contrib/subtree/t/t7900-subtree.sh\n> > +++ b/contrib/subtree/t/t7900-subtree.sh\n> > @@ -385,6 +385,48 @@ test_expect_success 'split sub dir/ with --rejoin' '\n> > )\n> > '\n> >\n> > +test_expect_success 'split with multiple subtrees' '\n> > + subtree_test_create_repo \"$test_count\" &&\n> > + subtree_test_create_repo \"$test_count/subA\" &&\n> > + subtree_test_create_repo \"$test_count/subB\" &&\n> > + test_create_commit \"$test_count\" main1 &&\n> > + test_create_commit \"$test_count/subA\" subA1 &&\n> > + test_create_commit \"$test_count/subA\" subA2 &&\n> > + test_create_commit \"$test_count/subA\" subA3 &&\n> > + test_create_commit \"$test_count/subB\" subB1 &&\n> > + (\n> > + cd \"$test_count\" &&\n> > + git fetch ./subA HEAD &&\n> > + git subtree add --prefix=subADir FETCH_HEAD\n> > + ) &&\n> > + (\n> > + cd \"$test_count\" &&\n> > + git fetch ./subB HEAD &&\n> > + git subtree add --prefix=subBDir FETCH_HEAD\n> > + ) &&\n> > + test_create_commit \"$test_count\" subADir/main-subA1 &&\n> > + test_create_commit \"$test_count\" subBDir/main-subB1 &&\n> > + (\n> > + cd \"$test_count\" &&\n> > + git subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 1\"\n> > + ) &&\n>\n> Not sure why there are so many sub-shells used, and why the -C option\n> is not used instead to tell Git to work in a subdirectory. I guess you\n> copied what most existing (old) tests in this test script do.\n>\n> For example perhaps the 4 line above could be replaced by just:\n>\n> + git -C \"$test_count\" subtree split --prefix=subADir\n> --squash --rejoin -m \"Sub A Split 1\" &&\n>\n> > + (\n> > + cd \"$test_count\" &&\n> > + git subtree split --prefix=subBDir --squash --rejoin -m \"Sub B Split 1\"\n> > + ) &&\n> > + test_create_commit \"$test_count\" subADir/main-subA2 &&\n> > + test_create_commit \"$test_count\" subBDir/main-subB2 &&\n> > + (\n> > + cd \"$test_count\" &&\n> > + git subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 2\"\n> > + ) &&\n> > + (\n> > + cd \"$test_count\" &&\n> > + test \"$(git subtree split --prefix=subBDir --squash --rejoin \\\n> > + -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n> > + )\n> > +'\n>\n> It's not clear to me what the test is doing. Maybe you could split it\n> into 2 tests. Perhaps one setting up a repo with multiple subtrees and\n> one checking that a new split ignores other subtree split commits.\n> Perhaps adding a few comments would help too.\n>\n> Best,\n> Christian.\n"},{"id":"485224","messageId":"pull.1587.v5.git.1701206267300.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v4.git.1698347871200.gitgitgadget@gmail.com","subject":"[PATCH v5] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-11-28T21:17:47Z","receivedAt":"2023-11-28T21:17:50Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nTo see this in practice you can use the open source GitHub repo\n'apollo-ios-dev' and do the following in order:\n\n-Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'\n directories\n-Create a commit containing these changes\n-Do a split on apollo-ios-codegen\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n-Do a split on apollo-ios\n - git subtree split --prefix=apollo-ios --squash --rejoin\n-Make changes to a file in apollo-ios-codegen\n-Create a commit containing the change(s)\n-Do a split on apollo-ios-codegen\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n\nYou will see that the final split is looking for the last split\non apollo-ios-codegen to use as it's starting point to process\ncommits. Since there is a split commit from apollo-ios in between the\n2 splits run on apollo-ios-codegen, the processing ends up traversing\nthe entire history of apollo-ios which increases the time it takes to\ndo a split based on how long of a history apollo-ios has, while none\nof these commits are relevant to the split being done on\napollo-ios-codegen.\n\nSo this commit makes a change to the processing of commits for the\nsplit command in order to ignore non-mainline commits from other\nsubtrees such as apollo-ios in the above breakdown by adding a new\nfunction 'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to\nstill function as expected but removes all of the unnecessary\nprocessing that takes place currently which greatly inflates the\nprocessing time. In the above example, previously the final split\nwould take ~10-12 minutes, while after this fix it takes seconds.\n\nAdded a test to validate that the proposed fix\nsolves the issue.\n\nThe test accomplishes this by checking the output\nof the split command to ensure the output from\nthe progress of 'process_split_commit' function\nthat represents the 'extracount' of commits\nprocessed remains at 0, meaning none of the commits\nfrom the second subtree were processed.\n\nThis was tested against the original functionality\nto show the test failed, and then with this fix\nto show the test passes.\n\nThis illustrated that when using multiple subtrees,\nA and B, when doing a split on subtree B, the\nprocessing does not traverse the entire history\nof subtree A which is unnecessary and would cause\nthe 'extracount' of processed commits to climb\nbased on the number of commits in the history of\nsubtree A.\n\nSigned-off-by: Zach FettersMoore \n---\n subtree: fix split processing with multiple subtrees present\n \n When there are multiple subtrees in a repo and git subtree split\n --rejoin is being used for the subtrees, the processing of commits for a\n new split can take a significant (and constantly growing) amount of time\n because the split commits from other subtrees cause the processing to\n have to scan the entire history of the other subtree(s). This patch\n filters out the other subtree split commits that are unnecessary for the\n split commit processing.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\nRange-diff vs v4:\n\n 1: 353152910eb ! 1: e7445a95f30 subtree: fix split processing with multiple subtrees present\n @@ Commit message\n though those commits are totally irrelevant to the current subtree\n split being run.\n \n - In the diagram below, 'M' represents the mainline repo branch, 'A'\n - represents one subtree, and 'B' represents another. M3 and B1 represent\n - a split commit for subtree B that was created from commit M4. M2 and A1\n - represent a split commit made from subtree A that was also created\n - based on changes back to and including M4. M1 represents new changes to\n - the repo, in this scenario if you try to run a 'git subtree split\n - --rejoin' for subtree B, commits M1, M2, and A1, will be included in\n - the processing of changes for the new split commit since the last\n - split/rejoin for subtree B was at M3. The issue is that by having A1\n - included in this processing the command ends up needing to processing\n - every commit down tree A even though none of that is needed or relevant\n - to the current command and result.\n + To see this in practice you can use the open source GitHub repo\n + 'apollo-ios-dev' and do the following in order:\n \n - M1\n - | \\ \\\n - M2 | |\n - | A1 |\n - M3 | |\n - | | B1\n - M4 | |\n + -Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'\n + directories\n + -Create a commit containing these changes\n + -Do a split on apollo-ios-codegen\n + - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n + -Do a split on apollo-ios\n + - git subtree split --prefix=apollo-ios --squash --rejoin\n + -Make changes to a file in apollo-ios-codegen\n + -Create a commit containing the change(s)\n + -Do a split on apollo-ios-codegen\n + - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n \n - So this commit makes a change to the processing of commits for the split\n - command in order to ignore non-mainline commits from other subtrees such\n - as A1 in the diagram by adding a new function\n - 'should_ignore_subtree_commit' which is called during\n - 'process_split_commit'. This allows the split/rejoin processing to still\n - function as expected but removes all of the unnecessary processing that\n - takes place currently which greatly inflates the processing time.\n + You will see that the final split is looking for the last split\n + on apollo-ios-codegen to use as it's starting point to process\n + commits. Since there is a split commit from apollo-ios in between the\n + 2 splits run on apollo-ios-codegen, the processing ends up traversing\n + the entire history of apollo-ios which increases the time it takes to\n + do a split based on how long of a history apollo-ios has, while none\n + of these commits are relevant to the split being done on\n + apollo-ios-codegen.\n +\n + So this commit makes a change to the processing of commits for the\n + split command in order to ignore non-mainline commits from other\n + subtrees such as apollo-ios in the above breakdown by adding a new\n + function 'should_ignore_subtree_commit' which is called during\n + 'process_split_commit'. This allows the split/rejoin processing to\n + still function as expected but removes all of the unnecessary\n + processing that takes place currently which greatly inflates the\n + processing time. In the above example, previously the final split\n + would take ~10-12 minutes, while after this fix it takes seconds.\n \n Added a test to validate that the proposed fix\n solves the issue.\n @@ Commit message\n of the split command to ensure the output from\n the progress of 'process_split_commit' function\n that represents the 'extracount' of commits\n - processed does not increment.\n + processed remains at 0, meaning none of the commits\n + from the second subtree were processed.\n \n This was tested against the original functionality\n to show the test failed, and then with this fix\n @@ contrib/subtree/git-subtree.sh: 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 -+ if test -n \"$(git log -1 --grep=\"git-subtree-dir:\" $1)\"\n -+ then\n -+ if test -z \"$(git log -1 --grep=\"git-subtree-mainline:\" $1)\" &&\n -+\t\t\ttest -z \"$(git log -1 --grep=\"git-subtree-dir: $arg_prefix$\" $1)\"\n -+ then\n -+ return 0\n -+ fi\n -+ fi\n -+ return 1\n ++\tassert test $# = 1\n ++\tlocal rev=\"$1\"\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\tthen\n ++\t\t\treturn 0\n ++\t\tfi\n ++\tfi\n ++\treturn 1\n +}\n +\n # Usage: process_split_commit REV PARENTS\n @@ contrib/subtree/git-subtree.sh: cmd_split () {\n +\t\tthen\n +\t\t\tcontinue\n +\t\tfi\n -+\t\tparsedParents=''\n ++\t\tparsedparents=''\n +\t\tfor parent in $parents\n +\t\tdo\n -+\t\t\tshould_ignore_subtree_split_commit \"$parent\"\n -+\t\t\tif test $? -eq 1\n ++\t\t\tif ! should_ignore_subtree_split_commit \"$parent\"\n +\t\t\tthen\n -+\t\t\t\tparsedParents+=\"$parent \"\n ++\t\t\t\tparsedparents=\"$parsedparents$parent \"\n +\t\t\tfi\n +\t\tdone\n -+\t\tprocess_split_commit \"$rev\" \"$parsedParents\"\n ++\t\tprocess_split_commit \"$rev\" \"$parsedparents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\n @@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r\n \t)\n '\n \n ++# Tests that commits from other subtrees are not processed as\n ++# part of a split.\n ++#\n ++# This test performs the following:\n ++# - Creates Repo with subtrees 'subA' and 'subB'\n ++# - Creates commits in the repo including changes to subtrees\n ++# - Runs the following 'split' and commit' commands in order:\n ++# \t- Perform 'split' on subtree A\n ++# \t- Perform 'split' on subtree B\n ++# \t- Create new commits with changes to subtree A and B\n ++# \t- Perform split on subtree A\n ++# \t- Check that the commits in subtree B are not processed\n ++#\t\t\tas part of the subtree A split\n +test_expect_success 'split with multiple subtrees' '\n +\tsubtree_test_create_repo \"$test_count\" &&\n +\tsubtree_test_create_repo \"$test_count/subA\" &&\n @@ contrib/subtree/t/t7900-subtree.sh: test_expect_success 'split sub dir/ with --r\n +\ttest_create_commit \"$test_count/subA\" subA2 &&\n +\ttest_create_commit \"$test_count/subA\" subA3 &&\n +\ttest_create_commit \"$test_count/subB\" subB1 &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\tgit fetch ./subA HEAD &&\n -+\t\tgit subtree add --prefix=subADir FETCH_HEAD\n -+\t) &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\tgit fetch ./subB HEAD &&\n -+\t\tgit subtree add --prefix=subBDir FETCH_HEAD\n -+\t) &&\n ++\tgit -C \"$test_count\" fetch ./subA HEAD &&\n ++\tgit -C \"$test_count\" subtree add --prefix=subADir FETCH_HEAD &&\n ++\tgit -C \"$test_count\" fetch ./subB HEAD &&\n ++\tgit -C \"$test_count\" subtree add --prefix=subBDir FETCH_HEAD &&\n +\ttest_create_commit \"$test_count\" subADir/main-subA1 &&\n +\ttest_create_commit \"$test_count\" subBDir/main-subB1 &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 1\"\n -+\t) &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\tgit subtree split --prefix=subBDir --squash --rejoin -m \"Sub B Split 1\"\n -+\t) &&\n ++\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n ++\t\t--squash --rejoin -m \"Sub A Split 1\" &&\n ++\tgit -C \"$test_count\" subtree split --prefix=subBDir \\\n ++\t\t--squash --rejoin -m \"Sub B Split 1\" &&\n +\ttest_create_commit \"$test_count\" subADir/main-subA2 &&\n +\ttest_create_commit \"$test_count\" subBDir/main-subB2 &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\tgit subtree split --prefix=subADir --squash --rejoin -m \"Sub A Split 2\"\n -+\t) &&\n -+\t(\n -+\t\tcd \"$test_count\" &&\n -+\t\ttest \"$(git subtree split --prefix=subBDir --squash --rejoin \\\n -+\t\t -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n -+\t)\n ++\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n ++\t\t--squash --rejoin -m \"Sub A Split 2\" &&\n ++\ttest \"$(git -C \"$test_count\" subtree split --prefix=subBDir \\\n ++\t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n +'\n +\n test_expect_success 'split sub dir/ with --rejoin from scratch' '\n\n\n contrib/subtree/git-subtree.sh | 30 +++++++++++++++++++++-\n contrib/subtree/t/t7900-subtree.sh | 40 ++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..a0bf958ea66 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,6 +778,22 @@ ensure_valid_ref_format () {\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+# 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+\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+\tfi\n+\treturn 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n@@ -963,7 +979,19 @@ cmd_split () {\n \teval \"$grl\" |\n \twhile read rev parents\n \tdo\n-\t\tprocess_split_commit \"$rev\" \"$parents\"\n+\t\tif should_ignore_subtree_split_commit \"$rev\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\t\tparsedparents=''\n+\t\tfor parent in $parents\n+\t\tdo\n+\t\t\tif ! should_ignore_subtree_split_commit \"$parent\"\n+\t\t\tthen\n+\t\t\t\tparsedparents=\"$parsedparents$parent \"\n+\t\t\tfi\n+\t\tdone\n+\t\tprocess_split_commit \"$rev\" \"$parsedparents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 49a21dd7c9c..ca4df5be832 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -385,6 +385,46 @@ test_expect_success 'split sub dir/ with --rejoin' '\n \t)\n '\n \n+# Tests that commits from other subtrees are not processed as\n+# part of a split.\n+#\n+# This test performs the following:\n+# - Creates Repo with subtrees 'subA' and 'subB'\n+# - Creates commits in the repo including changes to subtrees\n+# - Runs the following 'split' and commit' commands in order:\n+# \t- Perform 'split' on subtree A\n+# \t- Perform 'split' on subtree B\n+# \t- Create new commits with changes to subtree A and B\n+# \t- Perform split on subtree A\n+# \t- Check that the commits in subtree B are not processed\n+#\t\t\tas part of the subtree A split\n+test_expect_success 'split with multiple subtrees' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/subA\" &&\n+\tsubtree_test_create_repo \"$test_count/subB\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/subA\" subA1 &&\n+\ttest_create_commit \"$test_count/subA\" subA2 &&\n+\ttest_create_commit \"$test_count/subA\" subA3 &&\n+\ttest_create_commit \"$test_count/subB\" subB1 &&\n+\tgit -C \"$test_count\" fetch ./subA HEAD &&\n+\tgit -C \"$test_count\" subtree add --prefix=subADir FETCH_HEAD &&\n+\tgit -C \"$test_count\" fetch ./subB HEAD &&\n+\tgit -C \"$test_count\" subtree add --prefix=subBDir FETCH_HEAD &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA1 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB1 &&\n+\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n+\t\t--squash --rejoin -m \"Sub A Split 1\" &&\n+\tgit -C \"$test_count\" subtree split --prefix=subBDir \\\n+\t\t--squash --rejoin -m \"Sub B Split 1\" &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA2 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB2 &&\n+\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n+\t\t--squash --rejoin -m \"Sub A Split 2\" &&\n+\ttest \"$(git -C \"$test_count\" subtree split --prefix=subBDir \\\n+\t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n+'\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: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n-- \ngitgitgadget\n"},{"id":"485292","messageId":"CAP8UFD1rd+q-dC_w2VgZ_jC++LDeF6gu5wDcbQzSuhU1ksfBpA@mail.gmail.com","threadId":"60241","inReplyTo":"pull.1587.v5.git.1701206267300.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-11-30T20:33:54Z","receivedAt":"2023-11-30T20:34:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Nov 28, 2023 at 10:17 PM Zach FettersMoore via GitGitGadget\n wrote:\n\n> To see this in practice you can use the open source GitHub repo\n> 'apollo-ios-dev' and do the following in order:\n>\n> -Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'\n\nIt looks like there is a spurious A after 'apollo-ios' in the line above.\n\n> directories\n> -Create a commit containing these changes\n> -Do a split on apollo-ios-codegen\n> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n\nI might be doing something stupid or wrong, but I get the following:\n\n$ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\nfatal: could not rev-parse split hash\ncc70a7d49e84696f0df210710445784c504ed748 from commit\n360f068ea0d57f250621ab7dbe205313f52a0e98\nhint: hash might be a tag, try fetching it from the subtree repository:\nhint: git fetch cc70a7d49e84696f0df210710445784c504ed748\n\n> -Do a split on apollo-ios\n> - git subtree split --prefix=apollo-ios --squash --rejoin\n\nSame issue:\n\n$ git subtree split --prefix=apollo-ios --squash --rejoin\nfatal: could not rev-parse split hash\nb852c0aa1fd5ab9e1323da92b606ad3f2211e111 from commit\nb48030c3eb6e2faf4bff981c5c63ca72aceecdfa\nhint: hash might be a tag, try fetching it from the subtree repository:\nhint: git fetch b852c0aa1fd5ab9e1323da92b606ad3f2211e111\n\nI didn't try to get farther than this, as it seems that some\ninstructions might be missing.\n\n[...]\n\n> So this commit makes a change to the processing of commits for the\n> split command in order to ignore non-mainline commits from other\n> subtrees such as apollo-ios in the above breakdown by adding a new\n> function 'should_ignore_subtree_commit' which is called during\n> 'process_split_commit'. This allows the split/rejoin processing to\n> still function as expected but removes all of the unnecessary\n> processing that takes place currently which greatly inflates the\n> processing time. In the above example, previously the final split\n> would take ~10-12 minutes, while after this fix it takes seconds.\n\nNice!\n\nExcept for the above issues in the commit message, the rest of the\npatch looks good to me, thanks!\n"},{"id":"485293","messageId":"CAEWN6q14sd-KmMOgmKWqnKWPNUeW7MN5T=PhrkkN5d0VXQEohA@mail.gmail.com","threadId":"60241","inReplyTo":"CAP8UFD1rd+q-dC_w2VgZ_jC++LDeF6gu5wDcbQzSuhU1ksfBpA@mail.gmail.com","subject":"Re: [PATCH v5] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore","fromEmail":"zach.fetters@apollographql.com","sentAt":"2023-11-30T21:01:27Z","receivedAt":"2023-11-30T21:01:39Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":">> To see this in practice you can use the open source GitHub repo\n>> 'apollo-ios-dev' and do the following in order:\n>>\n>> -Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'\n\n> It looks like there is a spurious A after 'apollo-ios' in the line above.\n\nThanks for catching that, definitely a typo on my part.\n\n>> directories\n>> -Create a commit containing these changes\n>> -Do a split on apollo-ios-codegen\n>> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n\n> I might be doing something stupid or wrong, but I get the following:\n>\n> $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> fatal: could not rev-parse split hash\n> cc70a7d49e84696f0df210710445784c504ed748 from commit\n> 360f068ea0d57f250621ab7dbe205313f52a0e98\n> hint: hash might be a tag, try fetching it from the subtree repository:\n> hint: git fetch cc70a7d49e84696f0df210710445784c504ed748\n\nUpdated this to include doing a fetch to ensure all remote repo\ninfo is available locally.\n\n>> -Do a split on apollo-ios\n>> - git subtree split --prefix=apollo-ios --squash --rejoin\n\n> Same issue:\n>\n> $ git subtree split --prefix=apollo-ios --squash --rejoin\n> fatal: could not rev-parse split hash\n> b852c0aa1fd5ab9e1323da92b606ad3f2211e111 from commit\n> b48030c3eb6e2faf4bff981c5c63ca72aceecdfa\n> hint: hash might be a tag, try fetching it from the subtree repository:\n> hint: git fetch b852c0aa1fd5ab9e1323da92b606ad3f2211e111\n>\n> I didn't try to get farther than this, as it seems that some\n> instructions might be missing.\n\nSame as above, added extra instruction to do a fetch first.\n\nAlso added a little extra info that the issue may present after the\nfirst split in the instructions depending on the current state of the\nrepo being used. Also added a way to do the same steps with the changes\napplied to see that it resolves the issue.\n\n>> So this commit makes a change to the processing of commits for the\n>> split command in order to ignore non-mainline commits from other\n>> subtrees such as apollo-ios in the above breakdown by adding a new\n>> function 'should_ignore_subtree_commit' which is called during\n>> 'process_split_commit'. This allows the split/rejoin processing to\n>> still function as expected but removes all of the unnecessary\n>> processing that takes place currently which greatly inflates the\n>> processing time. In the above example, previously the final split\n>> would take ~10-12 minutes, while after this fix it takes seconds.\n\n> Nice!\n>\n> Except for the above issues in the commit message, the rest of the\n> patch looks good to me, thanks!\n\nGreat! Thanks for the review and guidance!\n"},{"id":"485306","messageId":"pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com","threadId":"60241","inReplyTo":"pull.1587.v5.git.1701206267300.gitgitgadget@gmail.com","subject":"[PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-12-01T14:54:54Z","receivedAt":"2023-12-01T14:54:57Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"From: Zach FettersMoore \n\nWhen there are multiple subtrees present in a repository and they are\nall using 'git subtree split', the 'split' command can take a\nsignificant (and constantly growing) amount of time to run even when\nusing the '--rejoin' flag. This is due to the fact that when processing\ncommits to determine the last known split to start from when looking\nfor changes, if there has been a split/merge done from another subtree\nthere will be 2 split commits, one mainline and one subtree, for the\nsecond subtree that are part of the processing. The non-mainline\nsubtree split commit will cause the processing to always need to search\nthe entire history of the given subtree as part of its processing even\nthough those commits are totally irrelevant to the current subtree\nsplit being run.\n\nTo see this in practice you can use the open source GitHub repo\n'apollo-ios-dev' and do the following in order:\n\n-Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n directories\n-Create a commit containing these changes\n-Do a split on apollo-ios-codegen\n - Do a fetch on the subtree repo\n - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n - Depending on the current state of the 'apollo-ios-dev' repo\n you may see the issue at this point if the last split was on\n apollo-ios\n-Do a split on apollo-ios\n - Do a fetch on the subtree repo\n - git fetch git@github.com:apollographql/apollo-ios.git\n - git subtree split --prefix=apollo-ios --squash --rejoin\n-Make changes to a file in apollo-ios-codegen\n-Create a commit containing the change(s)\n-Do a split on apollo-ios-codegen\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n-To see that the patch fixes the issue you can use the custom subtree\n script in the repo so following the same steps as above, except\n instead of using 'git subtree ...' for the commands use\n 'git-subtree.sh ...' for the commands\n\nYou will see that the final split is looking for the last split\non apollo-ios-codegen to use as it's starting point to process\ncommits. Since there is a split commit from apollo-ios in between the\n2 splits run on apollo-ios-codegen, the processing ends up traversing\nthe entire history of apollo-ios which increases the time it takes to\ndo a split based on how long of a history apollo-ios has, while none\nof these commits are relevant to the split being done on\napollo-ios-codegen.\n\nSo this commit makes a change to the processing of commits for the\nsplit command in order to ignore non-mainline commits from other\nsubtrees such as apollo-ios in the above breakdown by adding a new\nfunction 'should_ignore_subtree_commit' which is called during\n'process_split_commit'. This allows the split/rejoin processing to\nstill function as expected but removes all of the unnecessary\nprocessing that takes place currently which greatly inflates the\nprocessing time. In the above example, previously the final split\nwould take ~10-12 minutes, while after this fix it takes seconds.\n\nAdded a test to validate that the proposed fix\nsolves the issue.\n\nThe test accomplishes this by checking the output\nof the split command to ensure the output from\nthe progress of 'process_split_commit' function\nthat represents the 'extracount' of commits\nprocessed remains at 0, meaning none of the commits\nfrom the second subtree were processed.\n\nThis was tested against the original functionality\nto show the test failed, and then with this fix\nto show the test passes.\n\nThis illustrated that when using multiple subtrees,\nA and B, when doing a split on subtree B, the\nprocessing does not traverse the entire history\nof subtree A which is unnecessary and would cause\nthe 'extracount' of processed commits to climb\nbased on the number of commits in the history of\nsubtree A.\n\nSigned-off-by: Zach FettersMoore \n---\n subtree: fix split processing with multiple subtrees present\n \n When there are multiple subtrees in a repo and git subtree split\n --rejoin is being used for the subtrees, the processing of commits for a\n new split can take a significant (and constantly growing) amount of time\n because the split commits from other subtrees cause the processing to\n have to scan the entire history of the other subtree(s). This patch\n filters out the other subtree split commits that are unnecessary for the\n split commit processing.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1587%2FBobaFetters%2Fzf%2Fmulti-subtree-processing-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1587/BobaFetters/zf/multi-subtree-processing-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/1587\n\nRange-diff vs v5:\n\n 1: e7445a95f30 ! 1: 2a65ec0e4df subtree: fix split processing with multiple subtrees present\n @@ Commit message\n To see this in practice you can use the open source GitHub repo\n 'apollo-ios-dev' and do the following in order:\n \n - -Make a changes to a file in 'apollo-ios'A and 'apollo-ios-codegen'\n + -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n directories\n -Create a commit containing these changes\n -Do a split on apollo-ios-codegen\n + - Do a fetch on the subtree repo\n + - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n + - Depending on the current state of the 'apollo-ios-dev' repo\n + you may see the issue at this point if the last split was on\n + apollo-ios\n -Do a split on apollo-ios\n + - Do a fetch on the subtree repo\n + - git fetch git@github.com:apollographql/apollo-ios.git\n - git subtree split --prefix=apollo-ios --squash --rejoin\n -Make changes to a file in apollo-ios-codegen\n -Create a commit containing the change(s)\n -Do a split on apollo-ios-codegen\n - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n + -To see that the patch fixes the issue you can use the custom subtree\n + script in the repo so following the same steps as above, except\n + instead of using 'git subtree ...' for the commands use\n + 'git-subtree.sh ...' for the commands\n \n You will see that the final split is looking for the last split\n on apollo-ios-codegen to use as it's starting point to process\n\n\n contrib/subtree/git-subtree.sh | 30 +++++++++++++++++++++-\n contrib/subtree/t/t7900-subtree.sh | 40 ++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..a0bf958ea66 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -778,6 +778,22 @@ ensure_valid_ref_format () {\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+# 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+\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+\tfi\n+\treturn 1\n+}\n+\n # Usage: process_split_commit REV PARENTS\n process_split_commit () {\n \tassert test $# = 2\n@@ -963,7 +979,19 @@ cmd_split () {\n \teval \"$grl\" |\n \twhile read rev parents\n \tdo\n-\t\tprocess_split_commit \"$rev\" \"$parents\"\n+\t\tif should_ignore_subtree_split_commit \"$rev\"\n+\t\tthen\n+\t\t\tcontinue\n+\t\tfi\n+\t\tparsedparents=''\n+\t\tfor parent in $parents\n+\t\tdo\n+\t\t\tif ! should_ignore_subtree_split_commit \"$parent\"\n+\t\t\tthen\n+\t\t\t\tparsedparents=\"$parsedparents$parent \"\n+\t\t\tfi\n+\t\tdone\n+\t\tprocess_split_commit \"$rev\" \"$parsedparents\"\n \tdone || exit $?\n \n \tlatest_new=$(cache_get latest_new) || exit $?\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 49a21dd7c9c..ca4df5be832 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -385,6 +385,46 @@ test_expect_success 'split sub dir/ with --rejoin' '\n \t)\n '\n \n+# Tests that commits from other subtrees are not processed as\n+# part of a split.\n+#\n+# This test performs the following:\n+# - Creates Repo with subtrees 'subA' and 'subB'\n+# - Creates commits in the repo including changes to subtrees\n+# - Runs the following 'split' and commit' commands in order:\n+# \t- Perform 'split' on subtree A\n+# \t- Perform 'split' on subtree B\n+# \t- Create new commits with changes to subtree A and B\n+# \t- Perform split on subtree A\n+# \t- Check that the commits in subtree B are not processed\n+#\t\t\tas part of the subtree A split\n+test_expect_success 'split with multiple subtrees' '\n+\tsubtree_test_create_repo \"$test_count\" &&\n+\tsubtree_test_create_repo \"$test_count/subA\" &&\n+\tsubtree_test_create_repo \"$test_count/subB\" &&\n+\ttest_create_commit \"$test_count\" main1 &&\n+\ttest_create_commit \"$test_count/subA\" subA1 &&\n+\ttest_create_commit \"$test_count/subA\" subA2 &&\n+\ttest_create_commit \"$test_count/subA\" subA3 &&\n+\ttest_create_commit \"$test_count/subB\" subB1 &&\n+\tgit -C \"$test_count\" fetch ./subA HEAD &&\n+\tgit -C \"$test_count\" subtree add --prefix=subADir FETCH_HEAD &&\n+\tgit -C \"$test_count\" fetch ./subB HEAD &&\n+\tgit -C \"$test_count\" subtree add --prefix=subBDir FETCH_HEAD &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA1 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB1 &&\n+\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n+\t\t--squash --rejoin -m \"Sub A Split 1\" &&\n+\tgit -C \"$test_count\" subtree split --prefix=subBDir \\\n+\t\t--squash --rejoin -m \"Sub B Split 1\" &&\n+\ttest_create_commit \"$test_count\" subADir/main-subA2 &&\n+\ttest_create_commit \"$test_count\" subBDir/main-subB2 &&\n+\tgit -C \"$test_count\" subtree split --prefix=subADir \\\n+\t\t--squash --rejoin -m \"Sub A Split 2\" &&\n+\ttest \"$(git -C \"$test_count\" subtree split --prefix=subBDir \\\n+\t\t--squash --rejoin -d -m \"Sub B Split 1\" 2>&1 | grep -w \"\\[1\\]\")\" = \"\"\n+'\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: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518\n-- \ngitgitgadget\n"},{"id":"485347","messageId":"CAP8UFD3FzP6QW4dJ9yiG1BAytLcsk+zGE+CBeArRJBJ8gsaDMQ@mail.gmail.com","threadId":"60241","inReplyTo":"pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-12-04T11:08:03Z","receivedAt":"2023-12-04T11:08:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Dec 1, 2023 at 3:54 PM Zach FettersMoore via GitGitGadget\n wrote:\n>\n> From: Zach FettersMoore \n>\n> When there are multiple subtrees present in a repository and they are\n> all using 'git subtree split', the 'split' command can take a\n> significant (and constantly growing) amount of time to run even when\n> using the '--rejoin' flag. This is due to the fact that when processing\n> commits to determine the last known split to start from when looking\n> for changes, if there has been a split/merge done from another subtree\n> there will be 2 split commits, one mainline and one subtree, for the\n> second subtree that are part of the processing. The non-mainline\n> subtree split commit will cause the processing to always need to search\n> the entire history of the given subtree as part of its processing even\n> though those commits are totally irrelevant to the current subtree\n> split being run.\n>\n> To see this in practice you can use the open source GitHub repo\n> 'apollo-ios-dev' and do the following in order:\n>\n> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n> directories\n> -Create a commit containing these changes\n> -Do a split on apollo-ios-codegen\n> - Do a fetch on the subtree repo\n> - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n\nNow I get the following without your patch at this step:\n\n$ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n[...]/libexec/git-core/git-subtree: 318: Maximum function recursion\ndepth (1000) reached\n\nLine 318 in git-subtree.sh contains the following:\n\nmissed=$(cache_miss \"$@\") || exit $?\n\nWith your patch it seems to work:\n\n$ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\nMerge made by the 'ort' strategy.\ne274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n\n> - Depending on the current state of the 'apollo-ios-dev' repo\n> you may see the issue at this point if the last split was on\n> apollo-ios\n\nI guess I see it, but it seems a bit different for me than what you describe.\n\nOtherwise your patch looks good to me now.\n\nThanks,\nChristian.\n"},{"id":"485540","messageId":"CAEWN6q3RTbVuMb0VyCYz196ZL+OGAAHbJLZ2-MnW1RVVabg7Mw@mail.gmail.com","threadId":"60241","inReplyTo":"CAP8UFD3FzP6QW4dJ9yiG1BAytLcsk+zGE+CBeArRJBJ8gsaDMQ@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore","fromEmail":"zach.fetters@apollographql.com","sentAt":"2023-12-11T15:39:38Z","receivedAt":"2023-12-11T15:39:50Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":">>\n>> From: Zach FettersMoore \n>>\n>> When there are multiple subtrees present in a repository and they are\n>> all using 'git subtree split', the 'split' command can take a\n>> significant (and constantly growing) amount of time to run even when\n>> using the '--rejoin' flag. This is due to the fact that when processing\n>> commits to determine the last known split to start from when looking\n>> for changes, if there has been a split/merge done from another subtree\n>> there will be 2 split commits, one mainline and one subtree, for the\n>> second subtree that are part of the processing. The non-mainline\n>> subtree split commit will cause the processing to always need to search\n>> the entire history of the given subtree as part of its processing even\n>> though those commits are totally irrelevant to the current subtree\n>> split being run.\n>>\n>> To see this in practice you can use the open source GitHub repo\n>> 'apollo-ios-dev' and do the following in order:\n>>\n>> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n>> directories\n>> -Create a commit containing these changes\n>> -Do a split on apollo-ios-codegen\n>> - Do a fetch on the subtree repo\n>> - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n>> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n\n> Now I get the following without your patch at this step:\n>\n> $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> [...]/libexec/git-core/git-subtree: 318: Maximum function recursion\n> depth (1000) reached\n>\n> Line 318 in git-subtree.sh contains the following:\n>\n> missed=$(cache_miss \"$@\") || exit $?\n>\n> With your patch it seems to work:\n>\n> $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> Merge made by the 'ort' strategy.\n> e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n\nLooking into this some it looks like it could be a bash config\ndifference? My machine always runs it all the way through vs\nfailing for recursion depth. Although that would also be an issue\nwhich is solved by this fix.\n\n>> - Depending on the current state of the 'apollo-ios-dev' repo\n>> you may see the issue at this point if the last split was on\n>> apollo-ios\n\n> I guess I see it, but it seems a bit different for me than what you describe.\n>\n> Otherwise your patch looks good to me now.\n\nYea I hadn't accounted for/realized that some folks may see a recursion\ndepth error vs it just taking a long time like it does for me. Also what\nI was saying with the apollo-ios-dev repo is you may not need all the steps\nto see the issue, because its possible the state of the repo is already\nin a position to display the issue just by doing a split on\napollo-ios-codegen.\n\nGreat! Thanks again for all the feedback and guidance! Is there anything\nelse I need to do to get this across the finish line and merged in?\n"},{"id":"485581","messageId":"CAP8UFD19phFz54d8fDM=MBRMSD9Rz4R0_463KgptN8eeFs7MnQ@mail.gmail.com","threadId":"60241","inReplyTo":"CAEWN6q3RTbVuMb0VyCYz196ZL+OGAAHbJLZ2-MnW1RVVabg7Mw@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-12-12T16:06:38Z","receivedAt":"2023-12-12T16:06:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore\n wrote:\n>\n> >>\n> >> From: Zach FettersMoore \n\n> >> To see this in practice you can use the open source GitHub repo\n> >> 'apollo-ios-dev' and do the following in order:\n> >>\n> >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n> >> directories\n> >> -Create a commit containing these changes\n> >> -Do a split on apollo-ios-codegen\n> >> - Do a fetch on the subtree repo\n> >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n> >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n>\n> > Now I get the following without your patch at this step:\n> >\n> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion\n> > depth (1000) reached\n> >\n> > Line 318 in git-subtree.sh contains the following:\n> >\n> > missed=$(cache_miss \"$@\") || exit $?\n> >\n> > With your patch it seems to work:\n> >\n> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > Merge made by the 'ort' strategy.\n> > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n>\n> Looking into this some it looks like it could be a bash config\n> difference? My machine always runs it all the way through vs\n> failing for recursion depth. Although that would also be an issue\n> which is solved by this fix.\n\nI use Ubuntu where /bin/sh is dash so my current guess is that dash\nmight have a smaller recursion limit than bash.\n\nI just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth\nwhich seems to agree.\n\nI will try to test using bash soon.\n\n> >> - Depending on the current state of the 'apollo-ios-dev' repo\n> >> you may see the issue at this point if the last split was on\n> >> apollo-ios\n>\n> > I guess I see it, but it seems a bit different for me than what you describe.\n> >\n> > Otherwise your patch looks good to me now.\n>\n> Yea I hadn't accounted for/realized that some folks may see a recursion\n> depth error vs it just taking a long time like it does for me. Also what\n> I was saying with the apollo-ios-dev repo is you may not need all the steps\n> to see the issue, because its possible the state of the repo is already\n> in a position to display the issue just by doing a split on\n> apollo-ios-codegen.\n>\n> Great! Thanks again for all the feedback and guidance! Is there anything\n> else I need to do to get this across the finish line and merged in?\n\nHopefully I will be able to confirm I see the same error as you with\nbash soon, and it will be enough to get it merged.\n"},{"id":"485596","messageId":"xmqqzfyfoy2w.fsf@gitster.g","threadId":"60241","inReplyTo":"CAP8UFD19phFz54d8fDM=MBRMSD9Rz4R0_463KgptN8eeFs7MnQ@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T22:28:07Z","receivedAt":"2023-12-12T22:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n>> > Merge made by the 'ort' strategy.\n>> > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n>>\n>> Looking into this some it looks like it could be a bash config\n>> difference? My machine always runs it all the way through vs\n>> failing for recursion depth. Although that would also be an issue\n>> which is solved by this fix.\n>\n> I use Ubuntu where /bin/sh is dash so my current guess is that dash\n> might have a smaller recursion limit than bash.\n\nThat sounds quite bad. Does it have to be recursive (iow, if we can\nrewrite the logic to be iterative instead, that would be a much better\nway to fix the issue)?\n"},{"id":"485619","messageId":"CAEWN6q2XeDDLvSM-ik_-HVqpeyYZLWpPwoj2SUyB9L9NyMJPLw@mail.gmail.com","threadId":"60241","inReplyTo":"xmqqzfyfoy2w.fsf@gitster.g","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Zach FettersMoore","fromEmail":"zach.fetters@apollographql.com","sentAt":"2023-12-13T15:20:38Z","receivedAt":"2023-12-13T15:20:49Z","isPatch":true,"sender":{"key":"zach.fetters@apollographql.com","avatar":"https://gravatar.com/avatar/4d9f56700fe9fe2b546a9765f82f62a27a2c63397a98ea452f9f89bc8748879d?d=mp&s=160"},"body":"Christian Couder writes:\n\n>>> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n>>> > Merge made by the 'ort' strategy.\n>>> > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n>>>\n>>> Looking into this some it looks like it could be a bash config\n>>> difference? My machine always runs it all the way through vs\n>>> failing for recursion depth. Although that would also be an issue\n>>> which is solved by this fix.\n>>\n>> I use Ubuntu where /bin/sh is dash so my current guess is that dash\n>> might have a smaller recursion limit than bash.\n>\n> That sounds quite bad. Does it have to be recursive (iow, if we can\n> rewrite the logic to be iterative instead, that would be a much better\n> way to fix the issue)?\n\nI don't think an iterative vs recursive approach fixes this\nparticular issue, the root of the issue this patch is fixing\nis that lots of commits from the history of subtrees not\nbeing acted upon are being processed when they don't need to\nbe. So the iterative approach would likely resolve the\nrecursion limit issue for some shells, but in my instance\nI don't see a recursion limit error, it just takes an\nextraordinary amount of time to run the split command\nbecause of all the unnecessary processing which needs to be\navoided which this patch fixes.\n"},{"id":"485864","messageId":"CAP8UFD3b2y+55j3NMDm89hpVRNxX2TA-AdQS=zsboD30pZ1c4Q@mail.gmail.com","threadId":"60241","inReplyTo":"CAP8UFD19phFz54d8fDM=MBRMSD9Rz4R0_463KgptN8eeFs7MnQ@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-12-20T15:25:26Z","receivedAt":"2023-12-20T15:25:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 12, 2023 at 5:06 PM Christian Couder\n wrote:\n>\n> On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore\n> wrote:\n> >\n> > >>\n> > >> From: Zach FettersMoore \n>\n> > >> To see this in practice you can use the open source GitHub repo\n> > >> 'apollo-ios-dev' and do the following in order:\n> > >>\n> > >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n> > >> directories\n> > >> -Create a commit containing these changes\n> > >> -Do a split on apollo-ios-codegen\n> > >> - Do a fetch on the subtree repo\n> > >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n> > >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> >\n> > > Now I get the following without your patch at this step:\n> > >\n> > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion\n> > > depth (1000) reached\n> > >\n> > > Line 318 in git-subtree.sh contains the following:\n> > >\n> > > missed=$(cache_miss \"$@\") || exit $?\n> > >\n> > > With your patch it seems to work:\n> > >\n> > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > > Merge made by the 'ort' strategy.\n> > > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n> >\n> > Looking into this some it looks like it could be a bash config\n> > difference? My machine always runs it all the way through vs\n> > failing for recursion depth. Although that would also be an issue\n> > which is solved by this fix.\n>\n> I use Ubuntu where /bin/sh is dash so my current guess is that dash\n> might have a smaller recursion limit than bash.\n>\n> I just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth\n> which seems to agree.\n>\n> I will try to test using bash soon.\n\nSorry, to not have tried earlier before with bash.\n\nNow I have tried it and yeah it works fine with you patch, while\nwithout it the last step of the reproduction recipe takes a lot of\ntime and results in a core dump:\n\n/home/christian/libexec/git-core/git-subtree: line 924: 857920 Done\n eval \"$grl\"\n 857921 Segmentation fault (core dumped) | while read rev parents; do\n process_split_commit \"$rev\" \"$parents\";\ndone\n\nSo overall I think your patch is great! Thanks!\n"},{"id":"486259","messageId":"CAP8UFD38X5sT2qTB7P4oeOgWyc3W2Y4gp6DO3VZrAELSS8TzbQ@mail.gmail.com","threadId":"60241","inReplyTo":"CAEWN6q2XeDDLvSM-ik_-HVqpeyYZLWpPwoj2SUyB9L9NyMJPLw@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-03T16:33:00Z","receivedAt":"2024-01-03T16:33:15Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"(Sorry for replying only to Zach instead of everyone previously.)\n\nOn Wed, Dec 13, 2023 at 4:20 PM Zach FettersMoore\n wrote:\n>\n> Christian Couder writes:\n>\n> >>> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> >>> > Merge made by the 'ort' strategy.\n> >>> > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n> >>>\n> >>> Looking into this some it looks like it could be a bash config\n> >>> difference? My machine always runs it all the way through vs\n> >>> failing for recursion depth. Although that would also be an issue\n> >>> which is solved by this fix.\n> >>\n> >> I use Ubuntu where /bin/sh is dash so my current guess is that dash\n> >> might have a smaller recursion limit than bash.\n> >\n> > That sounds quite bad. Does it have to be recursive (iow, if we can\n> > rewrite the logic to be iterative instead, that would be a much better\n> > way to fix the issue)?\n>\n> I don't think an iterative vs recursive approach fixes this\n> particular issue, the root of the issue this patch is fixing\n> is that lots of commits from the history of subtrees not\n> being acted upon are being processed when they don't need to\n> be. So the iterative approach would likely resolve the\n> recursion limit issue for some shells, but in my instance\n> I don't see a recursion limit error, it just takes an\n> extraordinary amount of time to run the split command\n> because of all the unnecessary processing which needs to be\n> avoided which this patch fixes.\n\nFixing possible recursion might be an improvement on top of your\npatch. But without your patch the test case it describes would anyway\ntake a lot more time than seems necessary. So I agree that your patch\nshould definitely be merged anyway.\n"},{"id":"487360","messageId":"CAP8UFD0M_KeUTHthQ6n_a1KbEvuA1gAsE2jKkAqd-4twjbpNWw@mail.gmail.com","threadId":"60241","inReplyTo":"CAP8UFD3b2y+55j3NMDm89hpVRNxX2TA-AdQS=zsboD30pZ1c4Q@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-25T10:09:41Z","receivedAt":"2024-01-25T10:09:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"It seems that this topic has fallen into the cracks or something,\nwhile the associated pch looks good to me.\n\nOn Wed, Dec 20, 2023 at 4:25 PM Christian Couder\n wrote:\n>\n> On Tue, Dec 12, 2023 at 5:06 PM Christian Couder\n> wrote:\n> >\n> > On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore\n> > wrote:\n> > >\n> > > >>\n> > > >> From: Zach FettersMoore \n> >\n> > > >> To see this in practice you can use the open source GitHub repo\n> > > >> 'apollo-ios-dev' and do the following in order:\n> > > >>\n> > > >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'\n> > > >> directories\n> > > >> -Create a commit containing these changes\n> > > >> -Do a split on apollo-ios-codegen\n> > > >> - Do a fetch on the subtree repo\n> > > >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git\n> > > >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > >\n> > > > Now I get the following without your patch at this step:\n> > > >\n> > > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > > > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion\n> > > > depth (1000) reached\n> > > >\n> > > > Line 318 in git-subtree.sh contains the following:\n> > > >\n> > > > missed=$(cache_miss \"$@\") || exit $?\n> > > >\n> > > > With your patch it seems to work:\n> > > >\n> > > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin\n> > > > Merge made by the 'ort' strategy.\n> > > > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4\n> > >\n> > > Looking into this some it looks like it could be a bash config\n> > > difference? My machine always runs it all the way through vs\n> > > failing for recursion depth. Although that would also be an issue\n> > > which is solved by this fix.\n> >\n> > I use Ubuntu where /bin/sh is dash so my current guess is that dash\n> > might have a smaller recursion limit than bash.\n> >\n> > I just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth\n> > which seems to agree.\n> >\n> > I will try to test using bash soon.\n>\n> Sorry, to not have tried earlier before with bash.\n>\n> Now I have tried it and yeah it works fine with you patch, while\n> without it the last step of the reproduction recipe takes a lot of\n> time and results in a core dump:\n>\n> /home/christian/libexec/git-core/git-subtree: line 924: 857920 Done\n> eval \"$grl\"\n> 857921 Segmentation fault (core dumped) | while read rev parents; do\n> process_split_commit \"$rev\" \"$parents\";\n> done\n>\n> So overall I think your patch is great! Thanks!\n"},{"id":"487369","messageId":"xmqq5xzhxta3.fsf@gitster.g","threadId":"60241","inReplyTo":"CAP8UFD0M_KeUTHthQ6n_a1KbEvuA1gAsE2jKkAqd-4twjbpNWw@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-25T16:38:28Z","receivedAt":"2024-01-25T16:38:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> It seems that this topic has fallen into the cracks or something,\n> while the associated pch looks good to me.\n\nYeah, it wasn't clear to me that your message you are responding to\nwas your Reviewed-by:. If I recall my impression correctly from the\ntime I skimmed its proposed log message the last time, it focused on\ndescribing a single failure case the author encountered in the real\nworld and said that the patch changed the behaviour to correct that\nsingle case, and was not very clear if it was meant as a general\nfix. Is the patch text, including its proposed patch description,\nsatisfactory to you? In other words, is the above your Reviewed-by:?\n\nThanks for pinging the thread.\n"},{"id":"487375","messageId":"CAP8UFD2Oo8v8Qn0JPYURZA_s7ynZmk6v30b9zR==MxWBTXk9Ng@mail.gmail.com","threadId":"60241","inReplyTo":"xmqq5xzhxta3.fsf@gitster.g","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-25T18:52:22Z","receivedAt":"2024-01-25T18:52:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Jan 25, 2024 at 5:38 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > It seems that this topic has fallen into the cracks or something,\n> > while the associated pch looks good to me.\n>\n> Yeah, it wasn't clear to me that your message you are responding to\n> was your Reviewed-by:. If I recall my impression correctly from the\n> time I skimmed its proposed log message the last time, it focused on\n> describing a single failure case the author encountered in the real\n> world and said that the patch changed the behaviour to correct that\n> single case, and was not very clear if it was meant as a general\n> fix. Is the patch text, including its proposed patch description,\n> satisfactory to you? In other words, is the above your Reviewed-by:?\n\nYes, it's satisfactory for me, and I am Ok to give my Reviewed-by:, thanks!\n"},{"id":"487378","messageId":"xmqqbk99w8c6.fsf@gitster.g","threadId":"60241","inReplyTo":"CAP8UFD2Oo8v8Qn0JPYURZA_s7ynZmk6v30b9zR==MxWBTXk9Ng@mail.gmail.com","subject":"Re: [PATCH v6] subtree: fix split processing with multiple subtrees present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-25T18:56:09Z","receivedAt":"2024-01-25T18:56:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Thu, Jan 25, 2024 at 5:38 PM Junio C Hamano wrote:\n>>\n>> Christian Couder writes:\n>>\n>> > It seems that this topic has fallen into the cracks or something,\n>> > while the associated pch looks good to me.\n>>\n>> Yeah, it wasn't clear to me that your message you are responding to\n>> was your Reviewed-by:. If I recall my impression correctly from the\n>> time I skimmed its proposed log message the last time, it focused on\n>> describing a single failure case the author encountered in the real\n>> world and said that the patch changed the behaviour to correct that\n>> single case, and was not very clear if it was meant as a general\n>> fix. Is the patch text, including its proposed patch description,\n>> satisfactory to you? In other words, is the above your Reviewed-by:?\n>\n> Yes, it's satisfactory for me, and I am Ok to give my Reviewed-by:, thanks!\n\nThanks. Will queue.\n"},{"id":"524594","messageId":"c9e8f54f-2594-4092-ae41-f1da73e97f6e@howdoi.land","threadId":"60241","inReplyTo":"pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com","subject":"subtree: [v2.44 regression] split may produce different history","fromName":"Colin Stagner","fromEmail":"ask+git@howdoi.land","sentAt":"2025-08-21T03:13:53Z","receivedAt":"2025-08-21T03:51:57Z","isPatch":false,"sender":{"key":"ask+git@howdoi.land","avatar":null},"body":"98ba49ccc247 likely introduces a regression in \"git subtree split\" [1] \n[2]. For some inputs, the split history is incomplete and does not match \nprevious git versions.\n\nFor\n\n git subtree split -P somedir\n\nif the history of `somedir` also contains *squashed* subtree *merges*, \nthe split history may be incomplete. MWE follows:\n\n```bash\ngit init mwe && cd mwe\n\n# create history we will subtree merge later\ngit checkout --orphan deeper\ntouch two_deep && git add two_deep\ngit commit -m 'deeper: a nested subtree'\n\n# create top-level project history with one\n# subproject in a directory sub/\ngit checkout --orphan main\ngit reset --hard\necho 'A test for git-subtree'>README.txt\nmkdir sub && touch sub/README.sub.txt\ngit add .\ngit commit -m 'Initial commit'\n\n# add \"deeper\" branch as sub/deeper\n# the --squash is important here since it omits\n# \"git-subtree-mainline:\", which 98ba49ccc247\n# looks for in `should_ignore_subtree_split_commit()`\ngit subtree add --squash -P sub/deeper deeper\n```\n\nNow `git ls-tree --name-only -r main` looks like this:\n\n README.txt\n sub/README.sub.txt\n sub/deeper/two_deep\n\nWe can split `sub` off as its own top-level history.\n\n```bash\ngit ls-tree -r --name-only -- \\\n \"$(git subtree.sh split -P sub)\"\n```\n\nBefore the patch, that looks like:\n\n README.sub.txt\n deeper/two_deep\n\nWhich is correct. After the patch, there is only:\n\n README.sub.txt\n\nwhich is missing the entire `deeper/` directory. (The hash output from \n`split` is also different.)\n\nI suspect the test in `should_ignore_subtree_split_commit ()` \ninadvertently rejects commits that should be kept.\n\nI tested with Git binaries from v2.43 on Ubuntu [3].\n\n[1]: \nhttps://git.kernel.org/pub/scm/git/git.git/commit/?id=98ba49ccc247c3521659aa3d43c970e8978922c5\n\n[2]: \nhttps://lore.kernel.org/all/pull.1587.v6.git.1701442494319.gitgitgadget@gmail.com/\n\n[3]: \nhttp://archive.ubuntu.com/ubuntu/pool/main/g/git/git_2.43.0-1ubuntu7.3_amd64.deb\n"}]}