{"thread":{"id":"57007","subject":"[PATCH] subtree: fix argument handling in check_parents","startedAt":"2021-12-01T02:06:57Z","lastAt":"2022-01-04T20:04:13Z","messageCount":10,"participants":["James Limbouris via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"442734","messageId":"pull.1086.git.1638324413653.gitgitgadget@gmail.com","threadId":"57007","inReplyTo":null,"subject":"[PATCH] subtree: fix argument handling in check_parents","fromName":"James Limbouris via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-01T02:06:53Z","receivedAt":"2021-12-01T02:06:57Z","isPatch":true,"sender":{"key":"james@digitalmatter.com","avatar":"https://avatars.githubusercontent.com/u/28915246?v=4"},"body":"From: James Limbouris <james@digitalmatter.com>\n\ncheck_parents was taking all of its arguments as a single string,\nand erroneously passing them to cache_miss as a single string.\ncache_miss would then fail, and the spurious cache misses it produced\nwould hurt performance.\n\nFor consistency, take multiple arguments in check_parents,\nand pass all of them to cache_miss separately.\n\nSigned-off-by: James Limbouris <james@digitalmatter.com>\n---\n    subtree: fix argument handling in check_parents\n    \n    Hello git developers. Please consider this small patch that fixes a bug\n    introduced during a coding style cleanup of the subtree command. Changes\n    to the argument handling were causing check_parents to fail when more\n    than one parent was supplied, which led to a small loss of performance.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1086\n\n contrib/subtree/git-subtree.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7f767b5c38f..56f24000c2c 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -296,10 +296,9 @@ cache_miss () {\n \tdone\n }\n \n-# Usage: check_parents PARENTS_EXPR\n+# Usage: check_parents [REVS...]\n check_parents () {\n-\tassert test $# = 1\n-\tmissed=$(cache_miss \"$1\") || exit $?\n+\tmissed=$(cache_miss $*) || exit $?\n \tlocal indent=$(($indent + 1))\n \tfor miss in $missed\n \tdo\n@@ -753,7 +752,7 @@ process_split_commit () {\n \tfi\n \tcreatecount=$(($createcount + 1))\n \tdebug \"parents: $parents\"\n-\tcheck_parents \"$parents\"\n+\tcheck_parents $parents\n \tnewparents=$(cache_get $parents) || exit $?\n \tdebug \"newparents: $newparents\"\n \n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"442825","messageId":"xmqq1r2wylik.fsf@gitster.g","threadId":"57007","inReplyTo":"pull.1086.git.1638324413653.gitgitgadget@gmail.com","subject":"Re: [PATCH] subtree: fix argument handling in check_parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-01T23:04:51Z","receivedAt":"2021-12-01T23:05:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"James Limbouris via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: James Limbouris <james@digitalmatter.com>\n>\n> check_parents was taking all of its arguments as a single string,\n> and erroneously passing them to cache_miss as a single string.\n> cache_miss would then fail, and the spurious cache misses it produced\n> would hurt performance.\n>\n> For consistency, take multiple arguments in check_parents,\n> and pass all of them to cache_miss separately.\n>\n> Signed-off-by: James Limbouris <james@digitalmatter.com>\n> ---\n>     subtree: fix argument handling in check_parents\n>     \n>     Hello git developers. Please consider this small patch that fixes a bug\n>     introduced during a coding style cleanup of the subtree command. Changes\n>     to the argument handling were causing check_parents to fail when more\n>     than one parent was supplied, which led to a small loss of performance.\n\nI do not do \"git subtree\", and this cannot really be a proper review\nthat is more than \"Looks OK from a cursory look\", but anyway...\n\nIt seems that 315a84f9 (subtree: use commits before rejoins for\nsplits, 2018-09-28) is what broke the logic, but it does not look\nlike a coding style clean-up to me.\n\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 7f767b5c38f..56f24000c2c 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -296,10 +296,9 @@ cache_miss () {\n>  \tdone\n>  }\n>  \n> -# Usage: check_parents PARENTS_EXPR\n> +# Usage: check_parents [REVS...]\n>  check_parents () {\n> -\tassert test $# = 1\n> -\tmissed=$(cache_miss \"$1\") || exit $?\n> +\tmissed=$(cache_miss $*) || exit $?\n\nWe know at this point each of $1, $2, etc. have exactly one\nrevision, and we want cache_miss function to take one revision per\nits parameter, so writing \"$@\" is much more preferrable over $* even\nthough they do the same thing in practice in the context of this\ncode, I think.\n\n>  \tlocal indent=$(($indent + 1))\n>  \tfor miss in $missed\n>  \tdo\n> @@ -753,7 +752,7 @@ process_split_commit () {\n>  \tfi\n>  \tcreatecount=$(($createcount + 1))\n>  \tdebug \"parents: $parents\"\n> -\tcheck_parents \"$parents\"\n> +\tcheck_parents $parents\n>  \tnewparents=$(cache_get $parents) || exit $?\n>  \tdebug \"newparents: $newparents\"\n>  \n>\n> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n"},{"id":"442852","messageId":"pull.1086.v2.git.1638424290015.gitgitgadget@gmail.com","threadId":"57007","inReplyTo":"pull.1086.git.1638324413653.gitgitgadget@gmail.com","subject":"[PATCH v2] subtree: fix argument handling in check_parents","fromName":"James Limbouris via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-02T05:51:29Z","receivedAt":"2021-12-02T05:57:32Z","isPatch":true,"sender":{"key":"james@digitalmatter.com","avatar":"https://avatars.githubusercontent.com/u/28915246?v=4"},"body":"From: James Limbouris <james@digitalmatter.com>\n\ncheck_parents was taking all of its arguments as a single string,\nand erroneously passing them to cache_miss as a single string.\ncache_miss would then fail, and the spurious cache misses it produced\nwould hurt performance.\n\nFor consistency, take multiple arguments in check_parents,\nand pass all of them to cache_miss separately.\n\nSigned-off-by: James Limbouris <james@digitalmatter.com>\n---\n    subtree: fix argument handling in check_parents\n    \n    > It seems that 315a84f9 (subtree: use commits before rejoins for\n    > splits, 2018-09-28) is what broke the logic, but it does not look like\n    > a coding style clean-up to me.\n    \n    Sorry - you're right. I thought it was 6ae6a2 (subtree: adjust style to\n    match CodingGuidelines) that broke it, but it was actually the addition\n    of quotes in the check_parents and cache_miss calls of 315a84f9.\n    \n    > so writing \"$@\" is much more preferrable over $*\n    \n    Yes agreed - thanks!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1086\n\nRange-diff vs v1:\n\n 1:  11b65a7b3a5 ! 1:  1198a84995b subtree: fix argument handling in check_parents\n     @@ contrib/subtree/git-subtree.sh: cache_miss () {\n       check_parents () {\n      -\tassert test $# = 1\n      -\tmissed=$(cache_miss \"$1\") || exit $?\n     -+\tmissed=$(cache_miss $*) || exit $?\n     ++\tmissed=$(cache_miss \"$@\") || exit $?\n       \tlocal indent=$(($indent + 1))\n       \tfor miss in $missed\n       \tdo\n\n\n contrib/subtree/git-subtree.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7f767b5c38f..71f1fd94bde 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -296,10 +296,9 @@ cache_miss () {\n \tdone\n }\n \n-# Usage: check_parents PARENTS_EXPR\n+# Usage: check_parents [REVS...]\n check_parents () {\n-\tassert test $# = 1\n-\tmissed=$(cache_miss \"$1\") || exit $?\n+\tmissed=$(cache_miss \"$@\") || exit $?\n \tlocal indent=$(($indent + 1))\n \tfor miss in $missed\n \tdo\n@@ -753,7 +752,7 @@ process_split_commit () {\n \tfi\n \tcreatecount=$(($createcount + 1))\n \tdebug \"parents: $parents\"\n-\tcheck_parents \"$parents\"\n+\tcheck_parents $parents\n \tnewparents=$(cache_get $parents) || exit $?\n \tdebug \"newparents: $newparents\"\n \n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"443002","messageId":"nycvar.QRO.7.76.6.2112031610590.63@tvgsbejvaqbjf.bet","threadId":"57007","inReplyTo":"pull.1086.git.1638324413653.gitgitgadget@gmail.com","subject":"Re: [PATCH] subtree: fix argument handling in check_parents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-03T15:22:23Z","receivedAt":"2021-12-03T15:22:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi James,\n\n\nOn Wed, 1 Dec 2021, James Limbouris via GitGitGadget wrote:\n\n> From: James Limbouris <james@digitalmatter.com>\n>\n> check_parents was taking all of its arguments as a single string,\n> and erroneously passing them to cache_miss as a single string.\n> cache_miss would then fail, and the spurious cache misses it produced\n> would hurt performance.\n>\n> For consistency, take multiple arguments in check_parents,\n> and pass all of them to cache_miss separately.\n>\n> Signed-off-by: James Limbouris <james@digitalmatter.com>\n> ---\n>     subtree: fix argument handling in check_parents\n>\n>     Hello git developers. Please consider this small patch that fixes a bug\n>     introduced during a coding style cleanup of the subtree command. Changes\n>     to the argument handling were causing check_parents to fail when more\n>     than one parent was supplied, which led to a small loss of performance.\n\nWhen I look through the commit history of `git-subtree.sh`, I see that the\nchange was introduced in 315a84f9aa0 (subtree: use commits before rejoins\nfor splits, 2018-09-28) (which was not really a coding style cleanup).\n\nThe change was actually not done right, if I read the commit correctly,\nbecause it added a new parameter _to the end_, even if the\n`check_parents()` function took an arbitrary number of parameters already.\nAnd indeed, it changed the `\"$@\"` into a \"$1\", pretending that only one\nparent would be passed.\n\nNow, I do not really understand under what circumstances multiple parents\ncould be passed to `check_parents()`, but I think that it does not matter\nwhether you use `--format=%P` or `^@` (the former was changed to the\nlatter in 19ad68d95d6 (subtree: performance improvement for finding\nunexpected parent commits, 2018-10-12)), you can always get an arbitrary\nnumber of parents that way.\n\nThe natural thing, now, would be to move the added `indent` parameter to\nthe front of the parameter list, but I see that there was some cleanup in\ne9525a8a029 (subtree: have $indent actually affect indentation,\n2021-04-27) which _removed_ that `indent` parameter.\n\nSo I _think_ your change is correct, even if I would love to see an\neasy-to-understand description of the scenario where more than one parents\nmight be checked.\n\nAnother thing I would like to see is probably even more important: rather\nthan using $*, we should use the original \"$@\" instead (with\ndouble-quotes). It should not matter a lot right now because we know that\nthe parameters are the output of `git rev-parse \"$rev^@\"` (which provides\nthem as a list of full object IDs, i.e. containing no white-space except\nto delimit the IDs), but it still the correct form to use \"$@\" instead.\n\nThanks,\nDscho\n\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1086\n>\n>  contrib/subtree/git-subtree.sh | 7 +++----\n>  1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 7f767b5c38f..56f24000c2c 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -296,10 +296,9 @@ cache_miss () {\n>  \tdone\n>  }\n>\n> -# Usage: check_parents PARENTS_EXPR\n> +# Usage: check_parents [REVS...]\n>  check_parents () {\n> -\tassert test $# = 1\n> -\tmissed=$(cache_miss \"$1\") || exit $?\n> +\tmissed=$(cache_miss $*) || exit $?\n>  \tlocal indent=$(($indent + 1))\n>  \tfor miss in $missed\n>  \tdo\n> @@ -753,7 +752,7 @@ process_split_commit () {\n>  \tfi\n>  \tcreatecount=$(($createcount + 1))\n>  \tdebug \"parents: $parents\"\n> -\tcheck_parents \"$parents\"\n> +\tcheck_parents $parents\n>  \tnewparents=$(cache_get $parents) || exit $?\n>  \tdebug \"newparents: $newparents\"\n>\n>\n> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n> --\n> gitgitgadget\n>\n"},{"id":"443024","messageId":"xmqq1r2tsbid.fsf@gitster.g","threadId":"57007","inReplyTo":"nycvar.QRO.7.76.6.2112031610590.63@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] subtree: fix argument handling in check_parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-03T20:02:02Z","receivedAt":"2021-12-03T20:02:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> So I _think_ your change is correct, even if I would love to see an\n> easy-to-understand description of the scenario where more than one parents\n> might be checked.\n\nI agree with this.  I'll mark the topic as \"Expecting a reroll\" in\nthe next \"What's cooking\" report, so that we do not forget that we\nare waiting for such an improved description in the log message.\n\nThanks.\n"},{"id":"443130","messageId":"pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com","threadId":"57007","inReplyTo":"pull.1086.v2.git.1638424290015.gitgitgadget@gmail.com","subject":"[PATCH v3] subtree: fix argument handling in check_parents","fromName":"James Limbouris via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-06T02:45:42Z","receivedAt":"2021-12-06T02:45:57Z","isPatch":true,"sender":{"key":"james@digitalmatter.com","avatar":"https://avatars.githubusercontent.com/u/28915246?v=4"},"body":"From: James Limbouris <james@digitalmatter.com>\n\ncheck_parents was taking all of its arguments as a single string,\nand erroneously passing them to cache_miss as a single string. For\ncommits with a single parent this would succeed, but whenever a merge\ncommit was processed, cache_miss would be passed \"parent1 parent2\"\ninstead of \"parent1\" \"parent2\" and fail, leading to unecessary rechecks\nof the parent commits.\n\nFor consistency, take multiple arguments in check_parents,\nand pass all of them to cache_miss separately.\n\nSigned-off-by: James Limbouris <james@digitalmatter.com>\n---\n    subtree: fix argument handling in check_parents\n    \n    > I agree with this. I'll mark the topic as \"Expecting a reroll\" in the\n    > next \"What's cooking\" report, so that we do not forget that we are\n    > waiting for such an improved description in the log message.\n    \n    Thanks for the feedback - log message has been amended.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1086\n\nRange-diff vs v2:\n\n 1:  1198a84995b ! 1:  f734ca9d276 subtree: fix argument handling in check_parents\n     @@ Commit message\n          subtree: fix argument handling in check_parents\n      \n          check_parents was taking all of its arguments as a single string,\n     -    and erroneously passing them to cache_miss as a single string.\n     -    cache_miss would then fail, and the spurious cache misses it produced\n     -    would hurt performance.\n     +    and erroneously passing them to cache_miss as a single string. For\n     +    commits with a single parent this would succeed, but whenever a merge\n     +    commit was processed, cache_miss would be passed \"parent1 parent2\"\n     +    instead of \"parent1\" \"parent2\" and fail, leading to unecessary rechecks\n     +    of the parent commits.\n      \n          For consistency, take multiple arguments in check_parents,\n          and pass all of them to cache_miss separately.\n\n\n contrib/subtree/git-subtree.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7f767b5c38f..71f1fd94bde 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -296,10 +296,9 @@ cache_miss () {\n \tdone\n }\n \n-# Usage: check_parents PARENTS_EXPR\n+# Usage: check_parents [REVS...]\n check_parents () {\n-\tassert test $# = 1\n-\tmissed=$(cache_miss \"$1\") || exit $?\n+\tmissed=$(cache_miss \"$@\") || exit $?\n \tlocal indent=$(($indent + 1))\n \tfor miss in $missed\n \tdo\n@@ -753,7 +752,7 @@ process_split_commit () {\n \tfi\n \tcreatecount=$(($createcount + 1))\n \tdebug \"parents: $parents\"\n-\tcheck_parents \"$parents\"\n+\tcheck_parents $parents\n \tnewparents=$(cache_get $parents) || exit $?\n \tdebug \"newparents: $newparents\"\n \n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"443376","messageId":"nycvar.QRO.7.76.6.2112072243310.90@tvgsbejvaqbjf.bet","threadId":"57007","inReplyTo":"nycvar.QRO.7.76.6.2112031610590.63@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] subtree: fix argument handling in check_parents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-07T21:45:20Z","receivedAt":"2021-12-07T21:45:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi James,\n\nI saw that you sent a v3, but did not see any of this information (which\ntook a good while to assemble, as you might have guessed) in the commit\nmessage. However, I think that message would make for the best home for\nthis information:\n\nOn Fri, 3 Dec 2021, Johannes Schindelin wrote:\n\n> On Wed, 1 Dec 2021, James Limbouris via GitGitGadget wrote:\n>\n> > From: James Limbouris <james@digitalmatter.com>\n> >\n> > check_parents was taking all of its arguments as a single string,\n> > and erroneously passing them to cache_miss as a single string.\n> > cache_miss would then fail, and the spurious cache misses it produced\n> > would hurt performance.\n> >\n> > For consistency, take multiple arguments in check_parents,\n> > and pass all of them to cache_miss separately.\n> >\n> > Signed-off-by: James Limbouris <james@digitalmatter.com>\n> > ---\n> >     subtree: fix argument handling in check_parents\n> >\n> >     Hello git developers. Please consider this small patch that fixes a bug\n> >     introduced during a coding style cleanup of the subtree command. Changes\n> >     to the argument handling were causing check_parents to fail when more\n> >     than one parent was supplied, which led to a small loss of performance.\n>\n> When I look through the commit history of `git-subtree.sh`, I see that the\n> change was introduced in 315a84f9aa0 (subtree: use commits before rejoins\n> for splits, 2018-09-28) (which was not really a coding style cleanup).\n>\n> The change was actually not done right, if I read the commit correctly,\n> because it added a new parameter _to the end_, even if the\n> `check_parents()` function took an arbitrary number of parameters already.\n> And indeed, it changed the `\"$@\"` into a \"$1\", pretending that only one\n> parent would be passed.\n>\n> Now, I do not really understand under what circumstances multiple parents\n> could be passed to `check_parents()`, but I think that it does not matter\n> whether you use `--format=%P` or `^@` (the former was changed to the\n> latter in 19ad68d95d6 (subtree: performance improvement for finding\n> unexpected parent commits, 2018-10-12)), you can always get an arbitrary\n> number of parents that way.\n>\n> The natural thing, now, would be to move the added `indent` parameter to\n> the front of the parameter list, but I see that there was some cleanup in\n> e9525a8a029 (subtree: have $indent actually affect indentation,\n> 2021-04-27) which _removed_ that `indent` parameter.\n\nThanks,\nDscho\n"},{"id":"443397","messageId":"pull.1086.v4.git.1638929518657.gitgitgadget@gmail.com","threadId":"57007","inReplyTo":"pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com","subject":"[PATCH v4] subtree: fix argument handling in check_parents","fromName":"James Limbouris via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-08T02:11:58Z","receivedAt":"2021-12-08T02:12:03Z","isPatch":true,"sender":{"key":"james@digitalmatter.com","avatar":"https://avatars.githubusercontent.com/u/28915246?v=4"},"body":"From: James Limbouris <james@digitalmatter.com>\n\n315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28)\nchanged the signature of check_parents from 'check_parents [REV...]'\nto 'check_parents PARENTS_EXPR INDENT'. In other words the variable list\nof parent revisions became a list embedded in a string. However it\nneglected to unpack the list again before sending it to cache_miss,\nleading to incorrect calls whenever more than one parent was present.\nThis is the case whenever a merge commit is processed, with the end\nresult being a loss of performance from unecessary rechecks.\n\nThe indent parameter was subsequently removed in e9525a8a029 (subtree:\nhave $indent actually affect indentation, 2021-04-27), but the argument\nhandling bug remained.\n\nFor consistency, take multiple arguments in check_parents,\nand pass all of them to cache_miss separately.\n\nSigned-off-by: James Limbouris <james@digitalmatter.com>\n---\n    subtree: fix argument handling in check_parents\n    \n    > I saw that you sent a v3, but did not see any of this information\n    > (which took a good while to assemble, as you might have guessed) in\n    > the commit message. However, I think that message would make for the\n    > best home for this information.\n    \n    Sorry Dscho - it wasn't 100% clear to me which details were required.\n    I've rerolled and tried again. Also sorry if I'm not replying to the\n    mail correctly - I'm not actually subscribed to the list, and this seems\n    like the only easy way to get text onto it through gitgitgadget without\n    fighting Outlook.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1086\n\nRange-diff vs v3:\n\n 1:  f734ca9d276 ! 1:  cdc5295a7ac subtree: fix argument handling in check_parents\n     @@ Metadata\n       ## Commit message ##\n          subtree: fix argument handling in check_parents\n      \n     -    check_parents was taking all of its arguments as a single string,\n     -    and erroneously passing them to cache_miss as a single string. For\n     -    commits with a single parent this would succeed, but whenever a merge\n     -    commit was processed, cache_miss would be passed \"parent1 parent2\"\n     -    instead of \"parent1\" \"parent2\" and fail, leading to unecessary rechecks\n     -    of the parent commits.\n     +    315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28)\n     +    changed the signature of check_parents from 'check_parents [REV...]'\n     +    to 'check_parents PARENTS_EXPR INDENT'. In other words the variable list\n     +    of parent revisions became a list embedded in a string. However it\n     +    neglected to unpack the list again before sending it to cache_miss,\n     +    leading to incorrect calls whenever more than one parent was present.\n     +    This is the case whenever a merge commit is processed, with the end\n     +    result being a loss of performance from unecessary rechecks.\n     +\n     +    The indent parameter was subsequently removed in e9525a8a029 (subtree:\n     +    have $indent actually affect indentation, 2021-04-27), but the argument\n     +    handling bug remained.\n      \n          For consistency, take multiple arguments in check_parents,\n          and pass all of them to cache_miss separately.\n\n\n contrib/subtree/git-subtree.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7f767b5c38f..71f1fd94bde 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -296,10 +296,9 @@ cache_miss () {\n \tdone\n }\n \n-# Usage: check_parents PARENTS_EXPR\n+# Usage: check_parents [REVS...]\n check_parents () {\n-\tassert test $# = 1\n-\tmissed=$(cache_miss \"$1\") || exit $?\n+\tmissed=$(cache_miss \"$@\") || exit $?\n \tlocal indent=$(($indent + 1))\n \tfor miss in $missed\n \tdo\n@@ -753,7 +752,7 @@ process_split_commit () {\n \tfi\n \tcreatecount=$(($createcount + 1))\n \tdebug \"parents: $parents\"\n-\tcheck_parents \"$parents\"\n+\tcheck_parents $parents\n \tnewparents=$(cache_get $parents) || exit $?\n \tdebug \"newparents: $newparents\"\n \n\nbase-commit: e9d7761bb94f20acc98824275e317fa82436c25d\n-- \ngitgitgadget\n"},{"id":"445392","messageId":"nycvar.QRO.7.76.6.2201041319300.7076@tvgsbejvaqbjf.bet","threadId":"57007","inReplyTo":"pull.1086.v4.git.1638929518657.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] subtree: fix argument handling in check_parents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-01-04T12:21:31Z","receivedAt":"2022-01-04T12:21:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi James,\n\nOn Wed, 8 Dec 2021, James Limbouris via GitGitGadget wrote:\n\n> From: James Limbouris <james@digitalmatter.com>\n>\n> 315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28)\n> changed the signature of check_parents from 'check_parents [REV...]'\n> to 'check_parents PARENTS_EXPR INDENT'. In other words the variable list\n> of parent revisions became a list embedded in a string. However it\n> neglected to unpack the list again before sending it to cache_miss,\n> leading to incorrect calls whenever more than one parent was present.\n> This is the case whenever a merge commit is processed, with the end\n> result being a loss of performance from unecessary rechecks.\n>\n> The indent parameter was subsequently removed in e9525a8a029 (subtree:\n> have $indent actually affect indentation, 2021-04-27), but the argument\n> handling bug remained.\n>\n> For consistency, take multiple arguments in check_parents,\n> and pass all of them to cache_miss separately.\n>\n> Signed-off-by: James Limbouris <james@digitalmatter.com>\n> ---\n>     subtree: fix argument handling in check_parents\n>\n>     > I saw that you sent a v3, but did not see any of this information\n>     > (which took a good while to assemble, as you might have guessed) in\n>     > the commit message. However, I think that message would make for the\n>     > best home for this information.\n>\n>     Sorry Dscho - it wasn't 100% clear to me which details were required.\n>     I've rerolled and tried again. Also sorry if I'm not replying to the\n>     mail correctly - I'm not actually subscribed to the list, and this seems\n>     like the only easy way to get text onto it through gitgitgadget without\n>     fighting Outlook.\n\nIt is not Outlook you're fighting. It is the decision by the majordomo of\nthe Git mailing list to drop @outlook.com and @hotmail.com mails. Because\nwho would use those email addresses, amirite?\n\nIn any case, thank you so much for sending the fixed commit, and sorry for\nnot reviewing it earlier. It looks good to me! With this commit message, I\nthink it is good to go.\n\nThanks,\nDscho\n"},{"id":"445463","messageId":"xmqqk0ffclol.fsf@gitster.g","threadId":"57007","inReplyTo":"nycvar.QRO.7.76.6.2201041319300.7076@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4] subtree: fix argument handling in check_parents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T20:04:10Z","receivedAt":"2022-01-04T20:04:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> In any case, thank you so much for sending the fixed commit, and sorry for\n> not reviewing it earlier. It looks good to me! With this commit message, I\n> think it is good to go.\n\nThanks, both.  Will replace.\n"}]}