patchsubtree: fix argument handling in check_parents

From: James Limbouris <james@digitalmatter.com>

check_parents was taking all of its arguments as a single string, and erroneously passing them to cache_miss as a single string. cache_miss would then fail, and the spurious cache misses it produced would hurt performance.

For consistency, take multiple arguments in check_parents, and pass all of them to cache_miss separately.

Signed-off-by: James Limbouris <james@digitalmatter.com>
---
    subtree: fix argument handling in check_parents
    
    Hello git developers. Please consider this small patch that fixes a bug
    introduced during a coding style cleanup of the subtree command. Changes
    to the argument handling were causing check_parents to fail when more
    than one parent was supplied, which led to a small loss of performance.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1086

 contrib/subtree/git-subtree.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

"James Limbouris via GitGitGadget" <gitgitgadget@gmail.com> writes:

I do not do "git subtree", and this cannot really be a proper review that is more than "Looks OK from a cursory look", but anyway...

It seems that 315a84f9 (subtree: use commits before rejoins for splits, 2018-09-28) is what broke the logic, but it does not look like a coding style clean-up to me.

We know at this point each of $1, $2, etc. have exactly one revision, and we want cache_miss function to take one revision per its parameter, so writing "$@" is much more preferrable over $* even though they do the same thing in practice in the context of this code, I think.

From: James Limbouris <james@digitalmatter.com>

check_parents was taking all of its arguments as a single string, and erroneously passing them to cache_miss as a single string. cache_miss would then fail, and the spurious cache misses it produced would hurt performance.

For consistency, take multiple arguments in check_parents, and pass all of them to cache_miss separately.

Signed-off-by: James Limbouris <james@digitalmatter.com>
---
    subtree: fix argument handling in check_parents
    
    > It seems that 315a84f9 (subtree: use commits before rejoins for
    > splits, 2018-09-28) is what broke the logic, but it does not look like
    > a coding style clean-up to me.
    
    Sorry - you're right. I thought it was 6ae6a2 (subtree: adjust style to
    match CodingGuidelines) that broke it, but it was actually the addition
    of quotes in the check_parents and cache_miss calls of 315a84f9.
    
    > so writing "$@" is much more preferrable over $*
    
    Yes agreed - thanks!

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1086

Range-diff vs v1:

 1:  11b65a7b3a5 ! 1:  1198a84995b subtree: fix argument handling in check_parents
     @@ contrib/subtree/git-subtree.sh: cache_miss () {
       check_parents () {
      -	assert test $# = 1
      -	missed=$(cache_miss "$1") || exit $?
     -+	missed=$(cache_miss $*) || exit $?
     ++	missed=$(cache_miss "$@") || exit $?
       	local indent=$(($indent + 1))
       	for miss in $missed
       	do

 contrib/subtree/git-subtree.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

From: James Limbouris <james@digitalmatter.com>

check_parents was taking all of its arguments as a single string, and erroneously passing them to cache_miss as a single string. For commits with a single parent this would succeed, but whenever a merge commit was processed, cache_miss would be passed "parent1 parent2" instead of "parent1" "parent2" and fail, leading to unecessary rechecks of the parent commits.

For consistency, take multiple arguments in check_parents, and pass all of them to cache_miss separately.

Signed-off-by: James Limbouris <james@digitalmatter.com>
---
    subtree: fix argument handling in check_parents
    
    > I agree with this. I'll mark the topic as "Expecting a reroll" in the
    > next "What's cooking" report, so that we do not forget that we are
    > waiting for such an improved description in the log message.
    
    Thanks for the feedback - log message has been amended.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1086

Range-diff vs v2:

 1:  1198a84995b ! 1:  f734ca9d276 subtree: fix argument handling in check_parents
     @@ Commit message
          subtree: fix argument handling in check_parents
      
          check_parents was taking all of its arguments as a single string,
     -    and erroneously passing them to cache_miss as a single string.
     -    cache_miss would then fail, and the spurious cache misses it produced
     -    would hurt performance.
     +    and erroneously passing them to cache_miss as a single string. For
     +    commits with a single parent this would succeed, but whenever a merge
     +    commit was processed, cache_miss would be passed "parent1 parent2"
     +    instead of "parent1" "parent2" and fail, leading to unecessary rechecks
     +    of the parent commits.
      
          For consistency, take multiple arguments in check_parents,
          and pass all of them to cache_miss separately.

 contrib/subtree/git-subtree.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

From: James Limbouris <james@digitalmatter.com>

315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28) changed the signature of check_parents from 'check_parents [REV...]' to 'check_parents PARENTS_EXPR INDENT'. In other words the variable list of parent revisions became a list embedded in a string. However it neglected to unpack the list again before sending it to cache_miss, leading to incorrect calls whenever more than one parent was present. This is the case whenever a merge commit is processed, with the end result being a loss of performance from unecessary rechecks.

The indent parameter was subsequently removed in e9525a8a029 (subtree: have $indent actually affect indentation, 2021-04-27), but the argument handling bug remained.

For consistency, take multiple arguments in check_parents, and pass all of them to cache_miss separately.

Signed-off-by: James Limbouris <james@digitalmatter.com>
---
    subtree: fix argument handling in check_parents
    
    > I saw that you sent a v3, but did not see any of this information
    > (which took a good while to assemble, as you might have guessed) in
    > the commit message. However, I think that message would make for the
    > best home for this information.
    
    Sorry Dscho - it wasn't 100% clear to me which details were required.
    I've rerolled and tried again. Also sorry if I'm not replying to the
    mail correctly - I'm not actually subscribed to the list, and this seems
    like the only easy way to get text onto it through gitgitgadget without
    fighting Outlook.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1086%2Fjamesl-dm%2Fmaint-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1086/jamesl-dm/maint-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1086

Range-diff vs v3:

 1:  f734ca9d276 ! 1:  cdc5295a7ac subtree: fix argument handling in check_parents
     @@ Metadata
       ## Commit message ##
          subtree: fix argument handling in check_parents
      
     -    check_parents was taking all of its arguments as a single string,
     -    and erroneously passing them to cache_miss as a single string. For
     -    commits with a single parent this would succeed, but whenever a merge
     -    commit was processed, cache_miss would be passed "parent1 parent2"
     -    instead of "parent1" "parent2" and fail, leading to unecessary rechecks
     -    of the parent commits.
     +    315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28)
     +    changed the signature of check_parents from 'check_parents [REV...]'
     +    to 'check_parents PARENTS_EXPR INDENT'. In other words the variable list
     +    of parent revisions became a list embedded in a string. However it
     +    neglected to unpack the list again before sending it to cache_miss,
     +    leading to incorrect calls whenever more than one parent was present.
     +    This is the case whenever a merge commit is processed, with the end
     +    result being a loss of performance from unecessary rechecks.
     +
     +    The indent parameter was subsequently removed in e9525a8a029 (subtree:
     +    have $indent actually affect indentation, 2021-04-27), but the argument
     +    handling bug remained.
      
          For consistency, take multiple arguments in check_parents,
          and pass all of them to cache_miss separately.

 contrib/subtree/git-subtree.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Hi James,

