Re: [PATCH] subtree: fix argument handling in check_parents
From: Junio C Hamano <hidden>
Date: 2021-12-01 23:05:03
"James Limbouris via GitGitGadget" [off-list ref] writes:
From: James Limbouris <redacted>
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 <redacted>
---
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.
quoted hunk
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.
quoted hunk
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