[PATCH] subtree: fix argument handling in check_parents

[PATCH] subtree: fix argument handling in check_parents

[James Limbouris via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7f767b5c38f..56f24000c2c 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,10 +296,9 @@ cache_miss () {
 	done
 }
 
-# Usage: check_parents PARENTS_EXPR
+# Usage: check_parents [REVS...]
 check_parents () {
-	assert test $# = 1
-	missed=$(cache_miss "$1") || exit $?
+	missed=$(cache_miss $*) || exit $?
 	local indent=$(($indent + 1))
 	for miss in $missed
 	do
@@ -753,7 +752,7 @@ process_split_commit () {
 	fi
 	createcount=$(($createcount + 1))
 	debug "parents: $parents"
-	check_parents "$parents"
+	check_parents $parents
 	newparents=$(cache_get $parents) || exit $?
 	debug "newparents: $newparents"
 

base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
-- 
gitgitgadget

Re: [PATCH] subtree: fix argument handling in check_parents

[Junio C Hamano]

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

> 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.

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.

> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 7f767b5c38f..56f24000c2c 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -296,10 +296,9 @@ cache_miss () {
>  	done
>  }
>  
> -# Usage: check_parents PARENTS_EXPR
> +# Usage: check_parents [REVS...]
>  check_parents () {
> -	assert test $# = 1
> -	missed=$(cache_miss "$1") || exit $?
> +	missed=$(cache_miss $*) || exit $?

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.

>  	local indent=$(($indent + 1))
>  	for miss in $missed
>  	do
> @@ -753,7 +752,7 @@ process_split_commit () {
>  	fi
>  	createcount=$(($createcount + 1))
>  	debug "parents: $parents"
> -	check_parents "$parents"
> +	check_parents $parents
>  	newparents=$(cache_get $parents) || exit $?
>  	debug "newparents: $newparents"
>  
>
> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d

[PATCH v2] subtree: fix argument handling in check_parents

[James Limbouris via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7f767b5c38f..71f1fd94bde 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,10 +296,9 @@ cache_miss () {
 	done
 }
 
-# Usage: check_parents PARENTS_EXPR
+# Usage: check_parents [REVS...]
 check_parents () {
-	assert test $# = 1
-	missed=$(cache_miss "$1") || exit $?
+	missed=$(cache_miss "$@") || exit $?
 	local indent=$(($indent + 1))
 	for miss in $missed
 	do
@@ -753,7 +752,7 @@ process_split_commit () {
 	fi
 	createcount=$(($createcount + 1))
 	debug "parents: $parents"
-	check_parents "$parents"
+	check_parents $parents
 	newparents=$(cache_get $parents) || exit $?
 	debug "newparents: $newparents"
 

base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
-- 
gitgitgadget

[PATCH v3] subtree: fix argument handling in check_parents

[James Limbouris via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7f767b5c38f..71f1fd94bde 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,10 +296,9 @@ cache_miss () {
 	done
 }
 
-# Usage: check_parents PARENTS_EXPR
+# Usage: check_parents [REVS...]
 check_parents () {
-	assert test $# = 1
-	missed=$(cache_miss "$1") || exit $?
+	missed=$(cache_miss "$@") || exit $?
 	local indent=$(($indent + 1))
 	for miss in $missed
 	do
@@ -753,7 +752,7 @@ process_split_commit () {
 	fi
 	createcount=$(($createcount + 1))
 	debug "parents: $parents"
-	check_parents "$parents"
+	check_parents $parents
 	newparents=$(cache_get $parents) || exit $?
 	debug "newparents: $newparents"
 

base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
-- 
gitgitgadget

[PATCH v4] subtree: fix argument handling in check_parents

[James Limbouris via GitGitGadget]

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(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7f767b5c38f..71f1fd94bde 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -296,10 +296,9 @@ cache_miss () {
 	done
 }
 
-# Usage: check_parents PARENTS_EXPR
+# Usage: check_parents [REVS...]
 check_parents () {
-	assert test $# = 1
-	missed=$(cache_miss "$1") || exit $?
+	missed=$(cache_miss "$@") || exit $?
 	local indent=$(($indent + 1))
 	for miss in $missed
 	do
@@ -753,7 +752,7 @@ process_split_commit () {
 	fi
 	createcount=$(($createcount + 1))
 	debug "parents: $parents"
-	check_parents "$parents"
+	check_parents $parents
 	newparents=$(cache_get $parents) || exit $?
 	debug "newparents: $newparents"
 

base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
-- 
gitgitgadget

Re: [PATCH v4] subtree: fix argument handling in check_parents

[Johannes Schindelin]

Hi James,

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

> 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.

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

Re: [PATCH v4] subtree: fix argument handling in check_parents

[Junio C Hamano]

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

> 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, both.  Will replace.

Re: [PATCH] subtree: fix argument handling in check_parents

[Johannes Schindelin]

Hi James,


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

> 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.

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

>
> 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(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 7f767b5c38f..56f24000c2c 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -296,10 +296,9 @@ cache_miss () {
>  	done
>  }
>
> -# Usage: check_parents PARENTS_EXPR
> +# Usage: check_parents [REVS...]
>  check_parents () {
> -	assert test $# = 1
> -	missed=$(cache_miss "$1") || exit $?
> +	missed=$(cache_miss $*) || exit $?
>  	local indent=$(($indent + 1))
>  	for miss in $missed
>  	do
> @@ -753,7 +752,7 @@ process_split_commit () {
>  	fi
>  	createcount=$(($createcount + 1))
>  	debug "parents: $parents"
> -	check_parents "$parents"
> +	check_parents $parents
>  	newparents=$(cache_get $parents) || exit $?
>  	debug "newparents: $newparents"
>
>
> base-commit: e9d7761bb94f20acc98824275e317fa82436c25d
> --
> gitgitgadget
>

Re: [PATCH] subtree: fix argument handling in check_parents

[Junio C Hamano]

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

> 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.

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.

Re: [PATCH] subtree: fix argument handling in check_parents

[Johannes Schindelin]

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:

> On Wed, 1 Dec 2021, James Limbouris via GitGitGadget wrote:
>
> > 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.
>
> 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.

Thanks,
Dscho