On Wed, 8 Dec 2021, James Limbouris via GitGitGadget wrote:

It is not Outlook you're fighting. It is the decision by the majordomo of the Git mailing list to drop @outlook.com and @hotmail.com mails. Because who would use those email addresses, amirite?

In any case, thank you so much for sending the fixed commit, and sorry for not reviewing it earlier. It looks good to me! With this commit message, I think it is good to go.

Thanks, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Thanks, both.  Will replace.

Hi James,

On Wed, 1 Dec 2021, James Limbouris via GitGitGadget wrote:

When I look through the commit history of `git-subtree.sh`, I see that the change was introduced in 315a84f9aa0 (subtree: use commits before rejoins for splits, 2018-09-28) (which was not really a coding style cleanup).

The change was actually not done right, if I read the commit correctly, because it added a new parameter _to the end_, even if the `check_parents()` function took an arbitrary number of parameters already. And indeed, it changed the `"$@"` into a "$1", pretending that only one parent would be passed.

Now, I do not really understand under what circumstances multiple parents could be passed to `check_parents()`, but I think that it does not matter whether you use `--format=%P` or `^@` (the former was changed to the latter in 19ad68d95d6 (subtree: performance improvement for finding unexpected parent commits, 2018-10-12)), you can always get an arbitrary number of parents that way.

The natural thing, now, would be to move the added `indent` parameter to the front of the parameter list, but I see that there was some cleanup in e9525a8a029 (subtree: have $indent actually affect indentation, 2021-04-27) which _removed_ that `indent` parameter.

So I _think_ your change is correct, even if I would love to see an easy-to-understand description of the scenario where more than one parents might be checked.

Another thing I would like to see is probably even more important: rather than using $*, we should use the original "$@" instead (with double-quotes). It should not matter a lot right now because we know that the parameters are the output of `git rev-parse "$rev^@"` (which provides them as a list of full object IDs, i.e. containing no white-space except to delimit the IDs), but it still the correct form to use "$@" instead.

Thanks, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I agree with this. I'll mark the topic as "Expecting a reroll" in the next "What's cooking" report, so that we do not forget that we are waiting for such an improved description in the log message.

Thanks.

Hi James,

I saw that you sent a v3, but did not see any of this information (which took a good while to assemble, as you might have guessed) in the commit message. However, I think that message would make for the best home for this information:

On Fri, 3 Dec 2021, Johannes Schindelin wrote:

Thanks, Dscho