{"thread":{"id":"56463","subject":"[PATCH 00/11] rebase: dereference tags","startedAt":"2021-09-08T09:49:27Z","lastAt":"2021-09-24T01:27:11Z","messageCount":55,"participants":["Phillip Wood via GitGitGadget","Ævar Arnfjörð Bjarmason","Phillip Wood","Johannes Schindelin","René Scharfe","Junio C Hamano","Elijah Newren","Sergey Organov"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"435047","messageId":"pull.1033.git.1631094563.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":null,"subject":"[PATCH 00/11] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:11Z","receivedAt":"2021-09-08T09:49:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Aborting a rebase stated with git rebase should\ncheckout the commit pointed to by . Instead it gives\n\n error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n\n\nThe fix for that is in the last patch, the rest of the patches are cleanups\nto t3407 and builtin/rebase.c\n\nPhillip Wood (11):\n t3407: run tests in $TEST_DIRECTORY\n t3407: use test_commit\n t3407: use test_cmp_rev\n t3407: rename a variable\n t3407: use test_path_is_missing\n t3407: strengthen rebase --abort tests\n t3407: rework rebase --quit tests\n rebase: remove redundant strbuf\n rebase: use our standard error return value\n rebase: use lookup_commit_reference_by_name()\n rebase: dereference tags\n\n builtin/rebase.c | 67 ++++++++++++-------------\n t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------\n 2 files changed, 76 insertions(+), 96 deletions(-)\n\n\nbase-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1033\n-- \ngitgitgadget\n"},{"id":"435048","messageId":"0f5992e8caba424e484524252a303a00e108df34.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 01/11] t3407: run tests in $TEST_DIRECTORY","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:12Z","receivedAt":"2021-09-08T09:49:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCommit 97b88dd58c (\"git-rebase.sh: Fix --merge --abort failures when\npath contains whitespace\", 2008-05-04) started running these tests in\na subdirectory with a space in its name. At that time $TEST_DIRECTORY\ndid not contain a space but shortly after in 4a7aaccd83 (\"Rename the\ntest trash directory to contain spaces.\", 2008-05-04) $TEST_DIRECTORY\nwas changed to contain a space so we no longer need to run these tests\nin a subdirectory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 7c381fbc89a..c747bd31d76 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n-### Test that we handle space characters properly\n-work_dir=\"$(pwd)/test dir\"\n-\n test_expect_success setup '\n-\tmkdir -p \"$work_dir\" &&\n-\tcd \"$work_dir\" &&\n-\tgit init &&\n \techo a > a &&\n \tgit add a &&\n \tgit commit -m a &&\n@@ -37,7 +31,6 @@ testrebase() {\n \tdotest=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -48,7 +41,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -61,7 +53,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -77,7 +68,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\tgit reflog show to-rebase > reflog_before &&\n@@ -89,7 +79,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success 'rebase --abort can not be used with other options' '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -103,7 +92,6 @@ testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n test_expect_success 'rebase --apply --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --apply main &&\n@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '\n '\n \n test_expect_success 'rebase --merge --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --merge main &&\n-- \ngitgitgadget\n\n"},{"id":"435049","messageId":"79b6dec910ecc9ceb61d03e3bbd9919ce457d78e.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 02/11] t3407: use test_commit","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:13Z","receivedAt":"2021-09-08T09:49:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSimplify the setup by using test_commit. Note that this replaces the\nbranch \"pre-rebase\" with a tag of the same name. \"pre-rebase\" is only\nused as a fixed reference point so it makes sense to use a tag rather\nthan a branch.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex c747bd31d76..0ad21966bc5 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n test_expect_success setup '\n-\techo a > a &&\n-\tgit add a &&\n-\tgit commit -m a &&\n+\ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\techo b > a &&\n-\tgit commit -a -m b &&\n-\techo c > a &&\n-\tgit commit -a -m c &&\n+\ttest_commit b a b &&\n+\ttest_commit c a c &&\n \n \tgit checkout to-rebase &&\n-\techo d > a &&\n-\tgit commit -a -m \"merge should fail on this\" &&\n-\techo e > a &&\n-\tgit commit -a -m \"merge should fail on this, too\" &&\n-\tgit branch pre-rebase\n+\ttest_commit \"merge should fail on this\" a d d &&\n+\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n testrebase() {\n-- \ngitgitgadget\n\n"},{"id":"435050","messageId":"dfa27d7a8e7e0a9e03e228653ebff639a449ac77.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 03/11] t3407: use test_cmp_rev","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:14Z","receivedAt":"2021-09-08T09:49:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis provides better diagnostics if a test fails\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 0ad21966bc5..8d913d73bad 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -29,7 +29,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -39,9 +39,9 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\ttest_must_fail git rebase --skip &&\n-\t\ttest $(git rev-parse HEAD) = $(git rev-parse main) &&\n+\t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -54,9 +54,9 @@ testrebase() {\n \t\techo d >> a &&\n \t\tgit add a &&\n \t\ttest_must_fail git rebase --continue &&\n-\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n+\t\t! test_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '\n \ttest_path_is_dir .git/rebase-apply &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-apply\n '\n \n@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '\n \ttest_path_is_dir .git/rebase-merge &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-merge\n '\n \n-- \ngitgitgadget\n\n"},{"id":"435051","messageId":"bef70d86d53714cc8e5ade1b65adf6cd1b0fde28.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 04/11] t3407: rename a variable","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:15Z","receivedAt":"2021-09-08T09:49:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n$dotest holds the name of the rebase state directory and was named\nafter the state directory used at the time the test was added. As we\nno longer use that name rename the variable to reflect its purpose.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 8d913d73bad..d36a3f2758b 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -21,35 +21,35 @@ test_expect_success setup '\n \n testrebase() {\n \ttype=$1\n-\tdotest=$2\n+\tstate_dir=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\techo c > a &&\n \t\techo d >> a &&\n \t\tgit add a &&\n@@ -57,7 +57,7 @@ testrebase() {\n \t\t! test_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-- \ngitgitgadget\n\n"},{"id":"435052","messageId":"87d7e9bf2d4537ddbdec8c8fa67190e81a426312.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 06/11] t3407: strengthen rebase --abort tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:17Z","receivedAt":"2021-09-08T09:49:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe existing tests only check that HEAD points to the correct\ncommit after aborting, they do not check that the original branch\nis checked out.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 624d2ee3fa4..72cf391156d 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -19,6 +19,13 @@ test_expect_success setup '\n \ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n+# Check that HEAD is equal to \"pre-rebase\" and the current branch is\n+# \"to-rebase\"\n+check_head() {\n+\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n+}\n+\n testrebase() {\n \ttype=$1\n \tstate_dir=$2\n@@ -29,7 +36,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -41,7 +48,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -56,7 +63,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --continue &&\n \t\t! test_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n-- \ngitgitgadget\n\n"},{"id":"435053","messageId":"d9376fe08183af70eeb0ba0ebf6300af70cb51a8.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 05/11] t3407: use test_path_is_missing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:16Z","receivedAt":"2021-09-08T09:49:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAt the end of the test we expect the state directory to be missing,\nbut the tests only check it is not a directory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex d36a3f2758b..624d2ee3fa4 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -30,7 +30,7 @@ testrebase() {\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n@@ -42,7 +42,7 @@ testrebase() {\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n@@ -57,7 +57,7 @@ testrebase() {\n \t\t! test_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-apply\n+\ttest_path_is_missing .git/rebase-apply\n '\n \n test_expect_success 'rebase --merge --quit' '\n@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-merge\n+\ttest_path_is_missing .git/rebase-merge\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"435054","messageId":"9a4f6ea73c5e9052c7105b0fc63c6d76c0d3e92e.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 07/11] t3407: rework rebase --quit tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:18Z","receivedAt":"2021-09-08T09:49:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n9512177b68 (\"rebase: add --quit to cleanup rebase, leave everything\nelse untouched\", 2016-11-12) seems to have copied the --abort tests\nbut added two separate tests for the two rebase backends rather than\nadding a single test into the existing testrebase() function.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------\n 1 file changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 72cf391156d..2f41b06e028 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -86,31 +86,21 @@ testrebase() {\n \t\ttest_must_fail git rebase --abort -v &&\n \t\tgit rebase --abort\n \t'\n+\n+\ttest_expect_success 'rebase$type --quit' '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\t# Clean up the state from the previous one\n+\t\tgit reset --hard pre-rebase &&\n+\t\ttest_must_fail git rebase$type main &&\n+\t\ttest_path_is_dir $state_dir &&\n+\t\thead_before=$(git rev-parse HEAD) &&\n+\t\tgit rebase --quit &&\n+\t\ttest_cmp_rev HEAD $head_before &&\n+\t\ttest_path_is_missing .git/rebase-apply\n+\t'\n }\n \n testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n-test_expect_success 'rebase --apply --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --apply main &&\n-\ttest_path_is_dir .git/rebase-apply &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-apply\n-'\n-\n-test_expect_success 'rebase --merge --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --merge main &&\n-\ttest_path_is_dir .git/rebase-merge &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-merge\n-'\n-\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435055","messageId":"35b6c26c8f934e3bac984014a59c095c15416c79.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 08/11] rebase: remove redundant strbuf","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:19Z","receivedAt":"2021-09-08T09:49:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThere is already an strbuf that can be reused for creating messages.\nmsg is not freed if there is an error and there is a logic error where\nwe call strbuf_release(&msg) followed by strbuf_reset(&msg) and\nstrbuf_addf(&msg).\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 33e09619005..48ab7d9ae3b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ret, flags, total_argc, in_progress = 0;\n \tint keep_base = 0;\n \tint ok_to_skip_pre_rebase = 0;\n-\tstruct strbuf msg = STRBUF_INIT;\n \tstruct strbuf revisions = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_id merge_base;\n@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tprintf(_(\"First, rewinding head to replay your work on top of \"\n \t\t\t \"it...\\n\"));\n \n-\tstrbuf_addf(&msg, \"%s: checkout %s\",\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, \"%s: checkout %s\",\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(the_repository, &options.onto->object.oid, \"checkout\", NULL,\n \t\t RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n-\t\t NULL, msg.buf, DEFAULT_REFLOG_ACTION))\n+\t\t NULL, buf.buf, DEFAULT_REFLOG_ACTION))\n \t\tdie(_(\"Could not detach HEAD\"));\n-\tstrbuf_release(&msg);\n \n \t/*\n \t * If the onto is a proper descendant of the tip of the branch, then\n \t * we just fast-forwarded.\n \t */\n-\tstrbuf_reset(&msg);\n+\tstrbuf_reset(&buf);\n \tif (oideq(&merge_base, &options.orig_head)) {\n \t\tprintf(_(\"Fast-forwarded %s to %s.\\n\"),\n \t\t\tbranch_name, options.onto_name);\n-\t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n+\t\tstrbuf_addf(&buf, \"rebase finished: %s onto %s\",\n \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n \t\t\toid_to_hex(&options.onto->object.oid));\n \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n-\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n+\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n-\t\tstrbuf_release(&msg);\n \t\tret = !!finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"435056","messageId":"f0644cde7252920abc831e7a81f33c85f17e818b.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 09/11] rebase: use our standard error return value","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:20Z","receivedAt":"2021-09-08T09:49:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nGit uses −1 to signal an error. The builtin rebase converts these to\n+1 all over the place using !! (presumably because the in the scripted\nversion an error was signalled by +1). This is confusing and clutters\nthe code, we only need to convert the value when the function returns.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 48ab7d9ae3b..e8c3c77bab0 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t oid_to_hex(&options.orig_head));\n \t\tremove_branch_state(the_repository, 0);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n \n \t\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n-\t\t\tret = !!sequencer_remove_state(&replay);\n+\t\t\tret = sequencer_remove_state(&replay);\n \t\t} else {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addstr(&buf, options.state_dir);\n-\t\t\tret = !!remove_dir_recursively(&buf, 0);\n+\t\t\tret = remove_dir_recursively(&buf, 0);\n \t\t\tif (ret)\n \t\t\t\terror(_(\"could not remove '%s'\"),\n \t\t\t\t options.state_dir);\n@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (require_clean_work_tree(the_repository, \"rebase\",\n \t\t\t\t _(\"Please commit or stash them.\"), 1, 1)) {\n-\t\tret = 1;\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t\t\t\t NULL, buf.buf,\n \t\t\t\t\t DEFAULT_REFLOG_ACTION) < 0) {\n-\t\t\t\t\tret = !!error(_(\"could not switch to \"\n+\t\t\t\t\tret = error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n \t\t\t\t\t\t options.switch_to);\n \t\t\t\t\tgoto cleanup;\n@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tprintf(_(\"Current branch %s is up to date.\\n\"),\n \t\t\t\t branch_name);\n-\t\t\tret = !!finish_rebase(&options);\n+\t\t\tret = finish_rebase(&options);\n \t\t\tgoto cleanup;\n \t\t} else if (!(options.flags & REBASE_NO_QUIET))\n \t\t\t; /* be quiet */\n@@ -2085,7 +2085,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n \t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \n@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \toptions.revisions = revisions.buf;\n \n run_rebase:\n-\tret = !!run_specific_rebase(&options, action);\n+\tret = run_specific_rebase(&options, action);\n \n cleanup:\n \tstrbuf_release(&buf);\n@@ -2110,5 +2110,5 @@ cleanup:\n \tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n-\treturn ret;\n+\treturn !!ret;\n }\n-- \ngitgitgadget\n\n"},{"id":"435057","messageId":"e87ce4fe2537fa5ab6cedec43dd8ffb241bfe5ce.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 11/11] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:22Z","receivedAt":"2021-09-08T09:49:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAborting a rebase stated with 'git rebase '\nshould checkout the commit pointed to by . Instead it gives\n\n error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n trying to write non-commit object\n 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n\nThis is because when we parse the command line arguments although we\ncheck that the tag points to a commit we remember the oid of the tag\nand try and checkout that object rather than the commit it points\nto. Fix this by using lookup_commit_reference_by_name() when parsing\nthe command line.\n\nSigned-off-by: Phillip Wood \n---\n---\n builtin/rebase.c | 18 +++++++++++-------\n t/t3407-rebase-abort.sh | 18 ++++++++++++++----\n 2 files changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 93fcc0df2ad..8bf7660a24b 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie_if_checked_out(buf.buf, 1);\n \t\t\toptions.head_name = xstrdup(buf.buf);\n \t\t/* If not is it a valid ref (branch or commit)? */\n-\t\t} else if (!get_oid(branch_name, &options.orig_head) &&\n-\t\t\t lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t &options.orig_head))\n-\t\t\toptions.head_name = NULL;\n-\t\telse\n-\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n-\t\t\t branch_name);\n+\t\t} else {\n+\t\t\tstruct commit *commit =\n+\t\t\t\tlookup_commit_reference_by_name(branch_name);\n+\t\t\tif (commit) {\n+\t\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n+\t\t\t\toptions.head_name = NULL;\n+\t\t\t} else {\n+\t\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n+\t\t\t\t branch_name);\n+\t\t\t}\n+\t\t}\n \t} else if (argc == 0) {\n \t\t/* Do not need to switch branches, we are already on it. */\n \t\toptions.head_name =\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 2f41b06e028..310cd0c736c 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -11,18 +11,18 @@ test_expect_success setup '\n \ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\ttest_commit b a b &&\n-\ttest_commit c a c &&\n+\ttest_commit --annotate b a b &&\n+\ttest_commit --annotate c a c &&\n \n \tgit checkout to-rebase &&\n \ttest_commit \"merge should fail on this\" a d d &&\n-\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n+\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n '\n \n # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n # \"to-rebase\"\n check_head() {\n-\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n }\n \n@@ -67,6 +67,16 @@ testrebase() {\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n+\ttest_expect_success \"rebase$type --abort when checking out a tag\" '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\tgit reset --hard a -- &&\n+\t\ttest_must_fail git rebase$type --onto b c pre-rebase &&\n+\t\ttest_cmp_rev HEAD b^{commit} &&\n+\t\tgit rebase --abort &&\n+\t\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n+\t\t! git symbolic-ref HEAD\n+\t'\n+\n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n-- \ngitgitgadget\n"},{"id":"435058","messageId":"c537897006c7f9ab61b965f4d0b2dae30c0c7efe.1631094563.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH 10/11] rebase: use lookup_commit_reference_by_name()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T09:49:21Z","receivedAt":"2021-09-08T09:49:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\npeel_committish() appears to have been copied from the scripted rebase\nbut it duplicates the functionality of\nlookup_commit_reference_by_name() so lets use that instead.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 17 ++++-------------\n 1 file changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e8c3c77bab0..93fcc0df2ad 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static struct commit *peel_committish(const char *name)\n-{\n-\tstruct object *obj;\n-\tstruct object_id oid;\n-\n-\tif (get_oid(name, &oid))\n-\t\treturn NULL;\n-\tobj = parse_object(the_repository, &oid);\n-\treturn (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n-}\n-\n static void add_var(struct strbuf *buf, const char *name, const char *value)\n {\n \tif (!value)\n@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (!strcmp(options.upstream_name, \"-\"))\n \t\t\t\toptions.upstream_name = \"@{-1}\";\n \t\t}\n-\t\toptions.upstream = peel_committish(options.upstream_name);\n+\t\toptions.upstream =\n+\t\t\tlookup_commit_reference_by_name(options.upstream_name);\n \t\tif (!options.upstream)\n \t\t\tdie(_(\"invalid upstream '%s'\"), options.upstream_name);\n \t\toptions.upstream_arg = options.upstream_name;\n@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.onto = lookup_commit_or_die(&merge_base,\n \t\t\t\t\t\t options.onto_name);\n \t} else {\n-\t\toptions.onto = peel_committish(options.onto_name);\n+\t\toptions.onto =\n+\t\t\tlookup_commit_reference_by_name(options.onto_name);\n \t\tif (!options.onto)\n \t\t\tdie(_(\"Does not point to a valid commit '%s'\"),\n \t\t\t\toptions.onto_name);\n-- \ngitgitgadget\n\n"},{"id":"435061","messageId":"87mtonic6e.fsf@evledraar.gmail.com","threadId":"56463","inReplyTo":"79b6dec910ecc9ceb61d03e3bbd9919ce457d78e.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 02/11] t3407: use test_commit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T10:39:01Z","receivedAt":"2021-09-08T10:39:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> Simplify the setup by using test_commit. Note that this replaces the\n> branch \"pre-rebase\" with a tag of the same name. \"pre-rebase\" is only\n> used as a fixed reference point so it makes sense to use a tag rather\n> than a branch.\n\nFWIW you could use --no-tag to test_commit to keep the behavior, but\nchanging it here seems fine.\n"},{"id":"435062","messageId":"87ilzbic3l.fsf@evledraar.gmail.com","threadId":"56463","inReplyTo":"dfa27d7a8e7e0a9e03e228653ebff639a449ac77.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/11] t3407: use test_cmp_rev","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T10:40:00Z","receivedAt":"2021-09-08T10:41:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n\n> @@ -54,9 +54,9 @@ testrebase() {\n> \t\techo d >> a &&\n> \t\tgit add a &&\n> \t\ttest_must_fail git rebase --continue &&\n> -\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n> +\t\t! test_cmp_rev HEAD main &&\n\nUse \"test_cmp_rev !\", making the shell do the negation is troublesome\nfor the same reason we don't do it with git command, it potentially\nhides segfaults (and that test helper invokes \"git\", hence the optional\n\"!\" first rgument).\n"},{"id":"435063","messageId":"87ee9zic30.fsf@evledraar.gmail.com","threadId":"56463","inReplyTo":"0f5992e8caba424e484524252a303a00e108df34.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T10:41:15Z","receivedAt":"2021-09-08T10:41:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> Commit 97b88dd58c (\"git-rebase.sh: Fix --merge --abort failures when\n> path contains whitespace\", 2008-05-04) started running these tests in\n> a subdirectory with a space in its name. At that time $TEST_DIRECTORY\n> did not contain a space but shortly after in 4a7aaccd83 (\"Rename the\n> test trash directory to contain spaces.\", 2008-05-04) $TEST_DIRECTORY\n> was changed to contain a space so we no longer need to run these tests\n> in a subdirectory.\n\nWell spotted & a nice cleanup!\n"},{"id":"435064","messageId":"87a6knibxh.fsf@evledraar.gmail.com","threadId":"56463","inReplyTo":"87d7e9bf2d4537ddbdec8c8fa67190e81a426312.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 06/11] t3407: strengthen rebase --abort tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T10:42:04Z","receivedAt":"2021-09-08T10:44:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n\n> The existing tests only check that HEAD points to the correct\n> commit after aborting, they do not check that the original branch\n> is checked out.\n> [...]\n> +# Check that HEAD is equal to \"pre-rebase\" and the current branch is\n> +# \"to-rebase\"\n> +check_head() {\n> +\ttest_cmp_rev HEAD pre-rebase &&\n> +\ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n> +}\n\nI reflexively thought \"use test_cmp here!\", but in this case that's\npointless as any difference is noted by test_cmp_rev, we're really\nseeing what HEAD is checked out to here. I see we use the same pattern\nin various other tests...\n"},{"id":"435065","messageId":"875yvbibtg.fsf@evledraar.gmail.com","threadId":"56463","inReplyTo":"e87ce4fe2537fa5ab6cedec43dd8ffb241bfe5ce.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] rebase: dereference tags","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T10:45:20Z","receivedAt":"2021-09-08T10:47:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> Aborting a rebase stated with 'git rebase '\n> should checkout the commit pointed to by . Instead it gives\n>\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n> trying to write non-commit object\n> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n> This is because when we parse the command line arguments although we\n> check that the tag points to a commit we remember the oid of the tag\n> and try and checkout that object rather than the commit it points\n> to. Fix this by using lookup_commit_reference_by_name() when parsing\n> the command line.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> ---\n> builtin/rebase.c | 18 +++++++++++-------\n> t/t3407-rebase-abort.sh | 18 ++++++++++++++----\n> 2 files changed, 25 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 93fcc0df2ad..8bf7660a24b 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\tdie_if_checked_out(buf.buf, 1);\n> \t\t\toptions.head_name = xstrdup(buf.buf);\n> \t\t/* If not is it a valid ref (branch or commit)? */\n> -\t\t} else if (!get_oid(branch_name, &options.orig_head) &&\n> -\t\t\t lookup_commit_reference(the_repository,\n> -\t\t\t\t\t\t &options.orig_head))\n> -\t\t\toptions.head_name = NULL;\n> -\t\telse\n> -\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n> -\t\t\t branch_name);\n> +\t\t} else {\n> +\t\t\tstruct commit *commit =\n> +\t\t\t\tlookup_commit_reference_by_name(branch_name);\n> +\t\t\tif (commit) {\n> +\t\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n> +\t\t\t\toptions.head_name = NULL;\n> +\t\t\t} else {\n> +\t\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n> +\t\t\t\t branch_name);\n> +\t\t\t}\n> +\t\t}\n\nSuggested style nit:\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 8bf7660a24b..c751ef866fd 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1906,13 +1906,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t} else {\n \t\t\tstruct commit *commit =\n \t\t\t\tlookup_commit_reference_by_name(branch_name);\n-\t\t\tif (commit) {\n-\t\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n-\t\t\t\toptions.head_name = NULL;\n-\t\t\t} else {\n+\t\t\tif (!commit)\n \t\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n \t\t\t\t branch_name);\n-\t\t\t}\n+\n+\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n+\t\t\toptions.head_name = NULL;\n \t\t}\n \t} else if (argc == 0) {\n \t\t/* Do not need to switch branches, we are already on it. */\n\nI.e. handle the \"die\" case right away & skip the indenting of the\nnon-assert code.\n\n(Also grepping around we could really use a fatal non-gentle version of\nlookup_commit_reference_by_name(), but that's another more general\ncleanup...)\n\n> \t} else if (argc == 0) {\n> \t\t/* Do not need to switch branches, we are already on it. */\n> \t\toptions.head_name =\n> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\n> index 2f41b06e028..310cd0c736c 100755\n> --- a/t/t3407-rebase-abort.sh\n> +++ b/t/t3407-rebase-abort.sh\n> @@ -11,18 +11,18 @@ test_expect_success setup '\n> \ttest_commit a a a &&\n> \tgit branch to-rebase &&\n> \n> -\ttest_commit b a b &&\n> -\ttest_commit c a c &&\n> +\ttest_commit --annotate b a b &&\n> +\ttest_commit --annotate c a c &&\n> \n> \tgit checkout to-rebase &&\n> \ttest_commit \"merge should fail on this\" a d d &&\n> -\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n> +\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n> '\n> \n> # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n> # \"to-rebase\"\n> check_head() {\n> -\ttest_cmp_rev HEAD pre-rebase &&\n> +\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n> \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n> }\n> \n> @@ -67,6 +67,16 @@ testrebase() {\n> \t\ttest_path_is_missing \"$state_dir\"\n> \t'\n> \n> +\ttest_expect_success \"rebase$type --abort when checking out a tag\" '\n> +\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n> +\t\tgit reset --hard a -- &&\n> +\t\ttest_must_fail git rebase$type --onto b c pre-rebase &&\n> +\t\ttest_cmp_rev HEAD b^{commit} &&\n> +\t\tgit rebase --abort &&\n> +\t\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n> +\t\t! git symbolic-ref HEAD\n> +\t'\n> +\n> \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n> \t\t# Clean up the state from the previous one\n> \t\tgit reset --hard pre-rebase &&\n\nThis whole series looks good to me, left some comments on other\npatches. Consider the above suggested squash highly optional :)\n"},{"id":"435089","messageId":"e347691c-9f5e-9103-8690-014aa0054717@gmail.com","threadId":"56463","inReplyTo":"87ilzbic3l.fsf@evledraar.gmail.com","subject":"Re: [PATCH 03/11] t3407: use test_cmp_rev","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-08T13:42:04Z","receivedAt":"2021-09-08T13:42:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 08/09/2021 11:40, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:\n> \n>> @@ -54,9 +54,9 @@ testrebase() {\n>> \t\techo d >> a &&\n>> \t\tgit add a &&\n>> \t\ttest_must_fail git rebase --continue &&\n>> -\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n>> +\t\t! test_cmp_rev HEAD main &&\n> \n> Use \"test_cmp_rev !\", making the shell do the negation is troublesome\n> for the same reason we don't do it with git command, it potentially\n> hides segfaults (and that test helper invokes \"git\", hence the optional\n> \"!\" first argument).\n\nThanks, I didn't realize one could pass \"!\" to test_cmp_rev - it gives a \nbetter message on failure as well as avoiding hidden segfaults\n\nBest Wishes\n\nPhillip\n"},{"id":"435234","messageId":"nycvar.QRO.7.76.6.2109091233330.59@tvgsbejvaqbjf.bet","threadId":"56463","inReplyTo":"35b6c26c8f934e3bac984014a59c095c15416c79.1631094563.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 08/11] rebase: remove redundant strbuf","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-09T10:35:21Z","receivedAt":"2021-09-09T10:35:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood \n>\n> There is already an strbuf that can be reused for creating messages.\n> msg is not freed if there is an error and there is a logic error where\n> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and\n> strbuf_addf(&msg).\n\nIn some instances, we use a `buf` variable to construct a string that is\nthen assigned to a `const char *` and is used elsewhere throughout the\nfunction. If this was the case in the code you modified, that would be a\nproblem.\n\nI did verify manually that this is not the case, though, so this patch is\ngood to go.\n\nThanks,\nDscho\n"},{"id":"435731","messageId":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.git.1631094563.gitgitgadget@gmail.com","subject":"[PATCH v2 00/11] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:11Z","receivedAt":"2021-09-13T15:45:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks to Ævar and Johannes for their comments.\n\n * Changed \"! test_cmp_rev\" to \"test_cmp_rev !\" (suggested by Ævar)\n * Fixed the quoting for the title of the \"rebase --quit\" tests.\n * Reworked the last commit to handle the error case first (suggested by\n Ævar)\n * Tweaked the commit messages for patches 8 & 11\n * Rebased onto 31e4a0db03 (\"Merge branch 'ab/rebase-fatal-fatal-fix'\",\n 2021-09-08) to avoid a merge conflict that upset gitgitgadget\n\nCover letter for V1:\n\nAborting a rebase stated with git rebase should\ncheckout the commit pointed to by . Instead it gives\n\n error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n\n\nThe fix for that is in the last patch, the rest of the patches are cleanups\nto t3407 and builtin/rebase.c\n\nPhillip Wood (11):\n t3407: run tests in $TEST_DIRECTORY\n t3407: use test_commit\n t3407: use test_cmp_rev\n t3407: rename a variable\n t3407: use test_path_is_missing\n t3407: strengthen rebase --abort tests\n t3407: rework rebase --quit tests\n rebase: remove redundant strbuf\n rebase: use our standard error return value\n rebase: use lookup_commit_reference_by_name()\n rebase: dereference tags\n\n builtin/rebase.c | 63 +++++++++++-------------\n t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------\n 2 files changed, 73 insertions(+), 95 deletions(-)\n\n\nbase-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1033\n\nRange-diff vs v1:\n\n 1: 0f5992e8cab = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY\n 2: 79b6dec910e = 2: abfffb31a56 t3407: use test_commit\n 3: dfa27d7a8e7 ! 3: 7755ce17fef t3407: use test_cmp_rev\n @@ t/t3407-rebase-abort.sh: testrebase() {\n \t\tgit add a &&\n \t\ttest_must_fail git rebase --continue &&\n -\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n -+\t\t! test_cmp_rev HEAD main &&\n ++\t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n -\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n +\t\ttest_cmp_rev to-rebase pre-rebase &&\n 4: bef70d86d53 ! 4: 38eee11baf5 t3407: rename a variable\n @@ t/t3407-rebase-abort.sh: test_expect_success setup '\n \t\techo d >> a &&\n \t\tgit add a &&\n @@ t/t3407-rebase-abort.sh: testrebase() {\n - \t\t! test_cmp_rev HEAD main &&\n + \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n -\t\ttest ! -d \"$dotest\"\n 5: d9376fe0818 ! 5: 61a37c89f1e t3407: use test_path_is_missing\n @@ t/t3407-rebase-abort.sh: testrebase() {\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n @@ t/t3407-rebase-abort.sh: testrebase() {\n - \t\t! test_cmp_rev HEAD main &&\n + \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n -\t\ttest ! -d \"$state_dir\"\n 6: 87d7e9bf2d4 ! 6: 6866630528b t3407: strengthen rebase --abort tests\n @@ t/t3407-rebase-abort.sh: testrebase() {\n \n @@ t/t3407-rebase-abort.sh: testrebase() {\n \t\ttest_must_fail git rebase --continue &&\n - \t\t! test_cmp_rev HEAD main &&\n + \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n -\t\ttest_cmp_rev to-rebase pre-rebase &&\n +\t\tcheck_head &&\n 7: 9a4f6ea73c5 ! 7: fd55a3196b1 t3407: rework rebase --quit tests\n @@ t/t3407-rebase-abort.sh: testrebase() {\n \t\tgit rebase --abort\n \t'\n +\n -+\ttest_expect_success 'rebase$type --quit' '\n ++\ttest_expect_success \"rebase$type --quit\" '\n +\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n +\t\t# Clean up the state from the previous one\n +\t\tgit reset --hard pre-rebase &&\n 8: 35b6c26c8f9 = 8: ad3c4efc027 rebase: remove redundant strbuf\n 9: f0644cde725 = 9: ad940b633d0 rebase: use our standard error return value\n 10: c537897006c = 10: bc103e703e8 rebase: use lookup_commit_reference_by_name()\n 11: e87ce4fe253 ! 11: 951de6bb199 rebase: dereference tags\n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix\n -\t\t} else if (!get_oid(branch_name, &options.orig_head) &&\n -\t\t\t lookup_commit_reference(the_repository,\n -\t\t\t\t\t\t &options.orig_head))\n --\t\t\toptions.head_name = NULL;\n --\t\telse\n --\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n --\t\t\t branch_name);\n +\t\t} else {\n +\t\t\tstruct commit *commit =\n +\t\t\t\tlookup_commit_reference_by_name(branch_name);\n -+\t\t\tif (commit) {\n -+\t\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n -+\t\t\t\toptions.head_name = NULL;\n -+\t\t\t} else {\n -+\t\t\t\tdie(_(\"fatal: no such branch/commit '%s'\"),\n ++\t\t\tif (!commit)\n ++\t\t\t\tdie(_(\"no such branch/commit '%s'\"),\n +\t\t\t\t branch_name);\n -+\t\t\t}\n ++\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n + \t\t\toptions.head_name = NULL;\n +-\t\telse\n +-\t\t\tdie(_(\"no such branch/commit '%s'\"),\n +-\t\t\t branch_name);\n +\t\t}\n \t} else if (argc == 0) {\n \t\t/* Do not need to switch branches, we are already on it. */\n\n-- \ngitgitgadget\n"},{"id":"435732","messageId":"bac009d8543acdd85574c85a3e659dba541c4d0e.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:12Z","receivedAt":"2021-09-13T15:45:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCommit 97b88dd58c (\"git-rebase.sh: Fix --merge --abort failures when\npath contains whitespace\", 2008-05-04) started running these tests in\na subdirectory with a space in its name. At that time $TEST_DIRECTORY\ndid not contain a space but shortly after in 4a7aaccd83 (\"Rename the\ntest trash directory to contain spaces.\", 2008-05-04) $TEST_DIRECTORY\nwas changed to contain a space so we no longer need to run these tests\nin a subdirectory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 7c381fbc89a..c747bd31d76 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n-### Test that we handle space characters properly\n-work_dir=\"$(pwd)/test dir\"\n-\n test_expect_success setup '\n-\tmkdir -p \"$work_dir\" &&\n-\tcd \"$work_dir\" &&\n-\tgit init &&\n \techo a > a &&\n \tgit add a &&\n \tgit commit -m a &&\n@@ -37,7 +31,6 @@ testrebase() {\n \tdotest=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -48,7 +41,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -61,7 +53,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -77,7 +68,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\tgit reflog show to-rebase > reflog_before &&\n@@ -89,7 +79,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success 'rebase --abort can not be used with other options' '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -103,7 +92,6 @@ testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n test_expect_success 'rebase --apply --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --apply main &&\n@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '\n '\n \n test_expect_success 'rebase --merge --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --merge main &&\n-- \ngitgitgadget\n\n"},{"id":"435733","messageId":"abfffb31a562cc3b896c0a94a1ef7456657e1268.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 02/11] t3407: use test_commit","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:13Z","receivedAt":"2021-09-13T15:45:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSimplify the setup by using test_commit. Note that this replaces the\nbranch \"pre-rebase\" with a tag of the same name. \"pre-rebase\" is only\nused as a fixed reference point so it makes sense to use a tag rather\nthan a branch.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex c747bd31d76..0ad21966bc5 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n test_expect_success setup '\n-\techo a > a &&\n-\tgit add a &&\n-\tgit commit -m a &&\n+\ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\techo b > a &&\n-\tgit commit -a -m b &&\n-\techo c > a &&\n-\tgit commit -a -m c &&\n+\ttest_commit b a b &&\n+\ttest_commit c a c &&\n \n \tgit checkout to-rebase &&\n-\techo d > a &&\n-\tgit commit -a -m \"merge should fail on this\" &&\n-\techo e > a &&\n-\tgit commit -a -m \"merge should fail on this, too\" &&\n-\tgit branch pre-rebase\n+\ttest_commit \"merge should fail on this\" a d d &&\n+\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n testrebase() {\n-- \ngitgitgadget\n\n"},{"id":"435734","messageId":"7755ce17fef90729275334a735d31a348a0ddbc0.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 03/11] t3407: use test_cmp_rev","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:14Z","receivedAt":"2021-09-13T15:45:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis provides better diagnostics if a test fails\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 0ad21966bc5..39076ac9462 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -29,7 +29,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -39,9 +39,9 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\ttest_must_fail git rebase --skip &&\n-\t\ttest $(git rev-parse HEAD) = $(git rev-parse main) &&\n+\t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -54,9 +54,9 @@ testrebase() {\n \t\techo d >> a &&\n \t\tgit add a &&\n \t\ttest_must_fail git rebase --continue &&\n-\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n+\t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '\n \ttest_path_is_dir .git/rebase-apply &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-apply\n '\n \n@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '\n \ttest_path_is_dir .git/rebase-merge &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-merge\n '\n \n-- \ngitgitgadget\n\n"},{"id":"435735","messageId":"38eee11baf580adbef406cd776551dda012c8b4b.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 04/11] t3407: rename a variable","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:15Z","receivedAt":"2021-09-13T15:45:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n$dotest holds the name of the rebase state directory and was named\nafter the state directory used at the time the test was added. As we\nno longer use that name rename the variable to reflect its purpose.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 39076ac9462..2c70230a4eb 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -21,35 +21,35 @@ test_expect_success setup '\n \n testrebase() {\n \ttype=$1\n-\tdotest=$2\n+\tstate_dir=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\techo c > a &&\n \t\techo d >> a &&\n \t\tgit add a &&\n@@ -57,7 +57,7 @@ testrebase() {\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-- \ngitgitgadget\n\n"},{"id":"435736","messageId":"61a37c89f1e97c00b28daf1ce7a0c4253ecec9fb.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 05/11] t3407: use test_path_is_missing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:16Z","receivedAt":"2021-09-13T15:45:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAt the end of the test we expect the state directory to be missing,\nbut the tests only check it is not a directory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 2c70230a4eb..7eba9ec1619 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -30,7 +30,7 @@ testrebase() {\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n@@ -42,7 +42,7 @@ testrebase() {\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n@@ -57,7 +57,7 @@ testrebase() {\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-apply\n+\ttest_path_is_missing .git/rebase-apply\n '\n \n test_expect_success 'rebase --merge --quit' '\n@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-merge\n+\ttest_path_is_missing .git/rebase-merge\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"435737","messageId":"6866630528b24d7629adebea132970241fa0d380.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 06/11] t3407: strengthen rebase --abort tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:17Z","receivedAt":"2021-09-13T15:45:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe existing tests only check that HEAD points to the correct\ncommit after aborting, they do not check that the original branch\nis checked out.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 7eba9ec1619..f8264449b6c 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -19,6 +19,13 @@ test_expect_success setup '\n \ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n+# Check that HEAD is equal to \"pre-rebase\" and the current branch is\n+# \"to-rebase\"\n+check_head() {\n+\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n+}\n+\n testrebase() {\n \ttype=$1\n \tstate_dir=$2\n@@ -29,7 +36,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -41,7 +48,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -56,7 +63,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --continue &&\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n-- \ngitgitgadget\n\n"},{"id":"435738","messageId":"fd55a3196b16a2746cdf8ba374c0bbe1bc78ecbc.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 07/11] t3407: rework rebase --quit tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:18Z","receivedAt":"2021-09-13T15:45:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n9512177b68 (\"rebase: add --quit to cleanup rebase, leave everything\nelse untouched\", 2016-11-12) seems to have copied the --abort tests\nbut added two separate tests for the two rebase backends rather than\nadding a single test into the existing testrebase() function.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------\n 1 file changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex f8264449b6c..162112ba5ea 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -86,31 +86,21 @@ testrebase() {\n \t\ttest_must_fail git rebase --abort -v &&\n \t\tgit rebase --abort\n \t'\n+\n+\ttest_expect_success \"rebase$type --quit\" '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\t# Clean up the state from the previous one\n+\t\tgit reset --hard pre-rebase &&\n+\t\ttest_must_fail git rebase$type main &&\n+\t\ttest_path_is_dir $state_dir &&\n+\t\thead_before=$(git rev-parse HEAD) &&\n+\t\tgit rebase --quit &&\n+\t\ttest_cmp_rev HEAD $head_before &&\n+\t\ttest_path_is_missing .git/rebase-apply\n+\t'\n }\n \n testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n-test_expect_success 'rebase --apply --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --apply main &&\n-\ttest_path_is_dir .git/rebase-apply &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-apply\n-'\n-\n-test_expect_success 'rebase --merge --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --merge main &&\n-\ttest_path_is_dir .git/rebase-merge &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-merge\n-'\n-\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435739","messageId":"ad3c4efc0272be8eee052a08731656a406f8f90b.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 08/11] rebase: remove redundant strbuf","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:19Z","receivedAt":"2021-09-13T15:45:49Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThere is already an strbuf that can be reused for creating messages.\nmsg is not freed if there is an error and there is a logic error where\nwe call strbuf_release(&msg) followed by strbuf_reset(&msg) and\nstrbuf_addf(&msg).\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6138009d6e4..69a67ab1252 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tint ret, flags, total_argc, in_progress = 0;\n \tint keep_base = 0;\n \tint ok_to_skip_pre_rebase = 0;\n-\tstruct strbuf msg = STRBUF_INIT;\n \tstruct strbuf revisions = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct object_id merge_base;\n@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tprintf(_(\"First, rewinding head to replay your work on top of \"\n \t\t\t \"it...\\n\"));\n \n-\tstrbuf_addf(&msg, \"%s: checkout %s\",\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, \"%s: checkout %s\",\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(the_repository, &options.onto->object.oid, \"checkout\", NULL,\n \t\t RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n-\t\t NULL, msg.buf, DEFAULT_REFLOG_ACTION))\n+\t\t NULL, buf.buf, DEFAULT_REFLOG_ACTION))\n \t\tdie(_(\"Could not detach HEAD\"));\n-\tstrbuf_release(&msg);\n \n \t/*\n \t * If the onto is a proper descendant of the tip of the branch, then\n \t * we just fast-forwarded.\n \t */\n-\tstrbuf_reset(&msg);\n+\tstrbuf_reset(&buf);\n \tif (oideq(&merge_base, &options.orig_head)) {\n \t\tprintf(_(\"Fast-forwarded %s to %s.\\n\"),\n \t\t\tbranch_name, options.onto_name);\n-\t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n+\t\tstrbuf_addf(&buf, \"rebase finished: %s onto %s\",\n \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n \t\t\toid_to_hex(&options.onto->object.oid));\n \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n-\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n+\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n-\t\tstrbuf_release(&msg);\n \t\tret = !!finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"435740","messageId":"ad940b633d08ac3e364adc7015cfc52ad0f9d900.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 09/11] rebase: use our standard error return value","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:20Z","receivedAt":"2021-09-13T15:45:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nGit uses −1 to signal an error. The builtin rebase converts these to\n+1 all over the place using !! (presumably because the in the scripted\nversion an error was signalled by +1). This is confusing and clutters\nthe code, we only need to convert the value when the function returns.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 69a67ab1252..7905672d8de 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t oid_to_hex(&options.orig_head));\n \t\tremove_branch_state(the_repository, 0);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n \n \t\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n-\t\t\tret = !!sequencer_remove_state(&replay);\n+\t\t\tret = sequencer_remove_state(&replay);\n \t\t} else {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addstr(&buf, options.state_dir);\n-\t\t\tret = !!remove_dir_recursively(&buf, 0);\n+\t\t\tret = remove_dir_recursively(&buf, 0);\n \t\t\tif (ret)\n \t\t\t\terror(_(\"could not remove '%s'\"),\n \t\t\t\t options.state_dir);\n@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (require_clean_work_tree(the_repository, \"rebase\",\n \t\t\t\t _(\"Please commit or stash them.\"), 1, 1)) {\n-\t\tret = 1;\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t\t\t\t NULL, buf.buf,\n \t\t\t\t\t DEFAULT_REFLOG_ACTION) < 0) {\n-\t\t\t\t\tret = !!error(_(\"could not switch to \"\n+\t\t\t\t\tret = error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n \t\t\t\t\t\t options.switch_to);\n \t\t\t\t\tgoto cleanup;\n@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tprintf(_(\"Current branch %s is up to date.\\n\"),\n \t\t\t\t branch_name);\n-\t\t\tret = !!finish_rebase(&options);\n+\t\t\tret = finish_rebase(&options);\n \t\t\tgoto cleanup;\n \t\t} else if (!(options.flags & REBASE_NO_QUIET))\n \t\t\t; /* be quiet */\n@@ -2085,7 +2085,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n \t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \n@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \toptions.revisions = revisions.buf;\n \n run_rebase:\n-\tret = !!run_specific_rebase(&options, action);\n+\tret = run_specific_rebase(&options, action);\n \n cleanup:\n \tstrbuf_release(&buf);\n@@ -2110,5 +2110,5 @@ cleanup:\n \tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n-\treturn ret;\n+\treturn !!ret;\n }\n-- \ngitgitgadget\n\n"},{"id":"435741","messageId":"951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 11/11] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:22Z","receivedAt":"2021-09-13T15:45:51Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAborting a rebase stated with 'git rebase '\nshould checkout the commit pointed to by . Instead it gives\n\n error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n trying to write non-commit object\n 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n\nThis is because when we parse the command line arguments although we\ncheck that the tag points to a commit we remember the oid of the tag\nand try and checkout that object rather than the commit it points\nto. Fix this by using lookup_commit_reference_by_name() when parsing\nthe command line.\n\nSigned-off-by: Phillip Wood \n---\n---\n builtin/rebase.c | 14 ++++++++------\n t/t3407-rebase-abort.sh | 18 ++++++++++++++----\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 74663208468..2b70a196f9a 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie_if_checked_out(buf.buf, 1);\n \t\t\toptions.head_name = xstrdup(buf.buf);\n \t\t/* If not is it a valid ref (branch or commit)? */\n-\t\t} else if (!get_oid(branch_name, &options.orig_head) &&\n-\t\t\t lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t &options.orig_head))\n+\t\t} else {\n+\t\t\tstruct commit *commit =\n+\t\t\t\tlookup_commit_reference_by_name(branch_name);\n+\t\t\tif (!commit)\n+\t\t\t\tdie(_(\"no such branch/commit '%s'\"),\n+\t\t\t\t branch_name);\n+\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n \t\t\toptions.head_name = NULL;\n-\t\telse\n-\t\t\tdie(_(\"no such branch/commit '%s'\"),\n-\t\t\t branch_name);\n+\t\t}\n \t} else if (argc == 0) {\n \t\t/* Do not need to switch branches, we are already on it. */\n \t\toptions.head_name =\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 162112ba5ea..ebbaed147a6 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -11,18 +11,18 @@ test_expect_success setup '\n \ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\ttest_commit b a b &&\n-\ttest_commit c a c &&\n+\ttest_commit --annotate b a b &&\n+\ttest_commit --annotate c a c &&\n \n \tgit checkout to-rebase &&\n \ttest_commit \"merge should fail on this\" a d d &&\n-\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n+\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n '\n \n # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n # \"to-rebase\"\n check_head() {\n-\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n }\n \n@@ -67,6 +67,16 @@ testrebase() {\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n+\ttest_expect_success \"rebase$type --abort when checking out a tag\" '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\tgit reset --hard a -- &&\n+\t\ttest_must_fail git rebase$type --onto b c pre-rebase &&\n+\t\ttest_cmp_rev HEAD b^{commit} &&\n+\t\tgit rebase --abort &&\n+\t\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n+\t\t! git symbolic-ref HEAD\n+\t'\n+\n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n-- \ngitgitgadget\n"},{"id":"435742","messageId":"bc103e703e80072733859315c0ada3b154e30b71.1631546362.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v2 10/11] rebase: use lookup_commit_reference_by_name()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-13T15:19:21Z","receivedAt":"2021-09-13T15:45:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\npeel_committish() appears to have been copied from the scripted rebase\nbut it duplicates the functionality of\nlookup_commit_reference_by_name() so lets use that instead.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 17 ++++-------------\n 1 file changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 7905672d8de..74663208468 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static struct commit *peel_committish(const char *name)\n-{\n-\tstruct object *obj;\n-\tstruct object_id oid;\n-\n-\tif (get_oid(name, &oid))\n-\t\treturn NULL;\n-\tobj = parse_object(the_repository, &oid);\n-\treturn (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n-}\n-\n static void add_var(struct strbuf *buf, const char *name, const char *value)\n {\n \tif (!value)\n@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (!strcmp(options.upstream_name, \"-\"))\n \t\t\t\toptions.upstream_name = \"@{-1}\";\n \t\t}\n-\t\toptions.upstream = peel_committish(options.upstream_name);\n+\t\toptions.upstream =\n+\t\t\tlookup_commit_reference_by_name(options.upstream_name);\n \t\tif (!options.upstream)\n \t\t\tdie(_(\"invalid upstream '%s'\"), options.upstream_name);\n \t\toptions.upstream_arg = options.upstream_name;\n@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.onto = lookup_commit_or_die(&merge_base,\n \t\t\t\t\t\t options.onto_name);\n \t} else {\n-\t\toptions.onto = peel_committish(options.onto_name);\n+\t\toptions.onto =\n+\t\t\tlookup_commit_reference_by_name(options.onto_name);\n \t\tif (!options.onto)\n \t\t\tdie(_(\"Does not point to a valid commit '%s'\"),\n \t\t\t\toptions.onto_name);\n-- \ngitgitgadget\n\n"},{"id":"435767","messageId":"3c7c5560-2cae-a4cf-a298-6d77a3fb9226@web.de","threadId":"56463","inReplyTo":"ad3c4efc0272be8eee052a08731656a406f8f90b.1631546362.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 08/11] rebase: remove redundant strbuf","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-13T18:34:35Z","receivedAt":"2021-09-13T18:34:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:\n> From: Phillip Wood \n>\n> There is already an strbuf that can be reused for creating messages.\n\nReminds me of a terrible joke from elementary school: In Peter's class\neverybody is called Klaus, except Franz -- his name is Michael.\n\nWhy would we want to use the same variable for multiple purposes? This\nmakes the code harder to read. And the allocation overhead for these\nfew cases should be negligible.\n\nThe most important question: Is this patch really needed to support\ntags (the purpose of this series)?\n\n> msg is not freed if there is an error and there is a logic error where\n> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and\n> strbuf_addf(&msg).\n\nstrbuf_reset() after strbuf_release() is redundant but legal.\n\n>\n> Signed-off-by: Phillip Wood \n> ---\n> builtin/rebase.c | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 6138009d6e4..69a67ab1252 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tint ret, flags, total_argc, in_progress = 0;\n> \tint keep_base = 0;\n> \tint ok_to_skip_pre_rebase = 0;\n> -\tstruct strbuf msg = STRBUF_INIT;\n> \tstruct strbuf revisions = STRBUF_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct object_id merge_base;\n> @@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tprintf(_(\"First, rewinding head to replay your work on top of \"\n> \t\t\t \"it...\\n\"));\n>\n> -\tstrbuf_addf(&msg, \"%s: checkout %s\",\n> +\tstrbuf_reset(&buf);\n> +\tstrbuf_addf(&buf, \"%s: checkout %s\",\n> \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n> \tif (reset_head(the_repository, &options.onto->object.oid, \"checkout\", NULL,\n> \t\t RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n> \t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n> -\t\t NULL, msg.buf, DEFAULT_REFLOG_ACTION))\n> +\t\t NULL, buf.buf, DEFAULT_REFLOG_ACTION))\n> \t\tdie(_(\"Could not detach HEAD\"));\n> -\tstrbuf_release(&msg);\n>\n> \t/*\n> \t * If the onto is a proper descendant of the tip of the branch, then\n> \t * we just fast-forwarded.\n> \t */\n> -\tstrbuf_reset(&msg);\n> +\tstrbuf_reset(&buf);\n> \tif (oideq(&merge_base, &options.orig_head)) {\n> \t\tprintf(_(\"Fast-forwarded %s to %s.\\n\"),\n> \t\t\tbranch_name, options.onto_name);\n> -\t\tstrbuf_addf(&msg, \"rebase finished: %s onto %s\",\n> +\t\tstrbuf_addf(&buf, \"rebase finished: %s onto %s\",\n> \t\t\toptions.head_name ? options.head_name : \"detached HEAD\",\n> \t\t\toid_to_hex(&options.onto->object.oid));\n> \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n> -\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n> +\t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n> \t\t\t DEFAULT_REFLOG_ACTION);\n> -\t\tstrbuf_release(&msg);\n> \t\tret = !!finish_rebase(&options);\n> \t\tgoto cleanup;\n> \t}\n>\n\nmsg is not released if die() is called, but that's OK; in all other\ncases it _is_ released in the old code.\n\nI'd rather see the use of that multi-purpose \"buf\" reduced, e.g. we\ncould simplify path-building like this in a few cases:\n\n- strbuf_reset(&buf);\n- strbuf_addf(&buf, \"%s/applying\", apply_dir());\n- if(file_exists(buf.buf))\n+ if (file_exists(mkpath(\"%s/applying\", apply_dir())))\n\nSure, this looks a bit lispy, but still better than the old code\nbecause there is no state to carry around and reset when \"buf\" is\nrepurposed.\n\nRené\n"},{"id":"435795","messageId":"xmqqczpccd57.fsf@gitster.g","threadId":"56463","inReplyTo":"3c7c5560-2cae-a4cf-a298-6d77a3fb9226@web.de","subject":"Re: [PATCH v2 08/11] rebase: remove redundant strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T22:40:52Z","receivedAt":"2021-09-13T22:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:\n>> From: Phillip Wood \n>>\n>> There is already an strbuf that can be reused for creating messages.\n>\n> Reminds me of a terrible joke from elementary school: In Peter's class\n> everybody is called Klaus, except Franz -- his name is Michael.\n>\n> Why would we want to use the same variable for multiple purposes? This\n> makes the code harder to read. And the allocation overhead for these\n> few cases should be negligible.\n>\n> The most important question: Is this patch really needed to support\n> tags (the purpose of this series)?\n>\n>> msg is not freed if there is an error and there is a logic error where\n>> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and\n>> strbuf_addf(&msg).\n>\n> strbuf_reset() after strbuf_release() is redundant but legal.\n\nAll good points.\n\nI do not care too deeply either way, in the sense that it probably\nis better for this function to have two variables (with at least one\nof them having a meaningful name \"msg\" that tells readers what it is\nabout), if the original submission to rewrite \"rebase\" in C used a\nsingle strbuf for both of these and given it a name (like \"tmp\")\nthat makes it clear that the buffer is merely a temporary area\nwithout any longer term significance, I probably wouldn't have told\nthe submitter to rewrite it to use separate strbuf variables.\n\nBut if existing code already uses two variables, with at least one\nof them having a meaningful name that tells what it is used for, I\nsee no reason why we want to rewrite it to use a single one.\n\nThanks.\n"},{"id":"435796","messageId":"xmqq5yv4ccc0.fsf@gitster.g","threadId":"56463","inReplyTo":"951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T22:58:23Z","receivedAt":"2021-09-13T22:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> Aborting a rebase stated with 'git rebase '\n> should checkout the commit pointed to by . Instead it gives\n\nI am not sure if \"should checkout the commit pointed to by.\" is a\ngood description. It does not seem to be sufficiently justified.\n\nDid we auto-peel in scripted version of \"git rebase\" and is this a\nregression when the command was rewritten in C?\n\nIf that is not the case, this topic is perhaps slightly below\nborderline \"meh\" to me. The optional \"first switch to this \nbefore doing anything\" command-line argument in\n\n git rebase [--onto ] []\n\nwas meant to give a branch, and because we treat detached HEAD as\nalmost first-class citizen when dealing with branch-ish things, we\nallowed\n\n\tgit rebase master my-topic^0\n\nto try rebasing my-topic on detached HEAD without losing the\noriginal. In other words, you had to be explicit that you meant the\ncommit object, not a ref that points at it, to trigger this \"rebase\ndetached\" feature. The same thing for tags.\n\n\tgit rebase master v12.3^0\n\nwould be a proper request to rebase the history leading to that\ncommit. Without the peeling, it appears the user is asking to\nupdate the ref that can be uniquely identified with \"v12.3\", but we\ndo not want to rebase a tag.\n\nIt would have been a different story if we had a problem when a tag\nis given to \"--onto \", but I do not think this topic is about\nthat case.\n\nHaving said that, even if we decide that we shouldn't accept the tag\nobject and require peeled form to avoid mistakes (instead of\nsilently peeling the tag ourselves), I do agree that\n\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n\nis a bad error message for this. It should be something like\n\n\terror: cannot rebase a tag\n\nperhaps.\n\nBut if we auto-peeled in an old version, I do not mind this series\n(but let's drop pointless \"clean-up\" that is not, like what was\npointed out by Réne). In such a case, the first paragraph should\nsay, instead of \"should checkout\", that \"we used to do X, but commit\nY broke us and now we die with an error message\".\n\nThanks.\n"},{"id":"435815","messageId":"CABPp-BF3BPA77NUyhJ5OGxoBU3=NBD581xWeg0PO1zqV-nkW-A@mail.gmail.com","threadId":"56463","inReplyTo":"951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-14T03:42:33Z","receivedAt":"2021-09-14T03:42:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 13, 2021 at 8:47 AM Phillip Wood via GitGitGadget\n wrote:\n>\n> From: Phillip Wood \n>\n> Aborting a rebase stated with 'git rebase '\n\ns/stated/started/\n\n> should checkout the commit pointed to by . Instead it gives\n>\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n> trying to write non-commit object\n> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n> This is because when we parse the command line arguments although we\n> check that the tag points to a commit we remember the oid of the tag\n> and try and checkout that object rather than the commit it points\n> to. Fix this by using lookup_commit_reference_by_name() when parsing\n> the command line.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> ---\n> builtin/rebase.c | 14 ++++++++------\n> t/t3407-rebase-abort.sh | 18 ++++++++++++++----\n> 2 files changed, 22 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 74663208468..2b70a196f9a 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> die_if_checked_out(buf.buf, 1);\n> options.head_name = xstrdup(buf.buf);\n> /* If not is it a valid ref (branch or commit)? */\n> - } else if (!get_oid(branch_name, &options.orig_head) &&\n> - lookup_commit_reference(the_repository,\n> - &options.orig_head))\n> + } else {\n> + struct commit *commit =\n> + lookup_commit_reference_by_name(branch_name);\n> + if (!commit)\n> + die(_(\"no such branch/commit '%s'\"),\n> + branch_name);\n> + oidcpy(&options.orig_head, &commit->object.oid);\n> options.head_name = NULL;\n> - else\n> - die(_(\"no such branch/commit '%s'\"),\n> - branch_name);\n> + }\n> } else if (argc == 0) {\n> /* Do not need to switch branches, we are already on it. */\n> options.head_name =\n> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\n> index 162112ba5ea..ebbaed147a6 100755\n> --- a/t/t3407-rebase-abort.sh\n> +++ b/t/t3407-rebase-abort.sh\n> @@ -11,18 +11,18 @@ test_expect_success setup '\n> test_commit a a a &&\n> git branch to-rebase &&\n>\n> - test_commit b a b &&\n> - test_commit c a c &&\n> + test_commit --annotate b a b &&\n> + test_commit --annotate c a c &&\n>\n> git checkout to-rebase &&\n> test_commit \"merge should fail on this\" a d d &&\n> - test_commit \"merge should fail on this, too\" a e pre-rebase\n> + test_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n> '\n>\n> # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n> # \"to-rebase\"\n> check_head() {\n> - test_cmp_rev HEAD pre-rebase &&\n> + test_cmp_rev HEAD pre-rebase^{commit} &&\n> test \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n> }\n>\n> @@ -67,6 +67,16 @@ testrebase() {\n> test_path_is_missing \"$state_dir\"\n> '\n>\n> + test_expect_success \"rebase$type --abort when checking out a tag\" '\n> + test_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n> + git reset --hard a -- &&\n> + test_must_fail git rebase$type --onto b c pre-rebase &&\n> + test_cmp_rev HEAD b^{commit} &&\n> + git rebase --abort &&\n> + test_cmp_rev HEAD pre-rebase^{commit} &&\n> + ! git symbolic-ref HEAD\n> + '\n> +\n> test_expect_success \"rebase$type --abort does not update reflog\" '\n> # Clean up the state from the previous one\n> git reset --hard pre-rebase &&\n> --\n> gitgitgadget\n"},{"id":"435819","messageId":"CABPp-BHoJsrnUeRD-Ft7tO5MBhEcacWoMRmrZ6HReNXnjJKHew@mail.gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/11] rebase: dereference tags","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-14T04:02:48Z","receivedAt":"2021-09-14T04:03:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 13, 2021 at 8:46 AM Phillip Wood via GitGitGadget\n wrote:\n>\n> Thanks to Ævar and Johannes for their comments.\n>\n> * Changed \"! test_cmp_rev\" to \"test_cmp_rev !\" (suggested by Ævar)\n> * Fixed the quoting for the title of the \"rebase --quit\" tests.\n> * Reworked the last commit to handle the error case first (suggested by\n> Ævar)\n> * Tweaked the commit messages for patches 8 & 11\n> * Rebased onto 31e4a0db03 (\"Merge branch 'ab/rebase-fatal-fatal-fix'\",\n> 2021-09-08) to avoid a merge conflict that upset gitgitgadget\n>\n> Cover letter for V1:\n>\n> Aborting a rebase stated with git rebase should\n> checkout the commit pointed to by . Instead it gives\n>\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n>\n> The fix for that is in the last patch, the rest of the patches are cleanups\n> to t3407 and builtin/rebase.c\n\nMight make sense to split this into 3 separate series (t3407 cleanups,\nbuiltin/rebase.c cleanups, and the handling of tag objects). But\nanyway, reading over the 11 patches, the only issue I noticed was what\nappears to be a simple typo.\n"},{"id":"435833","messageId":"87r1dr5vyd.fsf@osv.gnss.ru","threadId":"56463","inReplyTo":"951de6bb1992773cda60791c4b7a09867b5e0f19.1631546362.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-09-14T09:48:42Z","receivedAt":"2021-09-14T09:48:47Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"\"Phillip Wood via GitGitGadget\" writes:\n\n> From: Phillip Wood \n>\n> Aborting a rebase stated with 'git rebase '\n> should checkout the commit pointed to by . Instead it gives\n>\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n> trying to write non-commit object\n> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n> This is because when we parse the command line arguments although we\n> check that the tag points to a commit we remember the oid of the tag\n> and try and checkout that object rather than the commit it points\n> to. Fix this by using lookup_commit_reference_by_name() when parsing\n> the command line.\n>\n> Signed-off-by: Phillip Wood \n\n[...]\n\n> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\n> index 162112ba5ea..ebbaed147a6 100755\n> --- a/t/t3407-rebase-abort.sh\n> +++ b/t/t3407-rebase-abort.sh\n> @@ -11,18 +11,18 @@ test_expect_success setup '\n> \ttest_commit a a a &&\n> \tgit branch to-rebase &&\n> \n> -\ttest_commit b a b &&\n> -\ttest_commit c a c &&\n> +\ttest_commit --annotate b a b &&\n> +\ttest_commit --annotate c a c &&\n> \n> \tgit checkout to-rebase &&\n> \ttest_commit \"merge should fail on this\" a d d &&\n> -\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n> +\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n> '\n\nThese two do not seem to belong to this particular commit?\n\n> \n> # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n> # \"to-rebase\"\n> check_head() {\n> -\ttest_cmp_rev HEAD pre-rebase &&\n> +\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n> \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n> }\n\n\n-- Sergey Organov\n"},{"id":"435834","messageId":"c0e95f69-b4e9-4510-8880-3110e9af36ab@gmail.com","threadId":"56463","inReplyTo":"87r1dr5vyd.fsf@osv.gnss.ru","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-14T09:58:01Z","receivedAt":"2021-09-14T09:58:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Sergey\n\nOn 14/09/2021 10:48, Sergey Organov wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> From: Phillip Wood \n>>\n>> Aborting a rebase stated with 'git rebase '\n>> should checkout the commit pointed to by . Instead it gives\n>>\n>> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n>> trying to write non-commit object\n>> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>>\n>> This is because when we parse the command line arguments although we\n>> check that the tag points to a commit we remember the oid of the tag\n>> and try and checkout that object rather than the commit it points\n>> to. Fix this by using lookup_commit_reference_by_name() when parsing\n>> the command line.\n>>\n>> Signed-off-by: Phillip Wood \n> \n> [...]\n> \n>> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\n>> index 162112ba5ea..ebbaed147a6 100755\n>> --- a/t/t3407-rebase-abort.sh\n>> +++ b/t/t3407-rebase-abort.sh\n>> @@ -11,18 +11,18 @@ test_expect_success setup '\n>> \ttest_commit a a a &&\n>> \tgit branch to-rebase &&\n>> \n>> -\ttest_commit b a b &&\n>> -\ttest_commit c a c &&\n>> +\ttest_commit --annotate b a b &&\n>> +\ttest_commit --annotate c a c &&\n>> \n>> \tgit checkout to-rebase &&\n>> \ttest_commit \"merge should fail on this\" a d d &&\n>> -\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n>> +\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n>> '\n> \n> These two do not seem to belong to this particular commit?\n\nThey do, the annotated tag is used in the new test added in this commit \nwhich tests that we peel tags.\n\nBest Wishes\n\nPhillip\n>> # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n>> # \"to-rebase\"\n>> check_head() {\n>> -\ttest_cmp_rev HEAD pre-rebase &&\n>> +\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n>> \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n>> }\n> \n> \n> -- Sergey Organov\n> \n"},{"id":"435835","messageId":"5ef402a4-3477-6227-e08c-041ed373313e@gmail.com","threadId":"56463","inReplyTo":"xmqq5yv4ccc0.fsf@gitster.g","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-14T10:17:48Z","receivedAt":"2021-09-14T10:18:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 13/09/2021 23:58, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" writes:\n> \n>> From: Phillip Wood \n>>\n>> Aborting a rebase stated with 'git rebase '\n>> should checkout the commit pointed to by . Instead it gives\n> \n> I am not sure if \"should checkout the commit pointed to by.\" is a\n> good description. It does not seem to be sufficiently justified.\n\nMy logic was that as we handle commits here it would make sense to \nhandle tags as well - I discovered that this did not work when I \nhappened to use an annotated tag as the argument to rebase the \ncommits pointed to by the tag and was surprised it did not work when we \nhappily accept tags for and --onto.\n\n> Did we auto-peel in scripted version of \"git rebase\" and is this a\n> regression when the command was rewritten in C?\n\nAs far as I can tell we have never peeled tags here\n\n> If that is not the case, this topic is perhaps slightly below\n> borderline \"meh\" to me. The optional \"first switch to this \n> before doing anything\" command-line argument in\n> \n> git rebase [--onto ] []\n> \n> was meant to give a branch, and because we treat detached HEAD as\n> almost first-class citizen when dealing with branch-ish things, we\n> allowed\n> \n> \tgit rebase master my-topic^0\n> \n> to try rebasing my-topic on detached HEAD without losing the\n> original. In other words, you had to be explicit that you meant the\n> commit object, not a ref that points at it, to trigger this \"rebase\n> detached\" feature. The same thing for tags.\n> \n> \tgit rebase master v12.3^0\n> \n> would be a proper request to rebase the history leading to that\n> commit. Without the peeling, it appears the user is asking to\n> update the ref that can be uniquely identified with \"v12.3\", but we\n> do not want to rebase a tag.\n\nI wrote this patch as I felt it was an artificial distinction to require \nthat is a branch-ish thing rather than a commit-ish thing. \nRebase already peels and --onto so it feels inconsistent not \nto do it for . I guess the counter argument to that is users may \nbe confused and start complaining that the tag itself is not rebased.\n\n> It would have been a different story if we had a problem when a tag\n> is given to \"--onto \", but I do not think this topic is about\n> that case.\n\nNo \"--onto \" works fine. We also accept a tag object for upstream \nwithout requiring the user to peel it for us.\n\n> Having said that, even if we decide that we shouldn't accept the tag\n> object and require peeled form to avoid mistakes (instead of\n> silently peeling the tag ourselves), I do agree that\n> \n>> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>>\n> \n> is a bad error message for this. It should be something like\n> \n> \terror: cannot rebase a tag\n> \n> perhaps.\n\nWe could do that if we're worried that users would be confused by the \ntag not being rebased if we started automatically peeling . (I'm \nkind of leaning in that direction at the moment having read your email)\n\nBest Wishes\n\nPhillip\n\n> But if we auto-peeled in an old version, I do not mind this series\n> (but let's drop pointless \"clean-up\" that is not, like what was\n> pointed out by Réne). In such a case, the first paragraph should\n> say, instead of \"should checkout\", that \"we used to do X, but commit\n> Y broke us and now we die with an error message\".\n> \n> Thanks.\n> \n"},{"id":"435838","messageId":"0c067b02-44e4-f719-42d9-7ceac5804e4a@gmail.com","threadId":"56463","inReplyTo":"xmqqczpccd57.fsf@gitster.g","subject":"Re: [PATCH v2 08/11] rebase: remove redundant strbuf","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-14T10:31:41Z","receivedAt":"2021-09-14T10:31:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/09/2021 23:40, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:\n>>> From: Phillip Wood \n>>>\n>>> There is already an strbuf that can be reused for creating messages.\n>>\n>> Reminds me of a terrible joke from elementary school: In Peter's class\n>> everybody is called Klaus, except Franz -- his name is Michael.\n>>\n>> Why would we want to use the same variable for multiple purposes? This\n>> makes the code harder to read. And the allocation overhead for these\n>> few cases should be negligible.\n\nFor better or worse reusing the same strbuf is a common pattern and when \nI saw the code switch to using a different variable I wondered if it was \nbecause the value of buf was being used later. It is also confusing to \nfree msg in a different place to all the other variables. rebase_cmd() \nbeing so long does not help (msg is defined 763 lines before its first \nuse) as it makes it harder to see what is going on.\n\n>> The most important question: Is this patch really needed to support\n>> tags (the purpose of this series)?\n>>\n>>> msg is not freed if there is an error and there is a logic error where\n>>> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and\n>>> strbuf_addf(&msg).\n>>\n>> strbuf_reset() after strbuf_release() is redundant but legal.\n\nIt is confusing to the reader though, I spent time checking why the \nstrbuf_release() call was there before concluding it was a mistake.\n\n> All good points.\n> \n> I do not care too deeply either way, in the sense that it probably\n> is better for this function to have two variables (with at least one\n> of them having a meaningful name \"msg\" that tells readers what it is\n> about), if the original submission to rewrite \"rebase\" in C used a\n> single strbuf for both of these and given it a name (like \"tmp\")\n> that makes it clear that the buffer is merely a temporary area\n> without any longer term significance, I probably wouldn't have told\n> the submitter to rewrite it to use separate strbuf variables.\n> \n> But if existing code already uses two variables, with at least one\n> of them having a meaningful name that tells what it is used for, I\n> see no reason why we want to rewrite it to use a single one.\n\nIn a short function where it is easy to see what happens between the \nvariable declaration and its use I'd agree but here everything is so \nspread out I actually found the switch to a second variable confusing.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n"},{"id":"435839","messageId":"b6cb3935-7e6a-5440-3439-7e9076ba5051@gmail.com","threadId":"56463","inReplyTo":"3c7c5560-2cae-a4cf-a298-6d77a3fb9226@web.de","subject":"Re: [PATCH v2 08/11] rebase: remove redundant strbuf","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-14T10:33:57Z","receivedAt":"2021-09-14T10:34:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nThanks for looking at this series\n\nOn 13/09/2021 19:34, René Scharfe wrote:\n> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:\n>> From: Phillip Wood \n> I'd rather see the use of that multi-purpose \"buf\" reduced, e.g. we\n> could simplify path-building like this in a few cases:\n> \n> - strbuf_reset(&buf);\n> - strbuf_addf(&buf, \"%s/applying\", apply_dir());\n> - if(file_exists(buf.buf))\n> + if (file_exists(mkpath(\"%s/applying\", apply_dir())))\n> \n> Sure, this looks a bit lispy, but still better than the old code\n> because there is no state to carry around and reset when \"buf\" is\n> repurposed.\n\nThat's a nice suggestion, I think it's much clearer which file we're \nchecking for.\n\nBest Wishes\n\nPhillip\n"},{"id":"435845","messageId":"8c78eac4-676b-7bd1-0282-d52eb20f87ce@gmail.com","threadId":"56463","inReplyTo":"5ef402a4-3477-6227-e08c-041ed373313e@gmail.com","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-09-14T13:27:17Z","receivedAt":"2021-09-14T13:27:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/09/2021 11:17, Phillip Wood wrote:\n> Hi Junio\n> \n> On 13/09/2021 23:58, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" writes:\n>>\n>>> From: Phillip Wood \n>>>\n>>> Aborting a rebase stated with 'git rebase '\n>>> should checkout the commit pointed to by . Instead it gives\n>>\n>> I am not sure if \"should checkout the commit pointed to by.\" is a\n>> good description.  It does not seem to be sufficiently justified.\n> \n> My logic was that as we handle commits here it would make sense to \n> handle tags as well - I discovered that this did not work when I \n> happened to use an annotated tag as the argument to rebase the \n> commits pointed to by the tag and was surprised it did not work when we \n> happily accept tags for and --onto.\n> \n>> Did we auto-peel in scripted version of \"git rebase\" and is this a\n>> regression when the command was rewritten in C?\n> \n> As far as I can tell we have never peeled tags here\n\nThat's a bit misleading. We have never peeled a tag given as \nwhen we parse it. In the scripted version we just passed the tag oid \nalong to rev-list, checkout and reset and they peeled it. So I think \nthis is actually a regression in the builtin rebase. I'll update the \ncommit message to reflect that unless we feel that allowing a tag for \n is a mistake and we should be erroring out to avoid the \npossible confusion of the tag not being rebased, only the commits it \npoints to.\n\nSorry for the confusion\n\nPhillip\n\n>> If that is not the case, this topic is perhaps slightly below\n>> borderline \"meh\" to me.  The optional \"first switch to this \n>> before doing anything\" command-line argument in\n>>\n>>      git rebase [--onto ] []\n>>\n>> was meant to give a branch, and because we treat detached HEAD as\n>> almost first-class citizen when dealing with branch-ish things, we\n>> allowed\n>>\n>>     git rebase master my-topic^0\n>>\n>> to try rebasing my-topic on detached HEAD without losing the\n>> original.  In other words, you had to be explicit that you meant the\n>> commit object, not a ref that points at it, to trigger this \"rebase\n>> detached\" feature.  The same thing for tags.\n>>\n>>     git rebase master v12.3^0\n>>\n>> would be a proper request to rebase the history leading to that\n>> commit.  Without the peeling, it appears the user is asking to\n>> update the ref that can be uniquely identified with \"v12.3\", but we\n>> do not want to rebase a tag.\n> \n> I wrote this patch as I felt it was an artificial distinction to require \n> that is a branch-ish thing rather than a commit-ish thing. \n> Rebase already peels and --onto so it feels inconsistent not \n> to do it for . I guess the counter argument to that is users may \n> be confused and start complaining that the tag itself is not rebased.\n> \n>> It would have been a different story if we had a problem when a tag\n>> is given to \"--onto \", but I do not think this topic is about\n>> that case.\n> \n> No \"--onto \" works fine. We also accept a tag object for upstream \n> without requiring the user to peel it for us.\n> \n>> Having said that, even if we decide that we shouldn't accept the tag\n>> object and require peeled form to avoid mistakes (instead of\n>> silently peeling the tag ourselves), I do agree that\n>>\n>>>      error: update_ref failed for ref 'HEAD': cannot update ref \n>>> 'HEAD': trying to write non-commit object \n>>> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>>>\n>>\n>> is a bad error message for this.  It should be something like\n>>\n>>     error: cannot rebase a tag\n>>\n>> perhaps.\n> \n> We could do that if we're worried that users would be confused by the \n> tag not being rebased if we started automatically peeling . (I'm \n> kind of leaning in that direction at the moment having read your email)\n> \n> Best Wishes\n> \n> Phillip\n> \n>> But if we auto-peeled in an old version, I do not mind this series\n>> (but let's drop pointless \"clean-up\" that is not, like what was\n>> pointed out by Réne).  In such a case, the first paragraph should\n>> say, instead of \"should checkout\", that \"we used to do X, but commit\n>> Y broke us and now we die with an error message\".\n>>\n>> Thanks.\n>>\n"},{"id":"435879","messageId":"xmqqbl4v3yun.fsf@gitster.g","threadId":"56463","inReplyTo":"8c78eac4-676b-7bd1-0282-d52eb20f87ce@gmail.com","subject":"Re: [PATCH v2 11/11] rebase: dereference tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-14T16:29:04Z","receivedAt":"2021-09-14T16:29:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> Did we auto-peel in scripted version of \"git rebase\" and is this a\n>>> regression when the command was rewritten in C?\n>> As far as I can tell we have never peeled tags here\n>\n> That's a bit misleading. We have never peeled a tag given as \n> when we parse it. In the scripted version we just passed the tag oid \n> along to rev-list, checkout and reset and they peeled it. So I think\n> this is actually a regression in the builtin rebase. I'll update the \n> commit message to reflect that unless we feel that allowing a tag for\n> is a mistake and we should be erroring out to avoid the \n> possible confusion of the tag not being rebased, only the commits it\n> points to.\n\nOK, so this is a regression fix. That makes the change much simpler\nto sell. I'd expect that the description would be along the lines\nof something like this, perhaps.\n\n A rebase started with 'git rebase ' is conceptually to\n first checkout and run 'git rebase ' starting from that\n state. 'git rebase --abort' in the middle of such a rebase\n should take us back to the state we checked out .\n\n This used to work, even when is a tag that points at a\n commit, until Git X.Y.Z when the command was reimplemented in C.\n The command now complains that the tag object itself cannot be\n checked out, which may be technically correct but is not what\n the user asked to do.\n\n Fix this old regression by doing ....\n\nThanks for digging (and fixing, of course).\n\n"},{"id":"436582","messageId":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v2.git.1631546362.gitgitgadget@gmail.com","subject":"[PATCH v3 00/10] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:23:57Z","receivedAt":"2021-09-21T10:24:12Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Thanks for the comments on V2. Here are the changes from that version:\n\n * dropped the controversial cleanup that was patch 8\n * reworded patch 10 as suggested by Junio\n\nCover letter for v2: Thanks to Ævar and Johannes for their comments.\n\n * Changed \"! test_cmp_rev\" to \"test_cmp_rev !\" (suggested by Ævar)\n * Fixed the quoting for the title of the \"rebase --quit\" tests.\n * Reworked the last commit to handle the error case first (suggested by\n Ævar)\n * Tweaked the commit messages for patches 8 & 11\n * Rebased onto 31e4a0db03 (\"Merge branch 'ab/rebase-fatal-fatal-fix'\",\n 2021-09-08) to avoid a merge conflict that upset gitgitgadget\n\nCover letter for V1: Aborting a rebase stated with git rebase \n should checkout the commit pointed to by . Instead it gives\n\n error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n\n\nThe fix for that is in the last patch, the rest of the patches are cleanups\nto t3407 and builtin/rebase.c\n\nPhillip Wood (10):\n t3407: run tests in $TEST_DIRECTORY\n t3407: use test_commit\n t3407: use test_cmp_rev\n t3407: rename a variable\n t3407: use test_path_is_missing\n t3407: strengthen rebase --abort tests\n t3407: rework rebase --quit tests\n rebase: use our standard error return value\n rebase: use lookup_commit_reference_by_name()\n rebase: dereference tags\n\n builtin/rebase.c | 49 ++++++++-----------\n t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------\n 2 files changed, 67 insertions(+), 87 deletions(-)\n\n\nbase-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1033\n\nRange-diff vs v2:\n\n 1: bac009d8543 = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY\n 2: abfffb31a56 = 2: abfffb31a56 t3407: use test_commit\n 3: 7755ce17fef = 3: 7755ce17fef t3407: use test_cmp_rev\n 4: 38eee11baf5 = 4: 38eee11baf5 t3407: rename a variable\n 5: 61a37c89f1e = 5: 61a37c89f1e t3407: use test_path_is_missing\n 6: 6866630528b = 6: 6866630528b t3407: strengthen rebase --abort tests\n 7: fd55a3196b1 = 7: fd55a3196b1 t3407: rework rebase --quit tests\n 8: ad3c4efc027 < -: ----------- rebase: remove redundant strbuf\n 9: ad940b633d0 ! 8: d3af62c746c rebase: use our standard error return value\n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix\n \t\t} else if (!(options.flags & REBASE_NO_QUIET))\n \t\t\t; /* be quiet */\n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \t\treset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n - \t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n + \t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n + \t\tstrbuf_release(&msg);\n -\t\tret = !!finish_rebase(&options);\n +\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n 10: bc103e703e8 = 9: 4eedd3ae766 rebase: use lookup_commit_reference_by_name()\n 11: 951de6bb199 ! 10: 55a6250ab38 rebase: dereference tags\n @@ Metadata\n ## Commit message ##\n rebase: dereference tags\n \n - Aborting a rebase stated with 'git rebase '\n - should checkout the commit pointed to by . Instead it gives\n + A rebase started with 'git rebase ' is conceptually to first\n + checkout and run 'git rebase ' starting from that state. 'git\n + rebase --abort' in the middle of such a rebase should take us back to\n + the state we checked out .\n \n - error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n - trying to write non-commit object\n - 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n + This used to work, even when is a tag that points at a commit,\n + until Git 2.20.0 when the command was reimplemented in C. The command\n + now complains that the tag object itself cannot be checked out, which\n + may be technically correct but is not what the user asked to do.\n \n - This is because when we parse the command line arguments although we\n - check that the tag points to a commit we remember the oid of the tag\n - and try and checkout that object rather than the commit it points\n - to. Fix this by using lookup_commit_reference_by_name() when parsing\n - the command line.\n + Fix this old regression by using lookup_commit_reference_by_name()\n + when parsing . The scripted version did not need to peel the tag\n + because the commands it passed the tag to (e.g 'git reset') peeled the\n + tag themselves.\n \n Signed-off-by: Phillip Wood \n ---\n\n-- \ngitgitgadget\n"},{"id":"436583","messageId":"bac009d8543acdd85574c85a3e659dba541c4d0e.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:23:58Z","receivedAt":"2021-09-21T10:24:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCommit 97b88dd58c (\"git-rebase.sh: Fix --merge --abort failures when\npath contains whitespace\", 2008-05-04) started running these tests in\na subdirectory with a space in its name. At that time $TEST_DIRECTORY\ndid not contain a space but shortly after in 4a7aaccd83 (\"Rename the\ntest trash directory to contain spaces.\", 2008-05-04) $TEST_DIRECTORY\nwas changed to contain a space so we no longer need to run these tests\nin a subdirectory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 -------------\n 1 file changed, 13 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 7c381fbc89a..c747bd31d76 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n-### Test that we handle space characters properly\n-work_dir=\"$(pwd)/test dir\"\n-\n test_expect_success setup '\n-\tmkdir -p \"$work_dir\" &&\n-\tcd \"$work_dir\" &&\n-\tgit init &&\n \techo a > a &&\n \tgit add a &&\n \tgit commit -m a &&\n@@ -37,7 +31,6 @@ testrebase() {\n \tdotest=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -48,7 +41,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -61,7 +53,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -77,7 +68,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\tgit reflog show to-rebase > reflog_before &&\n@@ -89,7 +79,6 @@ testrebase() {\n \t'\n \n \ttest_expect_success 'rebase --abort can not be used with other options' '\n-\t\tcd \"$work_dir\" &&\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n@@ -103,7 +92,6 @@ testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n test_expect_success 'rebase --apply --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --apply main &&\n@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '\n '\n \n test_expect_success 'rebase --merge --quit' '\n-\tcd \"$work_dir\" &&\n \t# Clean up the state from the previous one\n \tgit reset --hard pre-rebase &&\n \ttest_must_fail git rebase --merge main &&\n-- \ngitgitgadget\n\n"},{"id":"436584","messageId":"abfffb31a562cc3b896c0a94a1ef7456657e1268.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 02/10] t3407: use test_commit","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:23:59Z","receivedAt":"2021-09-21T10:24:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nSimplify the setup by using test_commit. Note that this replaces the\nbranch \"pre-rebase\" with a tag of the same name. \"pre-rebase\" is only\nused as a fixed reference point so it makes sense to use a tag rather\nthan a branch.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex c747bd31d76..0ad21966bc5 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n test_expect_success setup '\n-\techo a > a &&\n-\tgit add a &&\n-\tgit commit -m a &&\n+\ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\techo b > a &&\n-\tgit commit -a -m b &&\n-\techo c > a &&\n-\tgit commit -a -m c &&\n+\ttest_commit b a b &&\n+\ttest_commit c a c &&\n \n \tgit checkout to-rebase &&\n-\techo d > a &&\n-\tgit commit -a -m \"merge should fail on this\" &&\n-\techo e > a &&\n-\tgit commit -a -m \"merge should fail on this, too\" &&\n-\tgit branch pre-rebase\n+\ttest_commit \"merge should fail on this\" a d d &&\n+\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n testrebase() {\n-- \ngitgitgadget\n\n"},{"id":"436585","messageId":"7755ce17fef90729275334a735d31a348a0ddbc0.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 03/10] t3407: use test_cmp_rev","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:00Z","receivedAt":"2021-09-21T10:24:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis provides better diagnostics if a test fails\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 0ad21966bc5..39076ac9462 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -29,7 +29,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -39,9 +39,9 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$dotest\" &&\n \t\ttest_must_fail git rebase --skip &&\n-\t\ttest $(git rev-parse HEAD) = $(git rev-parse main) &&\n+\t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -54,9 +54,9 @@ testrebase() {\n \t\techo d >> a &&\n \t\tgit add a &&\n \t\ttest_must_fail git rebase --continue &&\n-\t\ttest $(git rev-parse HEAD) != $(git rev-parse main) &&\n+\t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&\n+\t\ttest_cmp_rev to-rebase pre-rebase &&\n \t\ttest ! -d \"$dotest\"\n \t'\n \n@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '\n \ttest_path_is_dir .git/rebase-apply &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-apply\n '\n \n@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '\n \ttest_path_is_dir .git/rebase-merge &&\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n-\ttest $(git rev-parse HEAD) = $head_before &&\n+\ttest_cmp_rev HEAD $head_before &&\n \ttest ! -d .git/rebase-merge\n '\n \n-- \ngitgitgadget\n\n"},{"id":"436586","messageId":"38eee11baf580adbef406cd776551dda012c8b4b.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 04/10] t3407: rename a variable","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:01Z","receivedAt":"2021-09-21T10:24:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n$dotest holds the name of the rebase state directory and was named\nafter the state directory used at the time the test was added. As we\nno longer use that name rename the variable to reflect its purpose.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 39076ac9462..2c70230a4eb 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -21,35 +21,35 @@ test_expect_success setup '\n \n testrebase() {\n \ttype=$1\n-\tdotest=$2\n+\tstate_dir=$2\n \n \ttest_expect_success \"rebase$type --abort\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n \t\ttest_must_fail git rebase$type main &&\n-\t\ttest_path_is_dir \"$dotest\" &&\n+\t\ttest_path_is_dir \"$state_dir\" &&\n \t\techo c > a &&\n \t\techo d >> a &&\n \t\tgit add a &&\n@@ -57,7 +57,7 @@ testrebase() {\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$dotest\"\n+\t\ttest ! -d \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n-- \ngitgitgadget\n\n"},{"id":"436587","messageId":"61a37c89f1e97c00b28daf1ce7a0c4253ecec9fb.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 05/10] t3407: use test_path_is_missing","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:02Z","receivedAt":"2021-09-21T10:24:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAt the end of the test we expect the state directory to be missing,\nbut the tests only check it is not a directory.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 2c70230a4eb..7eba9ec1619 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -30,7 +30,7 @@ testrebase() {\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --skip\" '\n@@ -42,7 +42,7 @@ testrebase() {\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort after --continue\" '\n@@ -57,7 +57,7 @@ testrebase() {\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n \t\ttest_cmp_rev to-rebase pre-rebase &&\n-\t\ttest ! -d \"$state_dir\"\n+\t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-apply\n+\ttest_path_is_missing .git/rebase-apply\n '\n \n test_expect_success 'rebase --merge --quit' '\n@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '\n \thead_before=$(git rev-parse HEAD) &&\n \tgit rebase --quit &&\n \ttest_cmp_rev HEAD $head_before &&\n-\ttest ! -d .git/rebase-merge\n+\ttest_path_is_missing .git/rebase-merge\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"436588","messageId":"6866630528b24d7629adebea132970241fa0d380.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 06/10] t3407: strengthen rebase --abort tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:03Z","receivedAt":"2021-09-21T10:24:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe existing tests only check that HEAD points to the correct\ncommit after aborting, they do not check that the original branch\nis checked out.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 7eba9ec1619..f8264449b6c 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -19,6 +19,13 @@ test_expect_success setup '\n \ttest_commit \"merge should fail on this, too\" a e pre-rebase\n '\n \n+# Check that HEAD is equal to \"pre-rebase\" and the current branch is\n+# \"to-rebase\"\n+check_head() {\n+\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n+}\n+\n testrebase() {\n \ttype=$1\n \tstate_dir=$2\n@@ -29,7 +36,7 @@ testrebase() {\n \t\ttest_must_fail git rebase$type main &&\n \t\ttest_path_is_dir \"$state_dir\" &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -41,7 +48,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --skip &&\n \t\ttest_cmp_rev HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n@@ -56,7 +63,7 @@ testrebase() {\n \t\ttest_must_fail git rebase --continue &&\n \t\ttest_cmp_rev ! HEAD main &&\n \t\tgit rebase --abort &&\n-\t\ttest_cmp_rev to-rebase pre-rebase &&\n+\t\tcheck_head &&\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n-- \ngitgitgadget\n\n"},{"id":"436589","messageId":"fd55a3196b16a2746cdf8ba374c0bbe1bc78ecbc.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 07/10] t3407: rework rebase --quit tests","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:04Z","receivedAt":"2021-09-21T10:24:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n9512177b68 (\"rebase: add --quit to cleanup rebase, leave everything\nelse untouched\", 2016-11-12) seems to have copied the --abort tests\nbut added two separate tests for the two rebase backends rather than\nadding a single test into the existing testrebase() function.\n\nSigned-off-by: Phillip Wood \n---\n t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------\n 1 file changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex f8264449b6c..162112ba5ea 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -86,31 +86,21 @@ testrebase() {\n \t\ttest_must_fail git rebase --abort -v &&\n \t\tgit rebase --abort\n \t'\n+\n+\ttest_expect_success \"rebase$type --quit\" '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\t# Clean up the state from the previous one\n+\t\tgit reset --hard pre-rebase &&\n+\t\ttest_must_fail git rebase$type main &&\n+\t\ttest_path_is_dir $state_dir &&\n+\t\thead_before=$(git rev-parse HEAD) &&\n+\t\tgit rebase --quit &&\n+\t\ttest_cmp_rev HEAD $head_before &&\n+\t\ttest_path_is_missing .git/rebase-apply\n+\t'\n }\n \n testrebase \" --apply\" .git/rebase-apply\n testrebase \" --merge\" .git/rebase-merge\n \n-test_expect_success 'rebase --apply --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --apply main &&\n-\ttest_path_is_dir .git/rebase-apply &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-apply\n-'\n-\n-test_expect_success 'rebase --merge --quit' '\n-\t# Clean up the state from the previous one\n-\tgit reset --hard pre-rebase &&\n-\ttest_must_fail git rebase --merge main &&\n-\ttest_path_is_dir .git/rebase-merge &&\n-\thead_before=$(git rev-parse HEAD) &&\n-\tgit rebase --quit &&\n-\ttest_cmp_rev HEAD $head_before &&\n-\ttest_path_is_missing .git/rebase-merge\n-'\n-\n test_done\n-- \ngitgitgadget\n\n"},{"id":"436590","messageId":"d3af62c746c18dd3124d1ec165d51a7aa627ab07.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 08/10] rebase: use our standard error return value","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:05Z","receivedAt":"2021-09-21T10:24:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nGit uses −1 to signal an error. The builtin rebase converts these to\n+1 all over the place using !! (presumably because the in the scripted\nversion an error was signalled by +1). This is confusing and clutters\nthe code, we only need to convert the value when the function returns.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6138009d6e4..444e169ef74 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1574,7 +1574,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t oid_to_hex(&options.orig_head));\n \t\tremove_branch_state(the_repository, 0);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n@@ -1583,11 +1583,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n \n \t\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n-\t\t\tret = !!sequencer_remove_state(&replay);\n+\t\t\tret = sequencer_remove_state(&replay);\n \t\t} else {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addstr(&buf, options.state_dir);\n-\t\t\tret = !!remove_dir_recursively(&buf, 0);\n+\t\t\tret = remove_dir_recursively(&buf, 0);\n \t\t\tif (ret)\n \t\t\t\terror(_(\"could not remove '%s'\"),\n \t\t\t\t options.state_dir);\n@@ -1959,7 +1959,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \tif (require_clean_work_tree(the_repository, \"rebase\",\n \t\t\t\t _(\"Please commit or stash them.\"), 1, 1)) {\n-\t\tret = 1;\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \n@@ -1994,7 +1994,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t\t\t\t NULL, buf.buf,\n \t\t\t\t\t DEFAULT_REFLOG_ACTION) < 0) {\n-\t\t\t\t\tret = !!error(_(\"could not switch to \"\n+\t\t\t\t\tret = error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n \t\t\t\t\t\t options.switch_to);\n \t\t\t\t\tgoto cleanup;\n@@ -2009,7 +2009,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tprintf(_(\"Current branch %s is up to date.\\n\"),\n \t\t\t\t branch_name);\n-\t\t\tret = !!finish_rebase(&options);\n+\t\t\tret = finish_rebase(&options);\n \t\t\tgoto cleanup;\n \t\t} else if (!(options.flags & REBASE_NO_QUIET))\n \t\t\t; /* be quiet */\n@@ -2087,7 +2087,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n \t\t\t DEFAULT_REFLOG_ACTION);\n \t\tstrbuf_release(&msg);\n-\t\tret = !!finish_rebase(&options);\n+\t\tret = finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \n@@ -2101,7 +2101,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \toptions.revisions = revisions.buf;\n \n run_rebase:\n-\tret = !!run_specific_rebase(&options, action);\n+\tret = run_specific_rebase(&options, action);\n \n cleanup:\n \tstrbuf_release(&buf);\n@@ -2112,5 +2112,5 @@ cleanup:\n \tfree(options.strategy);\n \tstrbuf_release(&options.git_format_patch_opt);\n \tfree(squash_onto_name);\n-\treturn ret;\n+\treturn !!ret;\n }\n-- \ngitgitgadget\n\n"},{"id":"436591","messageId":"4eedd3ae76614df5ac450a0cd901b05ea2e85c6d.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 09/10] rebase: use lookup_commit_reference_by_name()","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:06Z","receivedAt":"2021-09-21T10:24:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\npeel_committish() appears to have been copied from the scripted rebase\nbut it duplicates the functionality of\nlookup_commit_reference_by_name() so lets use that instead.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 17 ++++-------------\n 1 file changed, 4 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 444e169ef74..0ace9e0a8ec 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)\n \treturn ret;\n }\n \n-static struct commit *peel_committish(const char *name)\n-{\n-\tstruct object *obj;\n-\tstruct object_id oid;\n-\n-\tif (get_oid(name, &oid))\n-\t\treturn NULL;\n-\tobj = parse_object(the_repository, &oid);\n-\treturn (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);\n-}\n-\n static void add_var(struct strbuf *buf, const char *name, const char *value)\n {\n \tif (!value)\n@@ -1845,7 +1834,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (!strcmp(options.upstream_name, \"-\"))\n \t\t\t\toptions.upstream_name = \"@{-1}\";\n \t\t}\n-\t\toptions.upstream = peel_committish(options.upstream_name);\n+\t\toptions.upstream =\n+\t\t\tlookup_commit_reference_by_name(options.upstream_name);\n \t\tif (!options.upstream)\n \t\t\tdie(_(\"invalid upstream '%s'\"), options.upstream_name);\n \t\toptions.upstream_arg = options.upstream_name;\n@@ -1888,7 +1878,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\toptions.onto = lookup_commit_or_die(&merge_base,\n \t\t\t\t\t\t options.onto_name);\n \t} else {\n-\t\toptions.onto = peel_committish(options.onto_name);\n+\t\toptions.onto =\n+\t\t\tlookup_commit_reference_by_name(options.onto_name);\n \t\tif (!options.onto)\n \t\t\tdie(_(\"Does not point to a valid commit '%s'\"),\n \t\t\t\toptions.onto_name);\n-- \ngitgitgadget\n\n"},{"id":"436592","messageId":"55a6250ab38881ba56dfb6e65cf663bd2b631d5f.1632219848.git.gitgitgadget@gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"[PATCH v3 10/10] rebase: dereference tags","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-21T10:24:07Z","receivedAt":"2021-09-21T10:24:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nA rebase started with 'git rebase ' is conceptually to first\ncheckout and run 'git rebase ' starting from that state. 'git\nrebase --abort' in the middle of such a rebase should take us back to\nthe state we checked out .\n\nThis used to work, even when is a tag that points at a commit,\nuntil Git 2.20.0 when the command was reimplemented in C. The command\nnow complains that the tag object itself cannot be checked out, which\nmay be technically correct but is not what the user asked to do.\n\nFix this old regression by using lookup_commit_reference_by_name()\nwhen parsing . The scripted version did not need to peel the tag\nbecause the commands it passed the tag to (e.g 'git reset') peeled the\ntag themselves.\n\nSigned-off-by: Phillip Wood \n---\n---\n builtin/rebase.c | 14 ++++++++------\n t/t3407-rebase-abort.sh | 18 ++++++++++++++----\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0ace9e0a8ec..b4433ee7978 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1904,13 +1904,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie_if_checked_out(buf.buf, 1);\n \t\t\toptions.head_name = xstrdup(buf.buf);\n \t\t/* If not is it a valid ref (branch or commit)? */\n-\t\t} else if (!get_oid(branch_name, &options.orig_head) &&\n-\t\t\t lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t &options.orig_head))\n+\t\t} else {\n+\t\t\tstruct commit *commit =\n+\t\t\t\tlookup_commit_reference_by_name(branch_name);\n+\t\t\tif (!commit)\n+\t\t\t\tdie(_(\"no such branch/commit '%s'\"),\n+\t\t\t\t branch_name);\n+\t\t\toidcpy(&options.orig_head, &commit->object.oid);\n \t\t\toptions.head_name = NULL;\n-\t\telse\n-\t\t\tdie(_(\"no such branch/commit '%s'\"),\n-\t\t\t branch_name);\n+\t\t}\n \t} else if (argc == 0) {\n \t\t/* Do not need to switch branches, we are already on it. */\n \t\toptions.head_name =\ndiff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh\nindex 162112ba5ea..ebbaed147a6 100755\n--- a/t/t3407-rebase-abort.sh\n+++ b/t/t3407-rebase-abort.sh\n@@ -11,18 +11,18 @@ test_expect_success setup '\n \ttest_commit a a a &&\n \tgit branch to-rebase &&\n \n-\ttest_commit b a b &&\n-\ttest_commit c a c &&\n+\ttest_commit --annotate b a b &&\n+\ttest_commit --annotate c a c &&\n \n \tgit checkout to-rebase &&\n \ttest_commit \"merge should fail on this\" a d d &&\n-\ttest_commit \"merge should fail on this, too\" a e pre-rebase\n+\ttest_commit --annotate \"merge should fail on this, too\" a e pre-rebase\n '\n \n # Check that HEAD is equal to \"pre-rebase\" and the current branch is\n # \"to-rebase\"\n check_head() {\n-\ttest_cmp_rev HEAD pre-rebase &&\n+\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/to-rebase\n }\n \n@@ -67,6 +67,16 @@ testrebase() {\n \t\ttest_path_is_missing \"$state_dir\"\n \t'\n \n+\ttest_expect_success \"rebase$type --abort when checking out a tag\" '\n+\t\ttest_when_finished \"git symbolic-ref HEAD refs/heads/to-rebase\" &&\n+\t\tgit reset --hard a -- &&\n+\t\ttest_must_fail git rebase$type --onto b c pre-rebase &&\n+\t\ttest_cmp_rev HEAD b^{commit} &&\n+\t\tgit rebase --abort &&\n+\t\ttest_cmp_rev HEAD pre-rebase^{commit} &&\n+\t\t! git symbolic-ref HEAD\n+\t'\n+\n \ttest_expect_success \"rebase$type --abort does not update reflog\" '\n \t\t# Clean up the state from the previous one\n \t\tgit reset --hard pre-rebase &&\n-- \ngitgitgadget\n"},{"id":"436912","messageId":"CABPp-BHznKM_Y8cz55aP2xtB4a6XEgpFK2USOU=z32EwF6tgGw@mail.gmail.com","threadId":"56463","inReplyTo":"pull.1033.v3.git.1632219848.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 00/10] rebase: dereference tags","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-24T01:26:58Z","receivedAt":"2021-09-24T01:27:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 21, 2021 at 3:24 AM Phillip Wood via GitGitGadget\n wrote:\n>\n> Thanks for the comments on V2. Here are the changes from that version:\n>\n> * dropped the controversial cleanup that was patch 8\n> * reworded patch 10 as suggested by Junio\n>\n> Cover letter for v2: Thanks to Ævar and Johannes for their comments.\n\nThis version looks good to me.\n\n>\n> * Changed \"! test_cmp_rev\" to \"test_cmp_rev !\" (suggested by Ævar)\n> * Fixed the quoting for the title of the \"rebase --quit\" tests.\n> * Reworked the last commit to handle the error case first (suggested by\n> Ævar)\n> * Tweaked the commit messages for patches 8 & 11\n> * Rebased onto 31e4a0db03 (\"Merge branch 'ab/rebase-fatal-fatal-fix'\",\n> 2021-09-08) to avoid a merge conflict that upset gitgitgadget\n>\n> Cover letter for V1: Aborting a rebase stated with git rebase \n> should checkout the commit pointed to by . Instead it gives\n>\n> error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n>\n>\n> The fix for that is in the last patch, the rest of the patches are cleanups\n> to t3407 and builtin/rebase.c\n>\n> Phillip Wood (10):\n> t3407: run tests in $TEST_DIRECTORY\n> t3407: use test_commit\n> t3407: use test_cmp_rev\n> t3407: rename a variable\n> t3407: use test_path_is_missing\n> t3407: strengthen rebase --abort tests\n> t3407: rework rebase --quit tests\n> rebase: use our standard error return value\n> rebase: use lookup_commit_reference_by_name()\n> rebase: dereference tags\n>\n> builtin/rebase.c | 49 ++++++++-----------\n> t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------\n> 2 files changed, 67 insertions(+), 87 deletions(-)\n>\n>\n> base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1033\n>\n> Range-diff vs v2:\n>\n> 1: bac009d8543 = 1: bac009d8543 t3407: run tests in $TEST_DIRECTORY\n> 2: abfffb31a56 = 2: abfffb31a56 t3407: use test_commit\n> 3: 7755ce17fef = 3: 7755ce17fef t3407: use test_cmp_rev\n> 4: 38eee11baf5 = 4: 38eee11baf5 t3407: rename a variable\n> 5: 61a37c89f1e = 5: 61a37c89f1e t3407: use test_path_is_missing\n> 6: 6866630528b = 6: 6866630528b t3407: strengthen rebase --abort tests\n> 7: fd55a3196b1 = 7: fd55a3196b1 t3407: rework rebase --quit tests\n> 8: ad3c4efc027 < -: ----------- rebase: remove redundant strbuf\n> 9: ad940b633d0 ! 8: d3af62c746c rebase: use our standard error return value\n> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix\n> } else if (!(options.flags & REBASE_NO_QUIET))\n> ; /* be quiet */\n> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n> - reset_head(the_repository, NULL, \"Fast-forwarded\", options.head_name,\n> - RESET_HEAD_REFS_ONLY, \"HEAD\", buf.buf,\n> + RESET_HEAD_REFS_ONLY, \"HEAD\", msg.buf,\n> DEFAULT_REFLOG_ACTION);\n> + strbuf_release(&msg);\n> - ret = !!finish_rebase(&options);\n> + ret = finish_rebase(&options);\n> goto cleanup;\n> 10: bc103e703e8 = 9: 4eedd3ae766 rebase: use lookup_commit_reference_by_name()\n> 11: 951de6bb199 ! 10: 55a6250ab38 rebase: dereference tags\n> @@ Metadata\n> ## Commit message ##\n> rebase: dereference tags\n>\n> - Aborting a rebase stated with 'git rebase '\n> - should checkout the commit pointed to by . Instead it gives\n> + A rebase started with 'git rebase ' is conceptually to first\n> + checkout and run 'git rebase ' starting from that state. 'git\n> + rebase --abort' in the middle of such a rebase should take us back to\n> + the state we checked out .\n>\n> - error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':\n> - trying to write non-commit object\n> - 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'\n> + This used to work, even when is a tag that points at a commit,\n> + until Git 2.20.0 when the command was reimplemented in C. The command\n> + now complains that the tag object itself cannot be checked out, which\n> + may be technically correct but is not what the user asked to do.\n>\n> - This is because when we parse the command line arguments although we\n> - check that the tag points to a commit we remember the oid of the tag\n> - and try and checkout that object rather than the commit it points\n> - to. Fix this by using lookup_commit_reference_by_name() when parsing\n> - the command line.\n> + Fix this old regression by using lookup_commit_reference_by_name()\n> + when parsing . The scripted version did not need to peel the tag\n> + because the commands it passed the tag to (e.g 'git reset') peeled the\n> + tag themselves.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> --\n> gitgitgadget\n"}]}