{"thread":{"id":"58669","subject":"[PATCH 0/9] subtree: fix split and merge after annotated tag was squash-merged","startedAt":"2022-10-21T15:14:32Z","lastAt":"2022-10-26T21:38:49Z","messageCount":20,"participants":["Philippe Blain via GitGitGadget","Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"465468","messageId":"pull.1390.git.1666365219.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":null,"subject":"[PATCH 0/9] subtree: fix split and merge after annotated tag was squash-merged","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:30Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"This series fixes a limitation of 'git subtree merge' and 'git subtree\nsplit' that would fail when the previous squash-merge merged an annotated\ntag of the subtree repository that is missing locally.\n\nThe 5 first commits are small improvements for coherency with the rest of\nthe code base, robustness, user experience and maintanability.\n\nCommits 6-7 are small refactors that prepare for the fix in the next\ncommits. Commits 8-9 fix the behaviour for merge and split, respectively.\n\nI'm CC-ing people that worked on 'git subtree' recently.\n\nCheers!\n\nPhilippe Blain (9):\n  test-lib-functions: mark 'test_commit' variables as 'local'\n  subtree: use 'git rev-parse --verify [--quiet]' for better error\n    messages\n  subtree: add 'die_incompatible_opt' function to reduce duplication\n  subtree: prefix die messages with 'fatal'\n  subtree: define a variable before its first use in\n    'find_latest_squash'\n  subtree: use named variables instead of \"$@\" in cmd_pull\n  subtree: process 'git-subtree-split' trailer in separate function\n  subtree: fix squash merging after annotated tag was squashed merged\n  subtree: fix split after annotated tag was squashed merged\n\n contrib/subtree/git-subtree.sh     | 177 +++++++++++++++++++----------\n contrib/subtree/git-subtree.txt    |  16 ++-\n contrib/subtree/t/t7900-subtree.sh |  60 +++++++++-\n t/test-lib-functions.sh            |  16 +--\n 4 files changed, 192 insertions(+), 77 deletions(-)\n\n\nbase-commit: 45c9f05c44b1cb6bd2d6cb95a22cf5e3d21d5b63\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1390%2Fphil-blain%2Fsubtree-pull-tag-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1390/phil-blain/subtree-pull-tag-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1390\n-- \ngitgitgadget\n"},{"id":"465469","messageId":"c890f55f59994231be6114f76f020510eb824453.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:33Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\n9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)\nadded validation code to check that options given to 'git subtree <cmd>'\nmade sense with the command being used.\n\nRefactor these checks by adding a 'die_incompatible_opt' function to\nreduce code duplication.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 49ef493ef92..f5eab198c80 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -102,6 +102,14 @@ assert () {\n \tfi\n }\n \n+# Usage: die_incompatible_opt OPTION COMMAND\n+die_incompatible_opt () {\n+\tassert test \"$#\" = 2\n+\topt=\"$1\"\n+\targ_command=\"$2\"\n+\tdie \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+}\n+\n main () {\n \tif test $# -eq 0\n \tthen\n@@ -176,16 +184,16 @@ main () {\n \t\t\targ_debug=1\n \t\t\t;;\n \t\t--annotate)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_annotate=\"$1\"\n \t\t\tshift\n \t\t\t;;\n \t\t--no-annotate)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_annotate=\n \t\t\t;;\n \t\t-b)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_branch=\"$1\"\n \t\t\tshift\n \t\t\t;;\n@@ -194,7 +202,7 @@ main () {\n \t\t\tshift\n \t\t\t;;\n \t\t-m)\n-\t\t\ttest -n \"$allow_addmerge\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_addmerge\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_addmerge_message=\"$1\"\n \t\t\tshift\n \t\t\t;;\n@@ -202,34 +210,34 @@ main () {\n \t\t\targ_prefix=\n \t\t\t;;\n \t\t--onto)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_onto=\"$1\"\n \t\t\tshift\n \t\t\t;;\n \t\t--no-onto)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_onto=\n \t\t\t;;\n \t\t--rejoin)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\t;;\n \t\t--no-rejoin)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\t;;\n \t\t--ignore-joins)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_ignore_joins=1\n \t\t\t;;\n \t\t--no-ignore-joins)\n-\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_split_ignore_joins=\n \t\t\t;;\n \t\t--squash)\n-\t\t\ttest -n \"$allow_addmerge\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_addmerge\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_addmerge_squash=1\n \t\t\t;;\n \t\t--no-squash)\n-\t\t\ttest -n \"$allow_addmerge\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\t\t\ttest -n \"$allow_addmerge\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n \t\t\targ_addmerge_squash=\n \t\t\t;;\n \t\t--)\n-- \ngitgitgadget\n\n"},{"id":"465470","messageId":"832e910aadc47309392feed69927c2e8693cbfc5.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:31Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSome variables in 'test_commit' have names that are common enough that\nit is very likely that test authors might use them in a test. If they do\nso and use 'test_commit' between setting such a variable and using it,\nthe variable value from 'test_commit' will leak back into the test and\nmost likely break it.\n\nPrevent that by marking all variables in 'test_commit' as 'local'. This\nallow a subsequent commit to use a 'tag' variable.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n t/test-lib-functions.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 527a7145000..adc0fb6330c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -273,13 +273,13 @@ debug () {\n # <file>, <contents>, and <tag> all default to <message>.\n \n test_commit () {\n-\tnotick= &&\n-\techo=echo &&\n-\tappend= &&\n-\tauthor= &&\n-\tsignoff= &&\n-\tindir= &&\n-\ttag=light &&\n+\tlocal notick= &&\n+\tlocal echo=echo &&\n+\tlocal append= &&\n+\tlocal author= &&\n+\tlocal signoff= &&\n+\tlocal indir= &&\n+\tlocal tag=light &&\n \twhile test $# != 0\n \tdo\n \t\tcase \"$1\" in\n@@ -322,7 +322,7 @@ test_commit () {\n \t\tshift\n \tdone &&\n \tindir=${indir:+\"$indir\"/} &&\n-\tfile=${2:-\"$1.t\"} &&\n+\tlocal file=${2:-\"$1.t\"} &&\n \tif test -n \"$append\"\n \tthen\n \t\t$echo \"${3-$1}\" >>\"$indir$file\"\n-- \ngitgitgadget\n\n"},{"id":"465471","messageId":"200f7341c91df2cd616cbc4bdcd8025bed3af7ea.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 5/9] subtree: define a variable before its first use in 'find_latest_squash'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:35Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe function 'find_latest_squash' takes a single argument, 'dir', but a\ndebug statement uses this variable before it takes its value from $1.\n\nThis statement thus gets the value of 'dir' from the calling function,\nwhich currently is the same as the 'dir' argument, so it works but it\nis confusing.\n\nMove the definition of 'dir' before its first use.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 89f1eb756f0..d91a9679075 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -374,10 +374,10 @@ try_remove_previous () {\n # Usage: find_latest_squash DIR\n find_latest_squash () {\n \tassert test $# = 1\n+\tdir=\"$1\"\n \tdebug \"Looking for latest squash ($dir)...\"\n \tlocal indent=$(($indent + 1))\n \n-\tdir=\"$1\"\n \tsq=\n \tmain=\n \tsub=\n-- \ngitgitgadget\n\n"},{"id":"465472","messageId":"040bc2224c9e42fb8d6fe6d2c01fcbe57a938613.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 2/9] subtree: use 'git rev-parse --verify [--quiet]' for better error messages","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:32Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThere are three occurences of 'git rev-parse <rev>' in 'git-subtree.sh'\nwhere the command expects a revision and the script dies or exits if the\nrevision can't be found. In that case, the error message from 'git\nrev-parse' is:\n\n    $ git rev-parse <bad rev>\n    <bad rev>\n    fatal: ambiguous argument '<bad rev>': unknown revision or path not in the working tree.\n    Use '--' to separate paths from revisions, like this:\n    'git <command> [<revision>...] -- [<file>...]'\n\nThis is a little confusing to the user, since this error message is\noutputed by 'git subtree'.\n\nAt these points in the script, we know that we are looking for a single\nrevision, so be explicit by using '--verify', resulting in a little\nbetter error message:\n\n    $ git rev-parse --verify <bad rev>\n    fatal: Needed a single revision\n\nIn the two occurences where we 'die' if 'git rev-parse' fails, 'git\nsubtree' outputs \"could not rev-parse split hash $b from commit $sq\", so\nwe actually do not need the supplementary error message from 'git\nrev-parse'; add '--quiet' to silence it.\n\nIn the third occurence, we 'exit', so keep the error message from 'git\nrev-parse'. Note that this messsage is still suboptimal since it can be\nunderstood to mean that 'git rev-parse' did not receive a single\nrevision as argument, which is not the case here: the command did\nreceive a single revision, but the revision is not resolvable to an\navailable object.\n\nThe alternative would be to use '--' after the revision, as suggested by\nthe first error message, resulting in a clearer error message:\n\n    $ git rev-parse <bad rev> --\n    fatal: bad revision '<bad rev>'\n\nUnfortunately we can't use that syntax because in the more common case\nof the revision resolving to a known object, the command outputs the\nobject's hash, a newline, and the dashdash, which breaks the 'git\nsubtree' script.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 7562a395c24..49ef493ef92 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -387,7 +387,7 @@ find_latest_squash () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tsub=\"$(git rev-parse \"$b^{commit}\")\" ||\n+\t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n \t\t\tdie \"could not rev-parse split hash $b from commit $sq\"\n \t\t\t;;\n \t\tEND)\n@@ -439,7 +439,7 @@ find_existing_splits () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tsub=\"$(git rev-parse \"$b^{commit}\")\" ||\n+\t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n \t\t\tdie \"could not rev-parse split hash $b from commit $sq\"\n \t\t\t;;\n \t\tEND)\n@@ -843,7 +843,7 @@ cmd_add_commit () {\n \tgit checkout -- \"$dir\" || exit $?\n \ttree=$(git write-tree) || exit $?\n \n-\theadrev=$(git rev-parse HEAD) || exit $?\n+\theadrev=$(git rev-parse --verify HEAD) || exit $?\n \tif test -n \"$headrev\" && test \"$headrev\" != \"$rev\"\n \tthen\n \t\theadp=\"-p $headrev\"\n-- \ngitgitgadget\n\n"},{"id":"465473","messageId":"a70fda5582d6bd84b8bedaba33768d3886846090.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 4/9] subtree: prefix die messages with 'fatal'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:34Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nJust as was done in 0008d12284 (submodule: prefix die messages with\n'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages\noutputed by 'git-subtree.sh' more in line with the rest of the code base\nby prefixing them with \"fatal: \", and do not capitalize their first\nletter.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh     | 60 +++++++++++++++---------------\n contrib/subtree/t/t7900-subtree.sh | 12 +++---\n 2 files changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex f5eab198c80..89f1eb756f0 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -98,7 +98,7 @@ progress () {\n assert () {\n \tif ! \"$@\"\n \tthen\n-\t\tdie \"assertion failed: $*\"\n+\t\tdie \"fatal: assertion failed: $*\"\n \tfi\n }\n \n@@ -107,7 +107,7 @@ die_incompatible_opt () {\n \tassert test \"$#\" = 2\n \topt=\"$1\"\n \targ_command=\"$2\"\n-\tdie \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n+\tdie \"fatal: the '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n }\n \n main () {\n@@ -155,7 +155,7 @@ main () {\n \t\tallow_addmerge=$arg_split_rejoin\n \t\t;;\n \t*)\n-\t\tdie \"Unknown command '$arg_command'\"\n+\t\tdie \"fatal: unknown command '$arg_command'\"\n \t\t;;\n \tesac\n \t# Reset the arguments array for \"real\" flag parsing.\n@@ -244,7 +244,7 @@ main () {\n \t\t\tbreak\n \t\t\t;;\n \t\t*)\n-\t\t\tdie \"Unexpected option: $opt\"\n+\t\t\tdie \"fatal: unexpected option: $opt\"\n \t\t\t;;\n \t\tesac\n \tdone\n@@ -252,17 +252,17 @@ main () {\n \n \tif test -z \"$arg_prefix\"\n \tthen\n-\t\tdie \"You must provide the --prefix option.\"\n+\t\tdie \"fatal: you must provide the --prefix option.\"\n \tfi\n \n \tcase \"$arg_command\" in\n \tadd)\n \t\ttest -e \"$arg_prefix\" &&\n-\t\t\tdie \"prefix '$arg_prefix' already exists.\"\n+\t\t\tdie \"fatal: prefix '$arg_prefix' already exists.\"\n \t\t;;\n \t*)\n \t\ttest -e \"$arg_prefix\" ||\n-\t\t\tdie \"'$arg_prefix' does not exist; use 'git subtree add'\"\n+\t\t\tdie \"fatal: '$arg_prefix' does not exist; use 'git subtree add'\"\n \t\t;;\n \tesac\n \n@@ -282,11 +282,11 @@ cache_setup () {\n \tassert test $# = 0\n \tcachedir=\"$GIT_DIR/subtree-cache/$$\"\n \trm -rf \"$cachedir\" ||\n-\t\tdie \"Can't delete old cachedir: $cachedir\"\n+\t\tdie \"fatal: can't delete old cachedir: $cachedir\"\n \tmkdir -p \"$cachedir\" ||\n-\t\tdie \"Can't create new cachedir: $cachedir\"\n+\t\tdie \"fatal: can't create new cachedir: $cachedir\"\n \tmkdir -p \"$cachedir/notree\" ||\n-\t\tdie \"Can't create new cachedir: $cachedir/notree\"\n+\t\tdie \"fatal: can't create new cachedir: $cachedir/notree\"\n \tdebug \"Using cachedir: $cachedir\" >&2\n }\n \n@@ -342,7 +342,7 @@ cache_set () {\n \t\ttest \"$oldrev\" != \"latest_new\" &&\n \t\ttest -e \"$cachedir/$oldrev\"\n \tthen\n-\t\tdie \"cache for $oldrev already exists!\"\n+\t\tdie \"fatal: cache for $oldrev already exists!\"\n \tfi\n \techo \"$newrev\" >\"$cachedir/$oldrev\"\n }\n@@ -396,7 +396,7 @@ find_latest_squash () {\n \t\t\t;;\n \t\tgit-subtree-split:)\n \t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n-\t\t\tdie \"could not rev-parse split hash $b from commit $sq\"\n+\t\t\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n \t\t\t;;\n \t\tEND)\n \t\t\tif test -n \"$sub\"\n@@ -448,7 +448,7 @@ find_existing_splits () {\n \t\t\t;;\n \t\tgit-subtree-split:)\n \t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n-\t\t\tdie \"could not rev-parse split hash $b from commit $sq\"\n+\t\t\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n \t\t\t;;\n \t\tEND)\n \t\t\tdebug \"Main is: '$main'\"\n@@ -498,7 +498,7 @@ copy_commit () {\n \t\t\tcat\n \t\t) |\n \t\tgit commit-tree \"$2\" $3  # reads the rest of stdin\n-\t) || die \"Can't copy commit $1\"\n+\t) || die \"fatal: can't copy commit $1\"\n }\n \n # Usage: add_msg DIR LATEST_OLD LATEST_NEW\n@@ -726,11 +726,11 @@ ensure_clean () {\n \tassert test $# = 0\n \tif ! git diff-index HEAD --exit-code --quiet 2>&1\n \tthen\n-\t\tdie \"Working tree has modifications.  Cannot add.\"\n+\t\tdie \"fatal: working tree has modifications.  Cannot add.\"\n \tfi\n \tif ! git diff-index --cached HEAD --exit-code --quiet 2>&1\n \tthen\n-\t\tdie \"Index has modifications.  Cannot add.\"\n+\t\tdie \"fatal: index has modifications.  Cannot add.\"\n \tfi\n }\n \n@@ -738,7 +738,7 @@ ensure_clean () {\n ensure_valid_ref_format () {\n \tassert test $# = 1\n \tgit check-ref-format \"refs/heads/$1\" ||\n-\t\tdie \"'$1' does not look like a ref\"\n+\t\tdie \"fatal: '$1' does not look like a ref\"\n }\n \n # Usage: process_split_commit REV PARENTS\n@@ -804,7 +804,7 @@ cmd_add () {\n \tif test $# -eq 1\n \tthen\n \t\tgit rev-parse -q --verify \"$1^{commit}\" >/dev/null ||\n-\t\t\tdie \"'$1' does not refer to a commit\"\n+\t\t\tdie \"fatal: '$1' does not refer to a commit\"\n \n \t\tcmd_add_commit \"$@\"\n \n@@ -819,7 +819,7 @@ cmd_add () {\n \n \t\tcmd_add_repository \"$@\"\n \telse\n-\t\tsay >&2 \"error: parameters were '$*'\"\n+\t\tsay >&2 \"fatal: parameters were '$*'\"\n \t\tdie \"Provide either a commit or a repository and commit.\"\n \tfi\n }\n@@ -882,9 +882,9 @@ cmd_split () {\n \telif test $# -eq 1\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n-\t\t\tdie \"'$1' does not refer to a commit\"\n+\t\t\tdie \"fatal: '$1' does not refer to a commit\"\n \telse\n-\t\tdie \"You must provide exactly one revision.  Got: '$*'\"\n+\t\tdie \"fatal: you must provide exactly one revision.  Got: '$*'\"\n \tfi\n \n \tif test -n \"$arg_split_rejoin\"\n@@ -927,7 +927,7 @@ cmd_split () {\n \tlatest_new=$(cache_get latest_new) || exit $?\n \tif test -z \"$latest_new\"\n \tthen\n-\t\tdie \"No new revisions were found\"\n+\t\tdie \"fatal: no new revisions were found\"\n \tfi\n \n \tif test -n \"$arg_split_rejoin\"\n@@ -948,7 +948,7 @@ cmd_split () {\n \t\tthen\n \t\t\tif ! git merge-base --is-ancestor \"$arg_split_branch\" \"$latest_new\"\n \t\t\tthen\n-\t\t\t\tdie \"Branch '$arg_split_branch' is not an ancestor of commit '$latest_new'.\"\n+\t\t\t\tdie \"fatal: branch '$arg_split_branch' is not an ancestor of commit '$latest_new'.\"\n \t\t\tfi\n \t\t\taction='Updated'\n \t\telse\n@@ -965,9 +965,9 @@ cmd_split () {\n # Usage: cmd_merge REV\n cmd_merge () {\n \ttest $# -eq 1 ||\n-\t\tdie \"You must provide exactly one revision.  Got: '$*'\"\n+\t\tdie \"fatal: you must provide exactly one revision.  Got: '$*'\"\n \trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n-\t\tdie \"'$1' does not refer to a commit\"\n+\t\tdie \"fatal: '$1' does not refer to a commit\"\n \tensure_clean\n \n \tif test -n \"$arg_addmerge_squash\"\n@@ -975,7 +975,7 @@ cmd_merge () {\n \t\tfirst_split=\"$(find_latest_squash \"$dir\")\" || exit $?\n \t\tif test -z \"$first_split\"\n \t\tthen\n-\t\t\tdie \"Can't squash-merge: '$dir' was never added.\"\n+\t\t\tdie \"fatal: can't squash-merge: '$dir' was never added.\"\n \t\tfi\n \t\tset $first_split\n \t\told=$1\n@@ -1003,7 +1003,7 @@ cmd_merge () {\n cmd_pull () {\n \tif test $# -ne 2\n \tthen\n-\t\tdie \"You must provide <repository> <ref>\"\n+\t\tdie \"fatal: you must provide <repository> <ref>\"\n \tfi\n \tensure_clean\n \tensure_valid_ref_format \"$2\"\n@@ -1015,7 +1015,7 @@ cmd_pull () {\n cmd_push () {\n \tif test $# -ne 2\n \tthen\n-\t\tdie \"You must provide <repository> <refspec>\"\n+\t\tdie \"fatal: you must provide <repository> <refspec>\"\n \tfi\n \tif test -e \"$dir\"\n \tthen\n@@ -1030,13 +1030,13 @@ cmd_push () {\n \t\tfi\n \t\tensure_valid_ref_format \"$remoteref\"\n \t\tlocalrev_presplit=$(git rev-parse -q --verify \"$localrevname_presplit^{commit}\") ||\n-\t\t\tdie \"'$localrevname_presplit' does not refer to a commit\"\n+\t\t\tdie \"fatal: '$localrevname_presplit' does not refer to a commit\"\n \n \t\techo \"git push using: \" \"$repository\" \"$refspec\"\n \t\tlocalrev=$(cmd_split \"$localrev_presplit\") || die\n \t\tgit push \"$repository\" \"$localrev\":\"refs/heads/$remoteref\"\n \telse\n-\t\tdie \"'$dir' must already exist. Try 'git subtree add'.\"\n+\t\tdie \"fatal: '$dir' must already exist. Try 'git subtree add'.\"\n \tfi\n }\n \ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 1c1f76f04aa..249743ab9aa 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -277,7 +277,7 @@ test_expect_success 'split requires option --prefix' '\n \t\tcd \"$test_count\" &&\n \t\tgit fetch ./\"sub proj\" HEAD &&\n \t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD &&\n-\t\techo \"You must provide the --prefix option.\" >expected &&\n+\t\techo \"fatal: you must provide the --prefix option.\" >expected &&\n \t\ttest_must_fail git subtree split >actual 2>&1 &&\n \t\ttest_debug \"printf '\"expected: \"'\" &&\n \t\ttest_debug \"cat expected\" &&\n@@ -296,7 +296,7 @@ test_expect_success 'split requires path given by option --prefix must exist' '\n \t\tcd \"$test_count\" &&\n \t\tgit fetch ./\"sub proj\" HEAD &&\n \t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD &&\n-\t\techo \"'\\''non-existent-directory'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n+\t\techo \"fatal: '\\''non-existent-directory'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n \t\ttest_must_fail git subtree split --prefix=non-existent-directory >actual 2>&1 &&\n \t\ttest_debug \"printf '\"expected: \"'\" &&\n \t\ttest_debug \"cat expected\" &&\n@@ -570,7 +570,7 @@ test_expect_success 'pull requires option --prefix' '\n \t\tcd \"$test_count\" &&\n \t\ttest_must_fail git subtree pull ./\"sub proj\" HEAD >out 2>err &&\n \n-\t\techo \"You must provide the --prefix option.\" >expected &&\n+\t\techo \"fatal: you must provide the --prefix option.\" >expected &&\n \t\ttest_must_be_empty out &&\n \t\ttest_cmp expected err\n \t)\n@@ -584,7 +584,7 @@ test_expect_success 'pull requires path given by option --prefix must exist' '\n \t(\n \t\ttest_must_fail git subtree pull --prefix=\"sub dir\" ./\"sub proj\" HEAD >out 2>err &&\n \n-\t\techo \"'\\''sub dir'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n+\t\techo \"fatal: '\\''sub dir'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n \t\ttest_must_be_empty out &&\n \t\ttest_cmp expected err\n \t)\n@@ -643,7 +643,7 @@ test_expect_success 'push requires option --prefix' '\n \t\tcd \"$test_count\" &&\n \t\tgit fetch ./\"sub proj\" HEAD &&\n \t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD &&\n-\t\techo \"You must provide the --prefix option.\" >expected &&\n+\t\techo \"fatal: you must provide the --prefix option.\" >expected &&\n \t\ttest_must_fail git subtree push \"./sub proj\" from-mainline >actual 2>&1 &&\n \t\ttest_debug \"printf '\"expected: \"'\" &&\n \t\ttest_debug \"cat expected\" &&\n@@ -662,7 +662,7 @@ test_expect_success 'push requires path given by option --prefix must exist' '\n \t\tcd \"$test_count\" &&\n \t\tgit fetch ./\"sub proj\" HEAD &&\n \t\tgit subtree add --prefix=\"sub dir\" FETCH_HEAD &&\n-\t\techo \"'\\''non-existent-directory'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n+\t\techo \"fatal: '\\''non-existent-directory'\\'' does not exist; use '\\''git subtree add'\\''\" >expected &&\n \t\ttest_must_fail git subtree push --prefix=non-existent-directory \"./sub proj\" from-mainline >actual 2>&1 &&\n \t\ttest_debug \"printf '\"expected: \"'\" &&\n \t\ttest_debug \"cat expected\" &&\n-- \ngitgitgadget\n\n"},{"id":"465474","messageId":"3e2ea2a8b761fc525c7b9a32d1db3f0e00d7833c.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 7/9] subtree: process 'git-subtree-split' trailer in separate function","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:37Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nBoth functions 'find_latest_squash' (called by 'git subtree merge\n--squash' and 'git subtree split --rejoin') and 'find_existing_splits'\n(called by git 'subtree split') loop through commits that have a\n'git-subtree-dir' trailer, and then process the 'git-subtree-mainline'\nand 'git-subtree-split' trailers for those commits.\n\nThe processing done for the 'git-subtree-split' trailer is simple: we\ncheck if the object exists with 'rev-parse' and set the variable\n'sub' to the object name, or we die if the object does not exist.\n\nIn a future commit we will add more steps to the processing of this\ntrailer in order to make the code more robust.\n\nTo reduce code duplication, move the processing of the\n'git-subtree-split' trailer to a dedicated function,\n'process_subtree_split_trailer'.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 2b3c429991b..b90ca0036f7 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -371,6 +371,15 @@ try_remove_previous () {\n \tfi\n }\n \n+# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH\n+process_subtree_split_trailer () {\n+\tassert test $# = 2\n+\tb=\"$1\"\n+\tsq=\"$2\"\n+\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n+\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n+}\n+\n # Usage: find_latest_squash DIR\n find_latest_squash () {\n \tassert test $# = 1\n@@ -395,8 +404,7 @@ find_latest_squash () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n-\t\t\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n+\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\"\n \t\t\t;;\n \t\tEND)\n \t\t\tif test -n \"$sub\"\n@@ -447,8 +455,7 @@ find_existing_splits () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n-\t\t\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n+\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\"\n \t\t\t;;\n \t\tEND)\n \t\t\tdebug \"Main is: '$main'\"\n-- \ngitgitgadget\n\n"},{"id":"465475","messageId":"86a842d50345f6d4d0b16c78d565474be6f8068a.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 9/9] subtree: fix split after annotated tag was squashed merged","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:39Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe previous commit fixed a failure in 'git subtree merge --squash' when\nthe previous squash-merge merged an annotated tag of the subtree\nrepository which is missing locally.\n\nThe same failure happens in 'git subtree split', either directly or when\ncalled by 'git subtree push', under the same circumstances: 'cmd_split'\ninvokes 'find_existing_splits', which loops through previous commits and\ninvokes 'git rev-parse' (via 'process_subtree_split_trailer') on the\nvalue of any 'git subtree-split' trailer it finds. This fails if this\nvalue is the hash of an annotated tag which is missing locally.\n\nAdd a new optional argument 'repository' to 'cmd_split' and\n'find_existing_splits', and invoke 'cmd_split' with that argument from\n'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch\nthe missing tag from the 'repository' if it's not available locally,\nmirroring the new behaviour of 'git subtree pull' and 'git subtree\nmerge'.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------\n contrib/subtree/git-subtree.txt    |  7 ++++++-\n contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++\n 3 files changed, 36 insertions(+), 9 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 2c67989fe8a..10c9c87839a 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -453,14 +453,19 @@ find_latest_squash () {\n \tdone || exit $?\n }\n \n-# Usage: find_existing_splits DIR REV\n+# Usage: find_existing_splits DIR REV [REPOSITORY]\n find_existing_splits () {\n-\tassert test $# = 2\n+\tassert test $# = 2 -o $# = 3\n \tdebug \"Looking for prior splits...\"\n \tlocal indent=$(($indent + 1))\n \n \tdir=\"$1\"\n \trev=\"$2\"\n+\trepository=\"\"\n+\tif test \"$#\" = 3\n+\tthen\n+\t\trepository=\"$3\"\n+\tfi\n \tmain=\n \tsub=\n \tlocal grep_format=\"^git-subtree-dir: $dir/*\\$\"\n@@ -480,7 +485,7 @@ find_existing_splits () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\"\n+\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\" \"$repository\"\n \t\t\t;;\n \t\tEND)\n \t\t\tdebug \"Main is: '$main'\"\n@@ -906,17 +911,22 @@ cmd_add_commit () {\n \tsay >&2 \"Added dir '$dir'\"\n }\n \n-# Usage: cmd_split [REV]\n+# Usage: cmd_split [REV] [REPOSITORY]\n cmd_split () {\n \tif test $# -eq 0\n \tthen\n \t\trev=$(git rev-parse HEAD)\n-\telif test $# -eq 1\n+\telif test $# -eq 1 -o $# -eq 2\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\t\tdie \"fatal: '$1' does not refer to a commit\"\n \telse\n-\t\tdie \"fatal: you must provide exactly one revision.  Got: '$*'\"\n+\t\tdie \"fatal: you must provide exactly one revision, and optionnally a repository.  Got: '$*'\"\n+\tfi\n+\trepository=\"\"\n+\tif test \"$#\" = 2\n+\tthen\n+\t\trepository=\"$2\"\n \tfi\n \n \tif test -n \"$arg_split_rejoin\"\n@@ -940,7 +950,7 @@ cmd_split () {\n \t\tdone || exit $?\n \tfi\n \n-\tunrevs=\"$(find_existing_splits \"$dir\" \"$rev\")\" || exit $?\n+\tunrevs=\"$(find_existing_splits \"$dir\" \"$rev\" \"$repository\")\" || exit $?\n \n \t# We can't restrict rev-list to only $dir here, because some of our\n \t# parents have the $dir contents the root, and those won't match.\n@@ -1072,7 +1082,7 @@ cmd_push () {\n \t\t\tdie \"fatal: '$localrevname_presplit' does not refer to a commit\"\n \n \t\techo \"git push using: \" \"$repository\" \"$refspec\"\n-\t\tlocalrev=$(cmd_split \"$localrev_presplit\") || die\n+\t\tlocalrev=$(cmd_split \"$localrev_presplit\" \"$repository\") || die\n \t\tgit push \"$repository\" \"$localrev\":\"refs/heads/$remoteref\"\n \telse\n \t\tdie \"fatal: '$dir' must already exist. Try 'git subtree add'.\"\ndiff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt\nindex 0e7524d7864..004abf415b8 100644\n--- a/contrib/subtree/git-subtree.txt\n+++ b/contrib/subtree/git-subtree.txt\n@@ -94,7 +94,7 @@ annotated tag of the subtree repository, that tag needs to be available locally.\n If <repository> is given, a missing tag will automatically be fetched from that\n repository.\n \n-split [<local-commit>]::\n+split [<local-commit>] [<repository>]::\n \tExtract a new, synthetic project history from the\n \thistory of the <prefix> subtree of <local-commit>, or of\n \tHEAD if no <local-commit> is given.  The new history\n@@ -114,6 +114,11 @@ settings passed to 'split' (such as '--annotate') are the same.\n Because of this, if you add new commits and then re-split, the new\n commits will be attached as commits on top of the history you\n generated last time, so 'git merge' and friends will work as expected.\n++\n+When a previous merge with '--squash' merged an annotated tag of the\n+subtree repository, that tag needs to be available locally.\n+If <repository> is given, a missing tag will automatically be fetched from that\n+repository.\n \n pull <repository> <remote-ref>::\n \tExactly like 'merge', but parallels 'git pull' in that\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex d0671676c7a..341c169eca7 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -582,6 +582,12 @@ test_expect_success 'split \"sub dir\"/ with --branch for an incompatible branch'\n \t)\n '\n \n+test_expect_success 'split after annotated tag was added/merged with --squash pre-v2.32.0' '\n+\ttest_create_pre2_32_repo \"$test_count\" &&\n+\ttest_must_fail git -C \"$test_count-clone\" subtree split --prefix=\"sub\" HEAD &&\n+\tgit -C \"$test_count-clone\" subtree split --prefix=\"sub\" HEAD \"../$test_count-sub\"\n+'\n+\n #\n # Tests for 'git subtree pull'\n #\n@@ -989,6 +995,12 @@ test_expect_success 'push \"sub dir\"/ with a local rev' '\n \t)\n '\n \n+test_expect_success 'push after annotated tag was added/merged with --squash pre-v2.32.0' '\n+\ttest_create_pre2_32_repo \"$test_count\" &&\n+\ttest_create_commit \"$test_count-clone\" sub/main-sub1 &&\n+\tgit -C \"$test_count-clone\" subtree push --prefix=\"sub\" \"../$test_count-sub\" from-mainline\n+'\n+\n #\n # Validity checking\n #\n-- \ngitgitgadget\n"},{"id":"465476","messageId":"db97bc1dbe004989d1c0d456167d1a160e85db01.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 8/9] subtree: fix squash merging after annotated tag was squashed merged","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:38Z","receivedAt":"2022-10-21T15:14:32Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nWhen 'git subtree merge --squash $ref' is invoked, either directly or\nthrough 'git subtree pull --squash $repo $ref', the code looks for the\nlatest squash merge of the subtree in order to create the new merge\ncommit as a child of the previous squash merge.\n\nThis search is done in function 'process_subtree_split_trailer', invoked\nby 'find_latest_squash', which looks for the most recent commit with a\n'git-subtree-split' trailer; that trailer's value is the object name in\nthe subtree repository of the ref that was last squash-merged. The\nfunction verifies that this object is present locally with 'git\nrev-parse', and aborts if it's not.\n\nThe hash referenced by the 'git-subtree-split' trailer is guaranteed to\ncorrespond to a commit since it is the result of running 'git rev-parse\n-q --verify \"$1^{commit}\"' on the first argument of 'cmd_merge' (this\ncorresponds to 'rev' in 'cmd_merge' which is passed through to\n'new_squash_commit' and 'squash_msg').\n\nBut this is only the case since e4f8baa88a (subtree: parse revs in\nindividual cmd_ functions, 2021-04-27), which went into Git 2.32. Before\nthat commit, 'cmd_merge' verified the revision it was given using 'git\nrev-parse --revs-only \"$@\"'. Such an invocation, when fed the name of an\nannotated tag, would return the hash of the tag, not of the commit\nreferenced by the tag.\n\nThis leads to a failure in 'find_latest_squash' when squash-merging if\nthe most recent squash-merge merged an annotated tag of the subtree\nrepository, using a pre-2.32 version of 'git subtree', unless that\nprevious annotated tag is present locally (which is not usually the\ncase).\n\nWe can fix this by fetching the object directly by its hash in\n'process_subtree_split_trailer' when 'git rev-parse' fails, but in order\nto do so we need to know the name or URL of the subtree repository.\nThis is not possible in general for 'git subtree merge', but is easy\nwhen it is invoked through 'git subtree pull' since in that case the\nsubtree repository is passed by the user at the command line.\n\nAllow the 'git subtree pull' scenario to work out-of-the-box by adding\nan optional 'repository' argument to functions 'cmd_merge',\n'find_latest_squash' and 'process_subtree_split_trailer', and invoke\n'cmd_merge' with that 'repository' argument in 'cmd_pull'.\n\nIf 'repository' is absent in 'process_subtree_split_trailer', instruct\nthe user to try fetching the missing object directly.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh     | 56 +++++++++++++++++++++++-------\n contrib/subtree/git-subtree.txt    |  9 +++--\n contrib/subtree/t/t7900-subtree.sh | 36 +++++++++++++++++++\n 3 files changed, 86 insertions(+), 15 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex b90ca0036f7..2c67989fe8a 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -371,20 +371,45 @@ try_remove_previous () {\n \tfi\n }\n \n-# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH\n+# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n process_subtree_split_trailer () {\n-\tassert test $# = 2\n+\tassert test $# = 2 -o $# = 3\n \tb=\"$1\"\n \tsq=\"$2\"\n-\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n-\tdie \"fatal: could not rev-parse split hash $b from commit $sq\"\n+\trepository=\"\"\n+\tif test \"$#\" = 3\n+\tthen\n+\t\trepository=\"$3\"\n+\tfi\n+\tfail_msg=\"fatal: could not rev-parse split hash $b from commit $sq\"\n+\tif ! sub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\"\n+\tthen\n+\t\t# if 'repository' was given, try to fetch the 'git-subtree-split' hash\n+\t\t# before 'rev-parse'-ing it again, as it might be a tag that we do not have locally\n+\t\tif test -n \"${repository}\"\n+\t\tthen\n+\t\t\tgit fetch \"$repository\" \"$b\"\n+\t\t\tsub=\"$(git rev-parse --verify --quiet \"$b^{commit}\")\" ||\n+\t\t\t\tdie \"$fail_msg\"\n+\t\telse\n+\t\t\thint1=$(printf \"hint: hash might be a tag, try fetching it from the subtree repository:\")\n+\t\t\thint2=$(printf \"hint:    git fetch <subtree-repository> $b\")\n+\t\t\tfail_msg=$(printf \"$fail_msg\\n$hint1\\n$hint2\")\n+\t\t\tdie \"$fail_msg\"\n+\t\tfi\n+\tfi\n }\n \n-# Usage: find_latest_squash DIR\n+# Usage: find_latest_squash DIR [REPOSITORY]\n find_latest_squash () {\n-\tassert test $# = 1\n+\tassert test $# = 1 -o $# = 2\n \tdir=\"$1\"\n-\tdebug \"Looking for latest squash ($dir)...\"\n+\trepository=\"\"\n+\tif test \"$#\" = 2\n+\tthen\n+\t\trepository=\"$2\"\n+\tfi\n+\tdebug \"Looking for latest squash (dir=$dir, repository=$repository)...\"\n \tlocal indent=$(($indent + 1))\n \n \tsq=\n@@ -404,7 +429,7 @@ find_latest_squash () {\n \t\t\tmain=\"$b\"\n \t\t\t;;\n \t\tgit-subtree-split:)\n-\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\"\n+\t\t\tprocess_subtree_split_trailer \"$b\" \"$sq\" \"$repository\"\n \t\t\t;;\n \t\tEND)\n \t\t\tif test -n \"$sub\"\n@@ -969,17 +994,22 @@ cmd_split () {\n \texit 0\n }\n \n-# Usage: cmd_merge REV\n+# Usage: cmd_merge REV [REPOSITORY]\n cmd_merge () {\n-\ttest $# -eq 1 ||\n-\t\tdie \"fatal: you must provide exactly one revision.  Got: '$*'\"\n+\ttest $# -eq 1 -o $# -eq 2 ||\n+\t\tdie \"fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'\"\n \trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\tdie \"fatal: '$1' does not refer to a commit\"\n+\trepository=\"\"\n+\tif test \"$#\" = 2\n+\tthen\n+\t\trepository=\"$2\"\n+\tfi\n \tensure_clean\n \n \tif test -n \"$arg_addmerge_squash\"\n \tthen\n-\t\tfirst_split=\"$(find_latest_squash \"$dir\")\" || exit $?\n+\t\tfirst_split=\"$(find_latest_squash \"$dir\" \"$repository\")\" || exit $?\n \t\tif test -z \"$first_split\"\n \t\tthen\n \t\t\tdie \"fatal: can't squash-merge: '$dir' was never added.\"\n@@ -1017,7 +1047,7 @@ cmd_pull () {\n \tensure_clean\n \tensure_valid_ref_format \"$ref\"\n \tgit fetch \"$repository\" \"$ref\" || exit $?\n-\tcmd_merge FETCH_HEAD\n+\tcmd_merge FETCH_HEAD \"$repository\"\n }\n \n # Usage: cmd_push REPOSITORY [+][LOCALREV:]REMOTEREF\ndiff --git a/contrib/subtree/git-subtree.txt b/contrib/subtree/git-subtree.txt\nindex 9cddfa26540..0e7524d7864 100644\n--- a/contrib/subtree/git-subtree.txt\n+++ b/contrib/subtree/git-subtree.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git subtree' [<options>] -P <prefix> add <local-commit>\n 'git subtree' [<options>] -P <prefix> add <repository> <remote-ref>\n-'git subtree' [<options>] -P <prefix> merge <local-commit>\n+'git subtree' [<options>] -P <prefix> merge <local-commit> [<repository>]\n 'git subtree' [<options>] -P <prefix> split [<local-commit>]\n \n [verse]\n@@ -76,7 +76,7 @@ add <repository> <remote-ref>::\n \tonly a single commit from the subproject, rather than its\n \tentire history.\n \n-merge <local-commit>::\n+merge <local-commit> [<repository>]::\n \tMerge recent changes up to <local-commit> into the <prefix>\n \tsubtree.  As with normal 'git merge', this doesn't\n \tremove your own local changes; it just merges those\n@@ -88,6 +88,11 @@ If you use '--squash', the merge direction doesn't always have to be\n forward; you can use this command to go back in time from v2.5 to v2.4,\n for example.  If your merge introduces a conflict, you can resolve it in\n the usual ways.\n++\n+When using '--squash', and the previous merge with '--squash' merged an\n+annotated tag of the subtree repository, that tag needs to be available locally.\n+If <repository> is given, a missing tag will automatically be fetched from that\n+repository.\n \n split [<local-commit>]::\n \tExtract a new, synthetic project history from the\ndiff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh\nindex 249743ab9aa..d0671676c7a 100755\n--- a/contrib/subtree/t/t7900-subtree.sh\n+++ b/contrib/subtree/t/t7900-subtree.sh\n@@ -43,6 +43,30 @@ last_commit_subject () {\n \tgit log --pretty=format:%s -1\n }\n \n+# Upon 'git subtree add|merge --squash' of an annotated tag,\n+# pre-2.32.0 versions of 'git subtree' would write the hash of the tag\n+# (sub1 below), instead of the commit (sub1^{commit}) in the\n+# \"git-subtree-split\" trailer.\n+# We immitate this behaviour below using a replace ref.\n+# This function creates 3 repositories:\n+# - $1\n+# - $1-sub (added as subtree \"sub\" in $1)\n+# - $1-clone (clone of $1)\n+test_create_pre2_32_repo () {\n+\tsubtree_test_create_repo \"$1\" &&\n+\tsubtree_test_create_repo \"$1-sub\" &&\n+\ttest_commit -C \"$1\" main1 &&\n+\ttest_commit -C \"$1-sub\" --annotate sub1 &&\n+\tgit -C \"$1\" subtree add --prefix=\"sub\" --squash \"../$1-sub\" sub1 &&\n+\ttag=$(git -C \"$1\" rev-parse FETCH_HEAD) &&\n+\tcommit=$(git -C \"$1\" rev-parse FETCH_HEAD^{commit}) &&\n+\tgit -C \"$1\" log -1 --format=%B HEAD^2 >msg &&\n+\ttest_commit -C \"$1-sub\" --annotate sub2 &&\n+\tgit clone --no-local \"$1\" \"$1-clone\" &&\n+\tnew_commit=$(cat msg | sed -e \"s/$commit/$tag/\" | git -C \"$1-clone\" commit-tree HEAD^2^{tree}) &&\n+\tgit -C \"$1-clone\" replace HEAD^2 $new_commit\n+}\n+\n test_expect_success 'shows short help text for -h' '\n \ttest_expect_code 129 git subtree -h >out 2>err &&\n \ttest_must_be_empty err &&\n@@ -264,6 +288,13 @@ test_expect_success 'merge new subproj history into subdir/ with a slash appende\n \t)\n '\n \n+test_expect_success 'merge with --squash after annotated tag was added/merged with --squash pre-v2.32.0 ' '\n+\ttest_create_pre2_32_repo \"$test_count\" &&\n+\tgit -C \"$test_count-clone\" fetch \"../$test_count-sub\" sub2  &&\n+\ttest_must_fail git -C \"$test_count-clone\" subtree merge --prefix=\"sub\" --squash FETCH_HEAD &&\n+\tgit -C \"$test_count-clone\" subtree merge --prefix=\"sub\" --squash FETCH_HEAD  \"../$test_count-sub\"\n+'\n+\n #\n # Tests for 'git subtree split'\n #\n@@ -630,6 +661,11 @@ test_expect_success 'pull rejects flags for split' '\n \t)\n '\n \n+test_expect_success 'pull with --squash after annotated tag was added/merged with --squash pre-v2.32.0 ' '\n+\ttest_create_pre2_32_repo \"$test_count\" &&\n+\tgit -C \"$test_count-clone\" subtree -d pull --prefix=\"sub\" --squash \"../$test_count-sub\" sub2\n+'\n+\n #\n # Tests for 'git subtree push'\n #\n-- \ngitgitgadget\n\n"},{"id":"465477","messageId":"429b3656f503785b60297ace4f83fcceb7363a4c.1666365220.git.gitgitgadget@gmail.com","threadId":"58669","inReplyTo":"pull.1390.git.1666365219.gitgitgadget@gmail.com","subject":"[PATCH 6/9] subtree: use named variables instead of \"$@\" in cmd_pull","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T15:13:36Z","receivedAt":"2022-10-21T15:15:04Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\n'cmd_pull' already checks that only two arguments are given,\n'repository' and 'ref'. Define variables with these names instead of\nusing the positional parameter $2 and \"$@\".\n\nThis will allow a subsequent commit to pass 'repository' to 'cmd_merge'.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n contrib/subtree/git-subtree.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex d91a9679075..2b3c429991b 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -1005,9 +1005,11 @@ cmd_pull () {\n \tthen\n \t\tdie \"fatal: you must provide <repository> <ref>\"\n \tfi\n+\trepository=\"$1\"\n+\tref=\"$2\"\n \tensure_clean\n-\tensure_valid_ref_format \"$2\"\n-\tgit fetch \"$@\" || exit $?\n+\tensure_valid_ref_format \"$ref\"\n+\tgit fetch \"$repository\" \"$ref\" || exit $?\n \tcmd_merge FETCH_HEAD\n }\n \n-- \ngitgitgadget\n\n"},{"id":"465482","messageId":"221021.86v8oddtuj.gmgdl@evledraar.gmail.com","threadId":"58669","inReplyTo":"c890f55f59994231be6114f76f020510eb824453.1666365220.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-21T16:22:39Z","receivedAt":"2022-10-21T16:28:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)\n> added validation code to check that options given to 'git subtree <cmd>'\n> made sense with the command being used.\n>\n> Refactor these checks by adding a 'die_incompatible_opt' function to\n> reduce code duplication.\n\nThis looks good, but...\n\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------\n>  1 file changed, 20 insertions(+), 12 deletions(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 49ef493ef92..f5eab198c80 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -102,6 +102,14 @@ assert () {\n>  \tfi\n>  }\n>  \n> +# Usage: die_incompatible_opt OPTION COMMAND\n> +die_incompatible_opt () {\n> +\tassert test \"$#\" = 2\n> +\topt=\"$1\"\n> +\targ_command=\"$2\"\n> +\tdie \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n> +}\n> +\n>  main () {\n>  \tif test $# -eq 0\n>  \tthen\n> @@ -176,16 +184,16 @@ main () {\n>  \t\t\targ_debug=1\n>  \t\t\t;;\n>  \t\t--annotate)\n> -\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n> +\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n>  \t\t\targ_split_annotate=\"$1\"\n>  \t\t\tshift\n>  \t\t\t;;\n\nSince this is all in this form I wonder why not (maybe adding some \"local\" and/or \"&&\" too while at it):\n\n\tdie_if_other_opt {\n\t\tassert test \"$#\" = 3\n\t\tother=\"$1\"\n                shift\n                if test -z \"$other\"\n\t\tthen\n\t\t\treturn\n\t\tfi\n\t\t[...the rest of your version]\n\t}\n\nThen:\n\n>  \t\t--no-annotate)\n> -\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n> +\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n\nInstead of this:\n\n\tdie_if_other_opt \"$allow_split\" \"$opt\" \"$arg_command\"\n\nOr actually just:\n\n\tdie_if_other_opt \"$allow_split\"\n\nMaybe you disagree, but since the function will see the variables & this\nis purely a helper for this getopts parse loop I think it's fine just to\nassume we can read \"$opt\" and \"$arg_command\" there..., so, urm, maybe just:\n\n\ttest -n \"$allow_split\" || die_incompatible_opt\n\n? :) Anyway, an easy bike shed, you should go for whatever variant you\nprefer :) Thanks for indulging me.\n"},{"id":"465483","messageId":"221021.86r0z1dtjq.gmgdl@evledraar.gmail.com","threadId":"58669","inReplyTo":"a70fda5582d6bd84b8bedaba33768d3886846090.1666365220.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/9] subtree: prefix die messages with 'fatal'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-21T16:30:44Z","receivedAt":"2022-10-21T16:34:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Just as was done in 0008d12284 (submodule: prefix die messages with\n> 'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages\n> outputed by 'git-subtree.sh' more in line with the rest of the code base\n> by prefixing them with \"fatal: \", and do not capitalize their first\n> letter.\n\nI don't really care since we're unlikely to ever give git-subtree the\ni18n treatment, so translators don't need to worry about the churn.\n\nBut given how few in-tree-users we have of \"die\" and \"git-sh-setup\" this\nwould be much shorter & future-proof as just e.g. (untested):\n\t\n\tdiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n\tindex 7562a395c24..0d8f87c5a20 100755\n\t--- a/contrib/subtree/git-subtree.sh\n\t+++ b/contrib/subtree/git-subtree.sh\n\t@@ -25,6 +25,8 @@ then\n\t \texit 126\n\t fi\n\t \n\t+GIT_SH_SETUP_DIE_PREFIX='fatal: '\n\t+\n\t OPTS_SPEC=\"\\\n\t git subtree add   --prefix=<prefix> <commit>\n\t git subtree add   --prefix=<prefix> <repository> <ref>\n\tdiff --git a/git-sh-setup.sh b/git-sh-setup.sh\n\tindex ce273fe0e48..81456d7266e 100644\n\t--- a/git-sh-setup.sh\n\t+++ b/git-sh-setup.sh\n\t@@ -53,7 +53,7 @@ die () {\n\t die_with_status () {\n\t \tstatus=$1\n\t \tshift\n\t-\tprintf >&2 '%s\\n' \"$*\"\n\t+\tprintf >&2 '%s%s\\n' \"$GIT_SH_SETUP_DIE_PREFIX\" \"$*\"\n\t \texit \"$status\"\n\t }\n\t \n> -\t\tdie \"assertion failed: $*\"\n> +\t\tdie \"fatal: assertion failed: $*\"\n\nThen you could just leave this, but...\n\n> -\t\tdie \"Unknown command '$arg_command'\"\n> +\t\tdie \"fatal: unknown command '$arg_command'\"\n\n...would still need to change these for the capitalization change.\n"},{"id":"465484","messageId":"221021.86mt9pdtcw.gmgdl@evledraar.gmail.com","threadId":"58669","inReplyTo":"86a842d50345f6d4d0b16c78d565474be6f8068a.1666365220.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-21T16:37:34Z","receivedAt":"2022-10-21T16:42:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> The previous commit fixed a failure in 'git subtree merge --squash' when\n> the previous squash-merge merged an annotated tag of the subtree\n> repository which is missing locally.\n>\n> The same failure happens in 'git subtree split', either directly or when\n> called by 'git subtree push', under the same circumstances: 'cmd_split'\n> invokes 'find_existing_splits', which loops through previous commits and\n> invokes 'git rev-parse' (via 'process_subtree_split_trailer') on the\n> value of any 'git subtree-split' trailer it finds. This fails if this\n> value is the hash of an annotated tag which is missing locally.\n>\n> Add a new optional argument 'repository' to 'cmd_split' and\n> 'find_existing_splits', and invoke 'cmd_split' with that argument from\n> 'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch\n> the missing tag from the 'repository' if it's not available locally,\n> mirroring the new behaviour of 'git subtree pull' and 'git subtree\n> merge'.\n>\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------\n>  contrib/subtree/git-subtree.txt    |  7 ++++++-\n>  contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++\n>  3 files changed, 36 insertions(+), 9 deletions(-)\n>\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index 2c67989fe8a..10c9c87839a 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -453,14 +453,19 @@ find_latest_squash () {\n>  \tdone || exit $?\n>  }\n>  \n> -# Usage: find_existing_splits DIR REV\n> +# Usage: find_existing_splits DIR REV [REPOSITORY]\n>  find_existing_splits () {\n> -\tassert test $# = 2\n> +\tassert test $# = 2 -o $# = 3\n\nThis \"test\" syntax is considered unportable, I'm too lazy to dig up the\nreference, but we've removed it in the past. Maybe it's OK with\ngit-subtree.sh\", but anyway, it's esay enough to change...\n\n...but looking at \"master\" I see one instance of it in git-subtree.sh\nalready, so maybe nobody cares...\n"},{"id":"465502","messageId":"CAPig+cRszqyrPFHSaB92_kiQ60huXw2rWZbCh+JsGKetnChJUQ@mail.gmail.com","threadId":"58669","inReplyTo":"221021.86mt9pdtcw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-21T18:24:07Z","receivedAt":"2022-10-21T18:24:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 21, 2022 at 12:48 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n> > +# Usage: find_existing_splits DIR REV [REPOSITORY]\n> >  find_existing_splits () {\n> > +     assert test $# = 2 -o $# = 3\n>\n> This \"test\" syntax is considered unportable, I'm too lazy to dig up the\n> reference, but we've removed it in the past. Maybe it's OK with\n> git-subtree.sh\", but anyway, it's esay enough to change...\n\nYou may be referring to POSIX[1] long considering -o/-a to be\nobsolescent, and GNU warning[2] against them for a couple decades or\nmore.\n\n[1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16\n[2]: https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Limitations-of-Builtins.html\n"},{"id":"465509","messageId":"xmqq8rl8kht3.fsf@gitster.g","threadId":"58669","inReplyTo":"832e910aadc47309392feed69927c2e8693cbfc5.1666365220.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T21:06:32Z","receivedAt":"2022-10-21T21:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Some variables in 'test_commit' have names that are common enough that\n> it is very likely that test authors might use them in a test. If they do\n> so and use 'test_commit' between setting such a variable and using it,\n> the variable value from 'test_commit' will leak back into the test and\n> most likely break it.\n>\n> Prevent that by marking all variables in 'test_commit' as 'local'. This\n> allow a subsequent commit to use a 'tag' variable.\n\nThis is the right thing to do, if done onn day 1 of the project, and\nit is the right thing to do for the longer term health of the\nproject.  But it is a bit scary thing to do in the middle.\n\nI wonder if there is an easy way to detect that a set of callers of\ntest_commit are relying on the fact that calling test_commit without\npassing --author option cleared their $author variable (similarly\nfor other variables that are cleared or set to a known value as a\nside effect of calling test_commit).  If their correctness depends\non $author becoming empty after calling the script today, they will\nget broken by this change.\n\nWhile it is OK to argue that they deserve it, we would have to be\nfinding and fixing them ourselves after all.\n\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  t/test-lib-functions.sh | 16 ++++++++--------\n>  1 file changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 527a7145000..adc0fb6330c 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -273,13 +273,13 @@ debug () {\n>  # <file>, <contents>, and <tag> all default to <message>.\n>  \n>  test_commit () {\n> -\tnotick= &&\n> -\techo=echo &&\n> -\tappend= &&\n> -\tauthor= &&\n> -\tsignoff= &&\n> -\tindir= &&\n> -\ttag=light &&\n> +\tlocal notick= &&\n> +\tlocal echo=echo &&\n> +\tlocal append= &&\n> +\tlocal author= &&\n> +\tlocal signoff= &&\n> +\tlocal indir= &&\n> +\tlocal tag=light &&\n>  \twhile test $# != 0\n>  \tdo\n>  \t\tcase \"$1\" in\n> @@ -322,7 +322,7 @@ test_commit () {\n>  \t\tshift\n>  \tdone &&\n>  \tindir=${indir:+\"$indir\"/} &&\n> -\tfile=${2:-\"$1.t\"} &&\n> +\tlocal file=${2:-\"$1.t\"} &&\n>  \tif test -n \"$append\"\n>  \tthen\n>  \t\t$echo \"${3-$1}\" >>\"$indir$file\"\n"},{"id":"465834","messageId":"3a41e02f-c1fb-64ed-377b-4e4168f2adae@gmail.com","threadId":"58669","inReplyTo":"xmqq8rl8kht3.fsf@gitster.g","subject":"Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-10-26T21:21:31Z","receivedAt":"2022-10-26T21:21:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2022-10-21 à 17:06, Junio C Hamano a écrit :\n> \"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> Some variables in 'test_commit' have names that are common enough that\n>> it is very likely that test authors might use them in a test. If they do\n>> so and use 'test_commit' between setting such a variable and using it,\n>> the variable value from 'test_commit' will leak back into the test and\n>> most likely break it.\n>>\n>> Prevent that by marking all variables in 'test_commit' as 'local'. This\n>> allow a subsequent commit to use a 'tag' variable.\n> \n> This is the right thing to do, if done onn day 1 of the project, and\n> it is the right thing to do for the longer term health of the\n> project.  But it is a bit scary thing to do in the middle.\n> \n> I wonder if there is an easy way to detect that a set of callers of\n> test_commit are relying on the fact that calling test_commit without\n> passing --author option cleared their $author variable (similarly\n> for other variables that are cleared or set to a known value as a\n> side effect of calling test_commit).  If their correctness depends\n> on $author becoming empty after calling the script today, they will\n> get broken by this change.\n> \n> While it is OK to argue that they deserve it, we would have to be\n> finding and fixing them ourselves after all.\n\nIsn't the fact that the test suite passes with this change enough for us\nto be confident that nothing is broken by it ?\n\nIn any case, I did a quick manual grep of each of the variables and could not\nfind a test that uses these names, so I think we are safe.\n\nThanks,\nPhilippe.\n"},{"id":"465835","messageId":"949edc06-8252-6995-addf-cfba398f73dd@gmail.com","threadId":"58669","inReplyTo":"221021.86v8oddtuj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/9] subtree: add 'die_incompatible_opt' function to reduce duplication","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-10-26T21:23:17Z","receivedAt":"2022-10-26T21:23:24Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Ævar,\n\nLe 2022-10-21 à 12:22, Ævar Arnfjörð Bjarmason a écrit :\n> \n> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> 9a3e3ca2ba (subtree: be stricter about validating flags, 2021-04-27)\n>> added validation code to check that options given to 'git subtree <cmd>'\n>> made sense with the command being used.\n>>\n>> Refactor these checks by adding a 'die_incompatible_opt' function to\n>> reduce code duplication.\n> \n> This looks good, but...\n> \n>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n>> ---\n>>  contrib/subtree/git-subtree.sh | 32 ++++++++++++++++++++------------\n>>  1 file changed, 20 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index 49ef493ef92..f5eab198c80 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -102,6 +102,14 @@ assert () {\n>>  \tfi\n>>  }\n>>  \n>> +# Usage: die_incompatible_opt OPTION COMMAND\n>> +die_incompatible_opt () {\n>> +\tassert test \"$#\" = 2\n>> +\topt=\"$1\"\n>> +\targ_command=\"$2\"\n>> +\tdie \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n>> +}\n>> +\n>>  main () {\n>>  \tif test $# -eq 0\n>>  \tthen\n>> @@ -176,16 +184,16 @@ main () {\n>>  \t\t\targ_debug=1\n>>  \t\t\t;;\n>>  \t\t--annotate)\n>> -\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n>> +\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n>>  \t\t\targ_split_annotate=\"$1\"\n>>  \t\t\tshift\n>>  \t\t\t;;\n> \n> Since this is all in this form I wonder why not (maybe adding some \"local\" and/or \"&&\" too while at it):\n> \n> \tdie_if_other_opt {\n> \t\tassert test \"$#\" = 3\n> \t\tother=\"$1\"\n>                 shift\n>                 if test -z \"$other\"\n> \t\tthen\n> \t\t\treturn\n> \t\tfi\n> \t\t[...the rest of your version]\n> \t}\n> \n> Then:\n> \n>>  \t\t--no-annotate)\n>> -\t\t\ttest -n \"$allow_split\" || die \"The '$opt' flag does not make sense with 'git subtree $arg_command'.\"\n>> +\t\t\ttest -n \"$allow_split\" || die_incompatible_opt \"$opt\" \"$arg_command\"\n> \n> Instead of this:\n> \n> \tdie_if_other_opt \"$allow_split\" \"$opt\" \"$arg_command\"\n> \n> Or actually just:\n> \n> \tdie_if_other_opt \"$allow_split\"\n> \n> Maybe you disagree, but since the function will see the variables & this\n> is purely a helper for this getopts parse loop I think it's fine just to\n> assume we can read \"$opt\" and \"$arg_command\" there..., so, urm, maybe just:\n> \n> \ttest -n \"$allow_split\" || die_incompatible_opt\n> \n> ? :) Anyway, an easy bike shed, you should go for whatever variant you\n> prefer :) Thanks for indulging me.\n> \n\nThanks for your suggestion, but I think I'll stick to what I have for now :)\n\nPhilippe.\n"},{"id":"465836","messageId":"3f8e9f2f-aa98-f152-46d3-21f69dc2d3bb@gmail.com","threadId":"58669","inReplyTo":"221021.86r0z1dtjq.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/9] subtree: prefix die messages with 'fatal'","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-10-26T21:24:59Z","receivedAt":"2022-10-26T21:25:06Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2022-10-21 à 12:30, Ævar Arnfjörð Bjarmason a écrit :\n> \n> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> Just as was done in 0008d12284 (submodule: prefix die messages with\n>> 'fatal', 2021-07-10) for 'git-submodule.sh', make the 'die' messages\n>> outputed by 'git-subtree.sh' more in line with the rest of the code base\n>> by prefixing them with \"fatal: \", and do not capitalize their first\n>> letter.\n> \n> I don't really care since we're unlikely to ever give git-subtree the\n> i18n treatment, so translators don't need to worry about the churn.\n> \n> But given how few in-tree-users we have of \"die\" and \"git-sh-setup\" this\n> would be much shorter & future-proof as just e.g. (untested):\n> \t\n> \tdiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> \tindex 7562a395c24..0d8f87c5a20 100755\n> \t--- a/contrib/subtree/git-subtree.sh\n> \t+++ b/contrib/subtree/git-subtree.sh\n> \t@@ -25,6 +25,8 @@ then\n> \t \texit 126\n> \t fi\n> \t \n> \t+GIT_SH_SETUP_DIE_PREFIX='fatal: '\n> \t+\n> \t OPTS_SPEC=\"\\\n> \t git subtree add   --prefix=<prefix> <commit>\n> \t git subtree add   --prefix=<prefix> <repository> <ref>\n> \tdiff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> \tindex ce273fe0e48..81456d7266e 100644\n> \t--- a/git-sh-setup.sh\n> \t+++ b/git-sh-setup.sh\n> \t@@ -53,7 +53,7 @@ die () {\n> \t die_with_status () {\n> \t \tstatus=$1\n> \t \tshift\n> \t-\tprintf >&2 '%s\\n' \"$*\"\n> \t+\tprintf >&2 '%s%s\\n' \"$GIT_SH_SETUP_DIE_PREFIX\" \"$*\"\n> \t \texit \"$status\"\n> \t }\n> \t \n>> -\t\tdie \"assertion failed: $*\"\n>> +\t\tdie \"fatal: assertion failed: $*\"\n> \n> Then you could just leave this, but...\n> \n>> -\t\tdie \"Unknown command '$arg_command'\"\n>> +\t\tdie \"fatal: unknown command '$arg_command'\"\n> \n> ...would still need to change these for the capitalization change.\n>\n\nYeah, I guess to this could be done in a later refactor if someone wishes.\n\nIt would probably require changes to other tests scripts (since there are other users\nof 'die'), though, and so would widen the scope of the series a bit too much, I would say. \n"},{"id":"465837","messageId":"b83b8c41-07c9-ba5a-93a0-ed0ae4c9d3cd@gmail.com","threadId":"58669","inReplyTo":"221021.86mt9pdtcw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 9/9] subtree: fix split after annotated tag was squashed merged","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-10-26T21:26:55Z","receivedAt":"2022-10-26T21:27:00Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2022-10-21 à 12:37, Ævar Arnfjörð Bjarmason a écrit :\n> \n> On Fri, Oct 21 2022, Philippe Blain via GitGitGadget wrote:\n> \n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>> The previous commit fixed a failure in 'git subtree merge --squash' when\n>> the previous squash-merge merged an annotated tag of the subtree\n>> repository which is missing locally.\n>>\n>> The same failure happens in 'git subtree split', either directly or when\n>> called by 'git subtree push', under the same circumstances: 'cmd_split'\n>> invokes 'find_existing_splits', which loops through previous commits and\n>> invokes 'git rev-parse' (via 'process_subtree_split_trailer') on the\n>> value of any 'git subtree-split' trailer it finds. This fails if this\n>> value is the hash of an annotated tag which is missing locally.\n>>\n>> Add a new optional argument 'repository' to 'cmd_split' and\n>> 'find_existing_splits', and invoke 'cmd_split' with that argument from\n>> 'cmd_push'. This allows 'process_subtree_split_trailer' to try to fetch\n>> the missing tag from the 'repository' if it's not available locally,\n>> mirroring the new behaviour of 'git subtree pull' and 'git subtree\n>> merge'.\n>>\n>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n>> ---\n>>  contrib/subtree/git-subtree.sh     | 26 ++++++++++++++++++--------\n>>  contrib/subtree/git-subtree.txt    |  7 ++++++-\n>>  contrib/subtree/t/t7900-subtree.sh | 12 ++++++++++++\n>>  3 files changed, 36 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n>> index 2c67989fe8a..10c9c87839a 100755\n>> --- a/contrib/subtree/git-subtree.sh\n>> +++ b/contrib/subtree/git-subtree.sh\n>> @@ -453,14 +453,19 @@ find_latest_squash () {\n>>  \tdone || exit $?\n>>  }\n>>  \n>> -# Usage: find_existing_splits DIR REV\n>> +# Usage: find_existing_splits DIR REV [REPOSITORY]\n>>  find_existing_splits () {\n>> -\tassert test $# = 2\n>> +\tassert test $# = 2 -o $# = 3\n> \n> This \"test\" syntax is considered unportable, I'm too lazy to dig up the\n> reference, but we've removed it in the past. Maybe it's OK with\n> git-subtree.sh\", but anyway, it's esay enough to change...\n> \n> ...but looking at \"master\" I see one instance of it in git-subtree.sh\n> already, so maybe nobody cares...\n> \n\nI did not know it was not portable, in fact I used this form because while\nreading the rest of the script I found that was the style used already. \n\nSo I guess I would leave this as a further cleanup for someone wishing to make\n'git subtree' more POSIX...\n"},{"id":"465842","messageId":"xmqqbkpyfeor.fsf@gitster.g","threadId":"58669","inReplyTo":"3a41e02f-c1fb-64ed-377b-4e4168f2adae@gmail.com","subject":"Re: [PATCH 1/9] test-lib-functions: mark 'test_commit' variables as 'local'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-26T21:38:44Z","receivedAt":"2022-10-26T21:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n>> While it is OK to argue that they deserve it, we would have to be\n>> finding and fixing them ourselves after all.\n>\n> Isn't the fact that the test suite passes with this change enough for us\n> to be confident that nothing is broken by it ?\n\nI am not brave enough to claim that, actually.\n\nBut even if the tests were subtly changing their behaviour and no\nlonger testing what they originally meant to test, I think that is\nOK.  It will not be the first time for us to later find out that\nsome tests were passing for a wrong reason, or some that expect\nfailure were failing for a wrong reason.  We had multiple such cases\nwe later found and fixed, so even if these \"local\" introduced changes\nin behaviour to some tests, I am OK ;-).\n\n\n"}]}