git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4] subtree: fix argument handling in check_parents

From
James Limbouris via GitGitGadget <gitgitgadget@gmail.com>
Date
Dec 8, 2021, 02:11 UTC
Message-ID
<pull.1086.v4.git.1638929518657.gitgitgadget@gmail.com>
In-Reply-To
<pull.1086.v3.git.1638758742741.gitgitgadget@gmail.com>
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
Previous: James Limbouris via GitGitGadgetNext: Johannes Schindelin
Message 5 of 10 in “subtree: fix argument handling in check_parents”
  1. subtree: fix argument handling in check_parentsJames Limbouris via GitGitGadget, Dec 1, 2021
  2. Junio C HamanoDec 1, 2021
  3. subtree: fix argument handling in check_parentsJames Limbouris via GitGitGadget, Dec 2, 2021
  4. subtree: fix argument handling in check_parentsJames Limbouris via GitGitGadget, Dec 6, 2021
  5. subtree: fix argument handling in check_parentsJames Limbouris via GitGitGadget, Dec 8, 2021
  6. Johannes SchindelinJan 4, 2022
  7. Junio C HamanoJan 4, 2022
  8. Johannes SchindelinDec 3, 2021
  9. Junio C HamanoDec 3, 2021
  10. Johannes SchindelinDec 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.