subtree: fix argument handling in check_parents

From: James Limbouris <[email protected]>

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.

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

index 7f767b5c38f..56f24000c2c 100755
@@ -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

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.

From: James Limbouris <[email protected]>

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.

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

index 7f767b5c38f..71f1fd94bde 100755
@@ -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

Hi James,

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

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.

From: James Limbouris <[email protected]>

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.

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

index 7f767b5c38f..71f1fd94bde 100755
@@ -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

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:

Thanks,
Dscho

From: James Limbouris <[email protected]>

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.

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

index 7f767b5c38f..71f1fd94bde 100755
@@ -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

Hi James,

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

Thanks, both. Will replace.