All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/11] rebase: dereference tags
@ 2021-09-08  9:49 Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
                   ` (11 more replies)
  0 siblings, 12 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood

Aborting a rebase stated with git rebase <upstream> <tag-object> should
checkout the commit pointed to by . Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'


The fix for that is in the last patch, the rest of the patches are cleanups
to t3407 and builtin/rebase.c

Phillip Wood (11):
  t3407: run tests in $TEST_DIRECTORY
  t3407: use test_commit
  t3407: use test_cmp_rev
  t3407: rename a variable
  t3407: use test_path_is_missing
  t3407: strengthen rebase --abort tests
  t3407: rework rebase --quit tests
  rebase: remove redundant strbuf
  rebase: use our standard error return value
  rebase: use lookup_commit_reference_by_name()
  rebase: dereference tags

 builtin/rebase.c        |  67 ++++++++++++-------------
 t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
 2 files changed, 76 insertions(+), 96 deletions(-)


base-commit: 225bc32a989d7a22fa6addafd4ce7dcd04675dbf
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1033
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08 10:41   ` Ævar Arnfjörð Bjarmason
  2021-09-08  9:49 ` [PATCH 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
                   ` (10 subsequent siblings)
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when
path contains whitespace", 2008-05-04) started running these tests in
a subdirectory with a space in its name. At that time $TEST_DIRECTORY
did not contain a space but shortly after in 4a7aaccd83 ("Rename the
test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY
was changed to contain a space so we no longer need to run these tests
in a subdirectory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7c381fbc89a..c747bd31d76 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 
-### Test that we handle space characters properly
-work_dir="$(pwd)/test dir"
-
 test_expect_success setup '
-	mkdir -p "$work_dir" &&
-	cd "$work_dir" &&
-	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() {
 	dotest=$2
 
 	test_expect_success "rebase$type --abort" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() {
 	'
 
 	test_expect_success 'rebase --abort can not be used with other options' '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
 test_expect_success 'rebase --apply --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '
 '
 
 test_expect_success 'rebase --merge --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --merge main &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 02/11] t3407: use test_commit
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08 10:39   ` Ævar Arnfjörð Bjarmason
  2021-09-08  9:49 ` [PATCH 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
                   ` (9 subsequent siblings)
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Simplify the setup by using test_commit. Note that this replaces the
branch "pre-rebase" with a tag of the same name. "pre-rebase" is only
used as a fixed reference point so it makes sense to use a tag rather
than a branch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index c747bd31d76..0ad21966bc5 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo a > a &&
-	git add a &&
-	git commit -m a &&
+	test_commit a a a &&
 	git branch to-rebase &&
 
-	echo b > a &&
-	git commit -a -m b &&
-	echo c > a &&
-	git commit -a -m c &&
+	test_commit b a b &&
+	test_commit c a c &&
 
 	git checkout to-rebase &&
-	echo d > a &&
-	git commit -a -m "merge should fail on this" &&
-	echo e > a &&
-	git commit -a -m "merge should fail on this, too" &&
-	git branch pre-rebase
+	test_commit "merge should fail on this" a d d &&
+	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
 testrebase() {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 03/11] t3407: use test_cmp_rev
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08 10:40   ` Ævar Arnfjörð Bjarmason
  2021-09-08  9:49 ` [PATCH 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
                   ` (8 subsequent siblings)
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This provides better diagnostics if a test fails

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 0ad21966bc5..8d913d73bad 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -29,7 +29,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -39,9 +39,9 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
-		test $(git rev-parse HEAD) = $(git rev-parse main) &&
+		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -54,9 +54,9 @@ testrebase() {
 		echo d >> a &&
 		git add a &&
 		test_must_fail git rebase --continue &&
-		test $(git rev-parse HEAD) != $(git rev-parse main) &&
+		! test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '
 	test_path_is_dir .git/rebase-apply &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-apply
 '
 
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '
 	test_path_is_dir .git/rebase-merge &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-merge
 '
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 04/11] t3407: rename a variable
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (2 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

$dotest holds the name of the rebase state directory and was named
after the state directory used at the time the test was added. As we
no longer use that name rename the variable to reflect its purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 8d913d73bad..d36a3f2758b 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -21,35 +21,35 @@ test_expect_success setup '
 
 testrebase() {
 	type=$1
-	dotest=$2
+	state_dir=$2
 
 	test_expect_success "rebase$type --abort" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
@@ -57,7 +57,7 @@ testrebase() {
 		! test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 05/11] t3407: use test_path_is_missing
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (3 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the end of the test we expect the state directory to be missing,
but the tests only check it is not a directory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index d36a3f2758b..624d2ee3fa4 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -30,7 +30,7 @@ testrebase() {
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() {
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() {
 		! test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-apply
+	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 06/11] t3407: strengthen rebase --abort tests
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (4 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08 10:42   ` Ævar Arnfjörð Bjarmason
  2021-09-08  9:49 ` [PATCH 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The existing tests only check that HEAD points to the correct
commit after aborting, they do not check that the original branch
is checked out.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 624d2ee3fa4..72cf391156d 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -19,6 +19,13 @@ test_expect_success setup '
 	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
+# Check that HEAD is equal to "pre-rebase" and the current branch is
+# "to-rebase"
+check_head() {
+	test_cmp_rev HEAD pre-rebase &&
+	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
+}
+
 testrebase() {
 	type=$1
 	state_dir=$2
@@ -29,7 +36,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -41,7 +48,7 @@ testrebase() {
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -56,7 +63,7 @@ testrebase() {
 		test_must_fail git rebase --continue &&
 		! test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 07/11] t3407: rework rebase --quit tests
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (5 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

9512177b68 ("rebase: add --quit to cleanup rebase, leave everything
else untouched", 2016-11-12) seems to have copied the --abort tests
but added two separate tests for the two rebase backends rather than
adding a single test into the existing testrebase() function.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 72cf391156d..2f41b06e028 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -86,31 +86,21 @@ testrebase() {
 		test_must_fail git rebase --abort -v &&
 		git rebase --abort
 	'
+
+	test_expect_success 'rebase$type --quit' '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		# Clean up the state from the previous one
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type main &&
+		test_path_is_dir $state_dir &&
+		head_before=$(git rev-parse HEAD) &&
+		git rebase --quit &&
+		test_cmp_rev HEAD $head_before &&
+		test_path_is_missing .git/rebase-apply
+	'
 }
 
 testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
-test_expect_success 'rebase --apply --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --apply main &&
-	test_path_is_dir .git/rebase-apply &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-apply
-'
-
-test_expect_success 'rebase --merge --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --merge main &&
-	test_path_is_dir .git/rebase-merge &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-merge
-'
-
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 08/11] rebase: remove redundant strbuf
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (6 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-09 10:35   ` Johannes Schindelin
  2021-09-08  9:49 ` [PATCH 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

There is already an strbuf that can be reused for creating messages.
msg is not freed if there is an error and there is a logic error where
we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
strbuf_addf(&msg).

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 33e09619005..48ab7d9ae3b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	int ret, flags, total_argc, in_progress = 0;
 	int keep_base = 0;
 	int ok_to_skip_pre_rebase = 0;
-	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		printf(_("First, rewinding head to replay your work on top of "
 			 "it...\n"));
 
-	strbuf_addf(&msg, "%s: checkout %s",
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
 		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))
+		       NULL, buf.buf, DEFAULT_REFLOG_ACTION))
 		die(_("Could not detach HEAD"));
-	strbuf_release(&msg);
 
 	/*
 	 * If the onto is a proper descendant of the tip of the branch, then
 	 * we just fast-forwarded.
 	 */
-	strbuf_reset(&msg);
+	strbuf_reset(&buf);
 	if (oideq(&merge_base, &options.orig_head)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
-		strbuf_addf(&msg, "rebase finished: %s onto %s",
+		strbuf_addf(&buf, "rebase finished: %s onto %s",
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
 		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
-			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
+			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
 			   DEFAULT_REFLOG_ACTION);
-		strbuf_release(&msg);
 		ret = !!finish_rebase(&options);
 		goto cleanup;
 	}
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 09/11] rebase: use our standard error return value
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (7 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Git uses −1 to signal an error. The builtin rebase converts these to
+1 all over the place using !! (presumably because the in the scripted
version an error was signalled by +1). This is confusing and clutters
the code, we only need to convert the value when the function returns.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 48ab7d9ae3b..e8c3c77bab0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
 		remove_branch_state(the_repository, 0);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 	case ACTION_QUIT: {
@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			struct replay_opts replay = REPLAY_OPTS_INIT;
 
 			replay.action = REPLAY_INTERACTIVE_REBASE;
-			ret = !!sequencer_remove_state(&replay);
+			ret = sequencer_remove_state(&replay);
 		} else {
 			strbuf_reset(&buf);
 			strbuf_addstr(&buf, options.state_dir);
-			ret = !!remove_dir_recursively(&buf, 0);
+			ret = remove_dir_recursively(&buf, 0);
 			if (ret)
 				error(_("could not remove '%s'"),
 				       options.state_dir);
@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	if (require_clean_work_tree(the_repository, "rebase",
 				    _("Please commit or stash them."), 1, 1)) {
-		ret = 1;
+		ret = -1;
 		goto cleanup;
 	}
 
@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 					       NULL, buf.buf,
 					       DEFAULT_REFLOG_ACTION) < 0) {
-					ret = !!error(_("could not switch to "
+					ret = error(_("could not switch to "
 							"%s"),
 						      options.switch_to);
 					goto cleanup;
@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			else
 				printf(_("Current branch %s is up to date.\n"),
 				       branch_name);
-			ret = !!finish_rebase(&options);
+			ret = finish_rebase(&options);
 			goto cleanup;
 		} else if (!(options.flags & REBASE_NO_QUIET))
 			; /* be quiet */
@@ -2085,7 +2085,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
 			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
 			   DEFAULT_REFLOG_ACTION);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 
@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = !!run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options, action);
 
 cleanup:
 	strbuf_release(&buf);
@@ -2110,5 +2110,5 @@ cleanup:
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
-	return ret;
+	return !!ret;
 }
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 10/11] rebase: use lookup_commit_reference_by_name()
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (8 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08  9:49 ` [PATCH 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
  11 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

peel_committish() appears to have been copied from the scripted rebase
but it duplicates the functionality of
lookup_commit_reference_by_name() so lets use that instead.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e8c3c77bab0..93fcc0df2ad 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)
 	return ret;
 }
 
-static struct commit *peel_committish(const char *name)
-{
-	struct object *obj;
-	struct object_id oid;
-
-	if (get_oid(name, &oid))
-		return NULL;
-	obj = parse_object(the_repository, &oid);
-	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
-}
-
 static void add_var(struct strbuf *buf, const char *name, const char *value)
 {
 	if (!value)
@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (!strcmp(options.upstream_name, "-"))
 				options.upstream_name = "@{-1}";
 		}
-		options.upstream = peel_committish(options.upstream_name);
+		options.upstream =
+			lookup_commit_reference_by_name(options.upstream_name);
 		if (!options.upstream)
 			die(_("invalid upstream '%s'"), options.upstream_name);
 		options.upstream_arg = options.upstream_name;
@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.onto = lookup_commit_or_die(&merge_base,
 						    options.onto_name);
 	} else {
-		options.onto = peel_committish(options.onto_name);
+		options.onto =
+			lookup_commit_reference_by_name(options.onto_name);
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH 11/11] rebase: dereference tags
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (9 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
@ 2021-09-08  9:49 ` Phillip Wood via GitGitGadget
  2021-09-08 10:45   ` Ævar Arnfjörð Bjarmason
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
  11 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-08  9:49 UTC (permalink / raw)
  To: git; +Cc: Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
should checkout the commit pointed to by <tag-object>. Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
    trying to write non-commit object
    710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'

This is because when we parse the command line arguments although we
check that the tag points to a commit we remember the oid of the tag
and try and checkout that object rather than the commit it points
to. Fix this by using lookup_commit_reference_by_name() when parsing
the command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
---
 builtin/rebase.c        | 18 +++++++++++-------
 t/t3407-rebase-abort.sh | 18 ++++++++++++++----
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 93fcc0df2ad..8bf7660a24b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
-		} else if (!get_oid(branch_name, &options.orig_head) &&
-			   lookup_commit_reference(the_repository,
-						   &options.orig_head))
-			options.head_name = NULL;
-		else
-			die(_("fatal: no such branch/commit '%s'"),
-			    branch_name);
+		} else {
+			struct commit *commit =
+				lookup_commit_reference_by_name(branch_name);
+			if (commit) {
+				oidcpy(&options.orig_head, &commit->object.oid);
+				options.head_name = NULL;
+			} else {
+				die(_("fatal: no such branch/commit '%s'"),
+				    branch_name);
+			}
+		}
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */
 		options.head_name =
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2f41b06e028..310cd0c736c 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -11,18 +11,18 @@ test_expect_success setup '
 	test_commit a a a &&
 	git branch to-rebase &&
 
-	test_commit b a b &&
-	test_commit c a c &&
+	test_commit --annotate b a b &&
+	test_commit --annotate c a c &&
 
 	git checkout to-rebase &&
 	test_commit "merge should fail on this" a d d &&
-	test_commit "merge should fail on this, too" a e pre-rebase
+	test_commit --annotate "merge should fail on this, too" a e pre-rebase
 '
 
 # Check that HEAD is equal to "pre-rebase" and the current branch is
 # "to-rebase"
 check_head() {
-	test_cmp_rev HEAD pre-rebase &&
+	test_cmp_rev HEAD pre-rebase^{commit} &&
 	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
 }
 
@@ -67,6 +67,16 @@ testrebase() {
 		test_path_is_missing "$state_dir"
 	'
 
+	test_expect_success "rebase$type --abort when checking out a tag" '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		git reset --hard a -- &&
+		test_must_fail git rebase$type --onto b c pre-rebase &&
+		test_cmp_rev HEAD b^{commit} &&
+		git rebase --abort &&
+		test_cmp_rev HEAD pre-rebase^{commit} &&
+		! git symbolic-ref HEAD
+	'
+
 	test_expect_success "rebase$type --abort does not update reflog" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* Re: [PATCH 02/11] t3407: use test_commit
  2021-09-08  9:49 ` [PATCH 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-08 10:39   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 55+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-08 10:39 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood


On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Simplify the setup by using test_commit. Note that this replaces the
> branch "pre-rebase" with a tag of the same name. "pre-rebase" is only
> used as a fixed reference point so it makes sense to use a tag rather
> than a branch.

FWIW you could use --no-tag to test_commit to keep the behavior, but
changing it here seems fine.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH 03/11] t3407: use test_cmp_rev
  2021-09-08  9:49 ` [PATCH 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-08 10:40   ` Ævar Arnfjörð Bjarmason
  2021-09-08 13:42     ` Phillip Wood
  0 siblings, 1 reply; 55+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-08 10:40 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood


On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:

> @@ -54,9 +54,9 @@ testrebase() {
>  		echo d >> a &&
>  		git add a &&
>  		test_must_fail git rebase --continue &&
> -		test $(git rev-parse HEAD) != $(git rev-parse main) &&
> +		! test_cmp_rev HEAD main &&

Use "test_cmp_rev !", making the shell do the negation is troublesome
for the same reason we don't do it with git command, it potentially
hides segfaults (and that test helper invokes "git", hence the optional
"!" first rgument).

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY
  2021-09-08  9:49 ` [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-08 10:41   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 55+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-08 10:41 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood


On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when
> path contains whitespace", 2008-05-04) started running these tests in
> a subdirectory with a space in its name. At that time $TEST_DIRECTORY
> did not contain a space but shortly after in 4a7aaccd83 ("Rename the
> test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY
> was changed to contain a space so we no longer need to run these tests
> in a subdirectory.

Well spotted & a nice cleanup!

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH 06/11] t3407: strengthen rebase --abort tests
  2021-09-08  9:49 ` [PATCH 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-08 10:42   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 55+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-08 10:42 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood


On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:

> The existing tests only check that HEAD points to the correct
> commit after aborting, they do not check that the original branch
> is checked out.
> [...]
> +# Check that HEAD is equal to "pre-rebase" and the current branch is
> +# "to-rebase"
> +check_head() {
> +	test_cmp_rev HEAD pre-rebase &&
> +	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
> +}

I reflexively thought "use test_cmp here!", but in this case that's
pointless as any difference is noted by test_cmp_rev, we're really
seeing what HEAD is checked out to here. I see we use the same pattern
in various other tests...

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH 11/11] rebase: dereference tags
  2021-09-08  9:49 ` [PATCH 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-08 10:45   ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 55+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-08 10:45 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood


On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
> should checkout the commit pointed to by <tag-object>. Instead it gives
>
>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
>     trying to write non-commit object
>     710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
> This is because when we parse the command line arguments although we
> check that the tag points to a commit we remember the oid of the tag
> and try and checkout that object rather than the commit it points
> to. Fix this by using lookup_commit_reference_by_name() when parsing
> the command line.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> ---
>  builtin/rebase.c        | 18 +++++++++++-------
>  t/t3407-rebase-abort.sh | 18 ++++++++++++++----
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 93fcc0df2ad..8bf7660a24b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1903,13 +1903,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die_if_checked_out(buf.buf, 1);
>  			options.head_name = xstrdup(buf.buf);
>  		/* If not is it a valid ref (branch or commit)? */
> -		} else if (!get_oid(branch_name, &options.orig_head) &&
> -			   lookup_commit_reference(the_repository,
> -						   &options.orig_head))
> -			options.head_name = NULL;
> -		else
> -			die(_("fatal: no such branch/commit '%s'"),
> -			    branch_name);
> +		} else {
> +			struct commit *commit =
> +				lookup_commit_reference_by_name(branch_name);
> +			if (commit) {
> +				oidcpy(&options.orig_head, &commit->object.oid);
> +				options.head_name = NULL;
> +			} else {
> +				die(_("fatal: no such branch/commit '%s'"),
> +				    branch_name);
> +			}
> +		}

Suggested style nit:

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8bf7660a24b..c751ef866fd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1906,13 +1906,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		} else {
 			struct commit *commit =
 				lookup_commit_reference_by_name(branch_name);
-			if (commit) {
-				oidcpy(&options.orig_head, &commit->object.oid);
-				options.head_name = NULL;
-			} else {
+			if (!commit)
 				die(_("fatal: no such branch/commit '%s'"),
 				    branch_name);
-			}
+
+			oidcpy(&options.orig_head, &commit->object.oid);
+			options.head_name = NULL;
 		}
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */

I.e. handle the "die" case right away & skip the indenting of the
non-assert code.

(Also grepping around we could really use a fatal non-gentle version of
lookup_commit_reference_by_name(), but that's another more general
cleanup...)

>  	} else if (argc == 0) {
>  		/* Do not need to switch branches, we are already on it. */
>  		options.head_name =
> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
> index 2f41b06e028..310cd0c736c 100755
> --- a/t/t3407-rebase-abort.sh
> +++ b/t/t3407-rebase-abort.sh
> @@ -11,18 +11,18 @@ test_expect_success setup '
>  	test_commit a a a &&
>  	git branch to-rebase &&
>  
> -	test_commit b a b &&
> -	test_commit c a c &&
> +	test_commit --annotate b a b &&
> +	test_commit --annotate c a c &&
>  
>  	git checkout to-rebase &&
>  	test_commit "merge should fail on this" a d d &&
> -	test_commit "merge should fail on this, too" a e pre-rebase
> +	test_commit --annotate "merge should fail on this, too" a e pre-rebase
>  '
>  
>  # Check that HEAD is equal to "pre-rebase" and the current branch is
>  # "to-rebase"
>  check_head() {
> -	test_cmp_rev HEAD pre-rebase &&
> +	test_cmp_rev HEAD pre-rebase^{commit} &&
>  	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
>  }
>  
> @@ -67,6 +67,16 @@ testrebase() {
>  		test_path_is_missing "$state_dir"
>  	'
>  
> +	test_expect_success "rebase$type --abort when checking out a tag" '
> +		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
> +		git reset --hard a -- &&
> +		test_must_fail git rebase$type --onto b c pre-rebase &&
> +		test_cmp_rev HEAD b^{commit} &&
> +		git rebase --abort &&
> +		test_cmp_rev HEAD pre-rebase^{commit} &&
> +		! git symbolic-ref HEAD
> +	'
> +
>  	test_expect_success "rebase$type --abort does not update reflog" '
>  		# Clean up the state from the previous one
>  		git reset --hard pre-rebase &&

This whole series looks good to me, left some comments on other
patches. Consider the above suggested squash highly optional :)

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* Re: [PATCH 03/11] t3407: use test_cmp_rev
  2021-09-08 10:40   ` Ævar Arnfjörð Bjarmason
@ 2021-09-08 13:42     ` Phillip Wood
  0 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-08 13:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Phillip Wood via GitGitGadget
  Cc: git, Phillip Wood

On 08/09/2021 11:40, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Sep 08 2021, Phillip Wood via GitGitGadget wrote:
> 
>> @@ -54,9 +54,9 @@ testrebase() {
>>   		echo d >> a &&
>>   		git add a &&
>>   		test_must_fail git rebase --continue &&
>> -		test $(git rev-parse HEAD) != $(git rev-parse main) &&
>> +		! test_cmp_rev HEAD main &&
> 
> Use "test_cmp_rev !", making the shell do the negation is troublesome
> for the same reason we don't do it with git command, it potentially
> hides segfaults (and that test helper invokes "git", hence the optional
> "!" first argument).

Thanks, I didn't realize one could pass "!" to test_cmp_rev - it gives a 
better message on failure as well as avoiding hidden segfaults

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH 08/11] rebase: remove redundant strbuf
  2021-09-08  9:49 ` [PATCH 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-09 10:35   ` Johannes Schindelin
  0 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin @ 2021-09-09 10:35 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood, Phillip Wood

Hi Phillip,

On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> There is already an strbuf that can be reused for creating messages.
> msg is not freed if there is an error and there is a logic error where
> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
> strbuf_addf(&msg).

In some instances, we use a `buf` variable to construct a string that is
then assigned to a `const char *` and is used elsewhere throughout the
function. If this was the case in the code you modified, that would be a
problem.

I did verify manually that this is not the case, though, so this patch is
good to go.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [PATCH v2 00/11] rebase: dereference tags
  2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
                   ` (10 preceding siblings ...)
  2021-09-08  9:49 ` [PATCH 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-13 15:19 ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
                     ` (12 more replies)
  11 siblings, 13 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood

Thanks to Ævar and Johannes for their comments.

 * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
 * Fixed the quoting for the title of the "rebase --quit" tests.
 * Reworked the last commit to handle the error case first (suggested by
   Ævar)
 * Tweaked the commit messages for patches 8 & 11
 * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
   2021-09-08) to avoid a merge conflict that upset gitgitgadget

Cover letter for V1:

Aborting a rebase stated with git rebase <upstream> <tag-object> should
checkout the commit pointed to by . Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'


The fix for that is in the last patch, the rest of the patches are cleanups
to t3407 and builtin/rebase.c

Phillip Wood (11):
  t3407: run tests in $TEST_DIRECTORY
  t3407: use test_commit
  t3407: use test_cmp_rev
  t3407: rename a variable
  t3407: use test_path_is_missing
  t3407: strengthen rebase --abort tests
  t3407: rework rebase --quit tests
  rebase: remove redundant strbuf
  rebase: use our standard error return value
  rebase: use lookup_commit_reference_by_name()
  rebase: dereference tags

 builtin/rebase.c        |  63 +++++++++++-------------
 t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
 2 files changed, 73 insertions(+), 95 deletions(-)


base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1033

Range-diff vs v1:

  1:  0f5992e8cab =  1:  bac009d8543 t3407: run tests in $TEST_DIRECTORY
  2:  79b6dec910e =  2:  abfffb31a56 t3407: use test_commit
  3:  dfa27d7a8e7 !  3:  7755ce17fef t3407: use test_cmp_rev
     @@ t/t3407-rebase-abort.sh: testrebase() {
       		git add a &&
       		test_must_fail git rebase --continue &&
      -		test $(git rev-parse HEAD) != $(git rev-parse main) &&
     -+		! test_cmp_rev HEAD main &&
     ++		test_cmp_rev ! HEAD main &&
       		git rebase --abort &&
      -		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
      +		test_cmp_rev to-rebase pre-rebase &&
  4:  bef70d86d53 !  4:  38eee11baf5 t3407: rename a variable
     @@ t/t3407-rebase-abort.sh: test_expect_success setup '
       		echo d >> a &&
       		git add a &&
      @@ t/t3407-rebase-abort.sh: testrebase() {
     - 		! test_cmp_rev HEAD main &&
     + 		test_cmp_rev ! HEAD main &&
       		git rebase --abort &&
       		test_cmp_rev to-rebase pre-rebase &&
      -		test ! -d "$dotest"
  5:  d9376fe0818 !  5:  61a37c89f1e t3407: use test_path_is_missing
     @@ t/t3407-rebase-abort.sh: testrebase() {
       
       	test_expect_success "rebase$type --abort after --continue" '
      @@ t/t3407-rebase-abort.sh: testrebase() {
     - 		! test_cmp_rev HEAD main &&
     + 		test_cmp_rev ! HEAD main &&
       		git rebase --abort &&
       		test_cmp_rev to-rebase pre-rebase &&
      -		test ! -d "$state_dir"
  6:  87d7e9bf2d4 !  6:  6866630528b t3407: strengthen rebase --abort tests
     @@ t/t3407-rebase-abort.sh: testrebase() {
       
      @@ t/t3407-rebase-abort.sh: testrebase() {
       		test_must_fail git rebase --continue &&
     - 		! test_cmp_rev HEAD main &&
     + 		test_cmp_rev ! HEAD main &&
       		git rebase --abort &&
      -		test_cmp_rev to-rebase pre-rebase &&
      +		check_head &&
  7:  9a4f6ea73c5 !  7:  fd55a3196b1 t3407: rework rebase --quit tests
     @@ t/t3407-rebase-abort.sh: testrebase() {
       		git rebase --abort
       	'
      +
     -+	test_expect_success 'rebase$type --quit' '
     ++	test_expect_success "rebase$type --quit" '
      +		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
      +		# Clean up the state from the previous one
      +		git reset --hard pre-rebase &&
  8:  35b6c26c8f9 =  8:  ad3c4efc027 rebase: remove redundant strbuf
  9:  f0644cde725 =  9:  ad940b633d0 rebase: use our standard error return value
 10:  c537897006c = 10:  bc103e703e8 rebase: use lookup_commit_reference_by_name()
 11:  e87ce4fe253 ! 11:  951de6bb199 rebase: dereference tags
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
      -		} else if (!get_oid(branch_name, &options.orig_head) &&
      -			   lookup_commit_reference(the_repository,
      -						   &options.orig_head))
     --			options.head_name = NULL;
     --		else
     --			die(_("fatal: no such branch/commit '%s'"),
     --			    branch_name);
      +		} else {
      +			struct commit *commit =
      +				lookup_commit_reference_by_name(branch_name);
     -+			if (commit) {
     -+				oidcpy(&options.orig_head, &commit->object.oid);
     -+				options.head_name = NULL;
     -+			} else {
     -+				die(_("fatal: no such branch/commit '%s'"),
     ++			if (!commit)
     ++				die(_("no such branch/commit '%s'"),
      +				    branch_name);
     -+			}
     ++			oidcpy(&options.orig_head, &commit->object.oid);
     + 			options.head_name = NULL;
     +-		else
     +-			die(_("no such branch/commit '%s'"),
     +-			    branch_name);
      +		}
       	} else if (argc == 0) {
       		/* Do not need to switch branches, we are already on it. */

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
                     ` (11 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when
path contains whitespace", 2008-05-04) started running these tests in
a subdirectory with a space in its name. At that time $TEST_DIRECTORY
did not contain a space but shortly after in 4a7aaccd83 ("Rename the
test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY
was changed to contain a space so we no longer need to run these tests
in a subdirectory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7c381fbc89a..c747bd31d76 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 
-### Test that we handle space characters properly
-work_dir="$(pwd)/test dir"
-
 test_expect_success setup '
-	mkdir -p "$work_dir" &&
-	cd "$work_dir" &&
-	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() {
 	dotest=$2
 
 	test_expect_success "rebase$type --abort" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() {
 	'
 
 	test_expect_success 'rebase --abort can not be used with other options' '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
 test_expect_success 'rebase --apply --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '
 '
 
 test_expect_success 'rebase --merge --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --merge main &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 02/11] t3407: use test_commit
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
                     ` (10 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Simplify the setup by using test_commit. Note that this replaces the
branch "pre-rebase" with a tag of the same name. "pre-rebase" is only
used as a fixed reference point so it makes sense to use a tag rather
than a branch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index c747bd31d76..0ad21966bc5 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo a > a &&
-	git add a &&
-	git commit -m a &&
+	test_commit a a a &&
 	git branch to-rebase &&
 
-	echo b > a &&
-	git commit -a -m b &&
-	echo c > a &&
-	git commit -a -m c &&
+	test_commit b a b &&
+	test_commit c a c &&
 
 	git checkout to-rebase &&
-	echo d > a &&
-	git commit -a -m "merge should fail on this" &&
-	echo e > a &&
-	git commit -a -m "merge should fail on this, too" &&
-	git branch pre-rebase
+	test_commit "merge should fail on this" a d d &&
+	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
 testrebase() {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 03/11] t3407: use test_cmp_rev
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
                     ` (9 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This provides better diagnostics if a test fails

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 0ad21966bc5..39076ac9462 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -29,7 +29,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -39,9 +39,9 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
-		test $(git rev-parse HEAD) = $(git rev-parse main) &&
+		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -54,9 +54,9 @@ testrebase() {
 		echo d >> a &&
 		git add a &&
 		test_must_fail git rebase --continue &&
-		test $(git rev-parse HEAD) != $(git rev-parse main) &&
+		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '
 	test_path_is_dir .git/rebase-apply &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-apply
 '
 
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '
 	test_path_is_dir .git/rebase-merge &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-merge
 '
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 04/11] t3407: rename a variable
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (2 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
                     ` (8 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

$dotest holds the name of the rebase state directory and was named
after the state directory used at the time the test was added. As we
no longer use that name rename the variable to reflect its purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 39076ac9462..2c70230a4eb 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -21,35 +21,35 @@ test_expect_success setup '
 
 testrebase() {
 	type=$1
-	dotest=$2
+	state_dir=$2
 
 	test_expect_success "rebase$type --abort" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
@@ -57,7 +57,7 @@ testrebase() {
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 05/11] t3407: use test_path_is_missing
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (3 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
                     ` (7 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the end of the test we expect the state directory to be missing,
but the tests only check it is not a directory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2c70230a4eb..7eba9ec1619 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -30,7 +30,7 @@ testrebase() {
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() {
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() {
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-apply
+	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 06/11] t3407: strengthen rebase --abort tests
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (4 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
                     ` (6 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The existing tests only check that HEAD points to the correct
commit after aborting, they do not check that the original branch
is checked out.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7eba9ec1619..f8264449b6c 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -19,6 +19,13 @@ test_expect_success setup '
 	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
+# Check that HEAD is equal to "pre-rebase" and the current branch is
+# "to-rebase"
+check_head() {
+	test_cmp_rev HEAD pre-rebase &&
+	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
+}
+
 testrebase() {
 	type=$1
 	state_dir=$2
@@ -29,7 +36,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -41,7 +48,7 @@ testrebase() {
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -56,7 +63,7 @@ testrebase() {
 		test_must_fail git rebase --continue &&
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 07/11] t3407: rework rebase --quit tests
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (5 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
                     ` (5 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

9512177b68 ("rebase: add --quit to cleanup rebase, leave everything
else untouched", 2016-11-12) seems to have copied the --abort tests
but added two separate tests for the two rebase backends rather than
adding a single test into the existing testrebase() function.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index f8264449b6c..162112ba5ea 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -86,31 +86,21 @@ testrebase() {
 		test_must_fail git rebase --abort -v &&
 		git rebase --abort
 	'
+
+	test_expect_success "rebase$type --quit" '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		# Clean up the state from the previous one
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type main &&
+		test_path_is_dir $state_dir &&
+		head_before=$(git rev-parse HEAD) &&
+		git rebase --quit &&
+		test_cmp_rev HEAD $head_before &&
+		test_path_is_missing .git/rebase-apply
+	'
 }
 
 testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
-test_expect_success 'rebase --apply --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --apply main &&
-	test_path_is_dir .git/rebase-apply &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-apply
-'
-
-test_expect_success 'rebase --merge --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --merge main &&
-	test_path_is_dir .git/rebase-merge &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-merge
-'
-
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 08/11] rebase: remove redundant strbuf
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (6 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 18:34     ` René Scharfe
  2021-09-13 15:19   ` [PATCH v2 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
                     ` (4 subsequent siblings)
  12 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

There is already an strbuf that can be reused for creating messages.
msg is not freed if there is an error and there is a logic error where
we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
strbuf_addf(&msg).

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6138009d6e4..69a67ab1252 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	int ret, flags, total_argc, in_progress = 0;
 	int keep_base = 0;
 	int ok_to_skip_pre_rebase = 0;
-	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
@@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		printf(_("First, rewinding head to replay your work on top of "
 			 "it...\n"));
 
-	strbuf_addf(&msg, "%s: checkout %s",
+	strbuf_reset(&buf);
+	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
 		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
-		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))
+		       NULL, buf.buf, DEFAULT_REFLOG_ACTION))
 		die(_("Could not detach HEAD"));
-	strbuf_release(&msg);
 
 	/*
 	 * If the onto is a proper descendant of the tip of the branch, then
 	 * we just fast-forwarded.
 	 */
-	strbuf_reset(&msg);
+	strbuf_reset(&buf);
 	if (oideq(&merge_base, &options.orig_head)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
-		strbuf_addf(&msg, "rebase finished: %s onto %s",
+		strbuf_addf(&buf, "rebase finished: %s onto %s",
 			options.head_name ? options.head_name : "detached HEAD",
 			oid_to_hex(&options.onto->object.oid));
 		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
-			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
+			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
 			   DEFAULT_REFLOG_ACTION);
-		strbuf_release(&msg);
 		ret = !!finish_rebase(&options);
 		goto cleanup;
 	}
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 09/11] rebase: use our standard error return value
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (7 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
                     ` (3 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Git uses −1 to signal an error. The builtin rebase converts these to
+1 all over the place using !! (presumably because the in the scripted
version an error was signalled by +1). This is confusing and clutters
the code, we only need to convert the value when the function returns.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 69a67ab1252..7905672d8de 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1573,7 +1573,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
 		remove_branch_state(the_repository, 0);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 	case ACTION_QUIT: {
@@ -1582,11 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			struct replay_opts replay = REPLAY_OPTS_INIT;
 
 			replay.action = REPLAY_INTERACTIVE_REBASE;
-			ret = !!sequencer_remove_state(&replay);
+			ret = sequencer_remove_state(&replay);
 		} else {
 			strbuf_reset(&buf);
 			strbuf_addstr(&buf, options.state_dir);
-			ret = !!remove_dir_recursively(&buf, 0);
+			ret = remove_dir_recursively(&buf, 0);
 			if (ret)
 				error(_("could not remove '%s'"),
 				       options.state_dir);
@@ -1958,7 +1958,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	if (require_clean_work_tree(the_repository, "rebase",
 				    _("Please commit or stash them."), 1, 1)) {
-		ret = 1;
+		ret = -1;
 		goto cleanup;
 	}
 
@@ -1993,7 +1993,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 					       NULL, buf.buf,
 					       DEFAULT_REFLOG_ACTION) < 0) {
-					ret = !!error(_("could not switch to "
+					ret = error(_("could not switch to "
 							"%s"),
 						      options.switch_to);
 					goto cleanup;
@@ -2008,7 +2008,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			else
 				printf(_("Current branch %s is up to date.\n"),
 				       branch_name);
-			ret = !!finish_rebase(&options);
+			ret = finish_rebase(&options);
 			goto cleanup;
 		} else if (!(options.flags & REBASE_NO_QUIET))
 			; /* be quiet */
@@ -2085,7 +2085,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
 			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
 			   DEFAULT_REFLOG_ACTION);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 
@@ -2099,7 +2099,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = !!run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options, action);
 
 cleanup:
 	strbuf_release(&buf);
@@ -2110,5 +2110,5 @@ cleanup:
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
-	return ret;
+	return !!ret;
 }
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name()
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (8 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
                     ` (2 subsequent siblings)
  12 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

peel_committish() appears to have been copied from the scripted rebase
but it duplicates the functionality of
lookup_commit_reference_by_name() so lets use that instead.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7905672d8de..74663208468 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)
 	return ret;
 }
 
-static struct commit *peel_committish(const char *name)
-{
-	struct object *obj;
-	struct object_id oid;
-
-	if (get_oid(name, &oid))
-		return NULL;
-	obj = parse_object(the_repository, &oid);
-	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
-}
-
 static void add_var(struct strbuf *buf, const char *name, const char *value)
 {
 	if (!value)
@@ -1844,7 +1833,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (!strcmp(options.upstream_name, "-"))
 				options.upstream_name = "@{-1}";
 		}
-		options.upstream = peel_committish(options.upstream_name);
+		options.upstream =
+			lookup_commit_reference_by_name(options.upstream_name);
 		if (!options.upstream)
 			die(_("invalid upstream '%s'"), options.upstream_name);
 		options.upstream_arg = options.upstream_name;
@@ -1887,7 +1877,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.onto = lookup_commit_or_die(&merge_base,
 						    options.onto_name);
 	} else {
-		options.onto = peel_committish(options.onto_name);
+		options.onto =
+			lookup_commit_reference_by_name(options.onto_name);
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v2 11/11] rebase: dereference tags
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (9 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
@ 2021-09-13 15:19   ` Phillip Wood via GitGitGadget
  2021-09-13 22:58     ` Junio C Hamano
                       ` (2 more replies)
  2021-09-14  4:02   ` [PATCH v2 00/11] " Elijah Newren
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
  12 siblings, 3 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-13 15:19 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
should checkout the commit pointed to by <tag-object>. Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
    trying to write non-commit object
    710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'

This is because when we parse the command line arguments although we
check that the tag points to a commit we remember the oid of the tag
and try and checkout that object rather than the commit it points
to. Fix this by using lookup_commit_reference_by_name() when parsing
the command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
---
 builtin/rebase.c        | 14 ++++++++------
 t/t3407-rebase-abort.sh | 18 ++++++++++++++----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 74663208468..2b70a196f9a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
-		} else if (!get_oid(branch_name, &options.orig_head) &&
-			   lookup_commit_reference(the_repository,
-						   &options.orig_head))
+		} else {
+			struct commit *commit =
+				lookup_commit_reference_by_name(branch_name);
+			if (!commit)
+				die(_("no such branch/commit '%s'"),
+				    branch_name);
+			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
-		else
-			die(_("no such branch/commit '%s'"),
-			    branch_name);
+		}
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */
 		options.head_name =
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 162112ba5ea..ebbaed147a6 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -11,18 +11,18 @@ test_expect_success setup '
 	test_commit a a a &&
 	git branch to-rebase &&
 
-	test_commit b a b &&
-	test_commit c a c &&
+	test_commit --annotate b a b &&
+	test_commit --annotate c a c &&
 
 	git checkout to-rebase &&
 	test_commit "merge should fail on this" a d d &&
-	test_commit "merge should fail on this, too" a e pre-rebase
+	test_commit --annotate "merge should fail on this, too" a e pre-rebase
 '
 
 # Check that HEAD is equal to "pre-rebase" and the current branch is
 # "to-rebase"
 check_head() {
-	test_cmp_rev HEAD pre-rebase &&
+	test_cmp_rev HEAD pre-rebase^{commit} &&
 	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
 }
 
@@ -67,6 +67,16 @@ testrebase() {
 		test_path_is_missing "$state_dir"
 	'
 
+	test_expect_success "rebase$type --abort when checking out a tag" '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		git reset --hard a -- &&
+		test_must_fail git rebase$type --onto b c pre-rebase &&
+		test_cmp_rev HEAD b^{commit} &&
+		git rebase --abort &&
+		test_cmp_rev HEAD pre-rebase^{commit} &&
+		! git symbolic-ref HEAD
+	'
+
 	test_expect_success "rebase$type --abort does not update reflog" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
  2021-09-13 15:19   ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
@ 2021-09-13 18:34     ` René Scharfe
  2021-09-13 22:40       ` Junio C Hamano
  2021-09-14 10:33       ` Phillip Wood
  0 siblings, 2 replies; 55+ messages in thread
From: René Scharfe @ 2021-09-13 18:34 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget, git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood

Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> There is already an strbuf that can be reused for creating messages.

Reminds me of a terrible joke from elementary school: In Peter's class
everybody is called Klaus, except Franz -- his name is Michael.

Why would we want to use the same variable for multiple purposes?  This
makes the code harder to read.  And the allocation overhead for these
few cases should be negligible.

The most important question: Is this patch really needed to support
tags (the purpose of this series)?

> msg is not freed if there is an error and there is a logic error where
> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
> strbuf_addf(&msg).

strbuf_reset() after strbuf_release() is redundant but legal.

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6138009d6e4..69a67ab1252 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1299,7 +1299,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	int ret, flags, total_argc, in_progress = 0;
>  	int keep_base = 0;
>  	int ok_to_skip_pre_rebase = 0;
> -	struct strbuf msg = STRBUF_INIT;
>  	struct strbuf revisions = STRBUF_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct object_id merge_base;
> @@ -2063,30 +2062,29 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		printf(_("First, rewinding head to replay your work on top of "
>  			 "it...\n"));
>
> -	strbuf_addf(&msg, "%s: checkout %s",
> +	strbuf_reset(&buf);
> +	strbuf_addf(&buf, "%s: checkout %s",
>  		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>  	if (reset_head(the_repository, &options.onto->object.oid, "checkout", NULL,
>  		       RESET_HEAD_DETACH | RESET_ORIG_HEAD |
>  		       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
> -		       NULL, msg.buf, DEFAULT_REFLOG_ACTION))
> +		       NULL, buf.buf, DEFAULT_REFLOG_ACTION))
>  		die(_("Could not detach HEAD"));
> -	strbuf_release(&msg);
>
>  	/*
>  	 * If the onto is a proper descendant of the tip of the branch, then
>  	 * we just fast-forwarded.
>  	 */
> -	strbuf_reset(&msg);
> +	strbuf_reset(&buf);
>  	if (oideq(&merge_base, &options.orig_head)) {
>  		printf(_("Fast-forwarded %s to %s.\n"),
>  			branch_name, options.onto_name);
> -		strbuf_addf(&msg, "rebase finished: %s onto %s",
> +		strbuf_addf(&buf, "rebase finished: %s onto %s",
>  			options.head_name ? options.head_name : "detached HEAD",
>  			oid_to_hex(&options.onto->object.oid));
>  		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
> -			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
> +			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
>  			   DEFAULT_REFLOG_ACTION);
> -		strbuf_release(&msg);
>  		ret = !!finish_rebase(&options);
>  		goto cleanup;
>  	}
>

msg is not released if die() is called, but that's OK; in all other
cases it _is_ released in the old code.

I'd rather see the use of that multi-purpose "buf" reduced, e.g. we
could simplify path-building like this in a few cases:

-       strbuf_reset(&buf);
-       strbuf_addf(&buf, "%s/applying", apply_dir());
-       if(file_exists(buf.buf))
+       if (file_exists(mkpath("%s/applying", apply_dir())))

Sure, this looks a bit lispy, but still better than the old code
because there is no state to carry around and reset when "buf" is
repurposed.

René

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
  2021-09-13 18:34     ` René Scharfe
@ 2021-09-13 22:40       ` Junio C Hamano
  2021-09-14 10:31         ` Phillip Wood
  2021-09-14 10:33       ` Phillip Wood
  1 sibling, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2021-09-13 22:40 UTC (permalink / raw)
  To: René Scharfe
  Cc: Phillip Wood via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood

René Scharfe <l.s.r@web.de> writes:

> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> There is already an strbuf that can be reused for creating messages.
>
> Reminds me of a terrible joke from elementary school: In Peter's class
> everybody is called Klaus, except Franz -- his name is Michael.
>
> Why would we want to use the same variable for multiple purposes?  This
> makes the code harder to read.  And the allocation overhead for these
> few cases should be negligible.
>
> The most important question: Is this patch really needed to support
> tags (the purpose of this series)?
>
>> msg is not freed if there is an error and there is a logic error where
>> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
>> strbuf_addf(&msg).
>
> strbuf_reset() after strbuf_release() is redundant but legal.

All good points.

I do not care too deeply either way, in the sense that it probably
is better for this function to have two variables (with at least one
of them having a meaningful name "msg" that tells readers what it is
about), if the original submission to rewrite "rebase" in C used a
single strbuf for both of these and given it a name (like "tmp")
that makes it clear that the buffer is merely a temporary area
without any longer term significance, I probably wouldn't have told
the submitter to rewrite it to use separate strbuf variables.

But if existing code already uses two variables, with at least one
of them having a meaningful name that tells what it is used for, I
see no reason why we want to rewrite it to use a single one.

Thanks.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-13 22:58     ` Junio C Hamano
  2021-09-14 10:17       ` Phillip Wood
  2021-09-14  3:42     ` Elijah Newren
  2021-09-14  9:48     ` Sergey Organov
  2 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2021-09-13 22:58 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
> should checkout the commit pointed to by <tag-object>. Instead it gives

I am not sure if "should checkout the commit pointed to by." is a
good description.  It does not seem to be sufficiently justified.

Did we auto-peel in scripted version of "git rebase" and is this a
regression when the command was rewritten in C?

If that is not the case, this topic is perhaps slightly below
borderline "meh" to me.  The optional "first switch to this <branch>
before doing anything" command-line argument in

    git rebase [--onto <there>] <upstream> [<branch>]

was meant to give a branch, and because we treat detached HEAD as
almost first-class citizen when dealing with branch-ish things, we
allowed

	git rebase master my-topic^0

to try rebasing my-topic on detached HEAD without losing the
original.  In other words, you had to be explicit that you meant the
commit object, not a ref that points at it, to trigger this "rebase
detached" feature.  The same thing for tags.

	git rebase master v12.3^0

would be a proper request to rebase the history leading to that
commit.  Without the peeling, it appears the user is asking to
update the ref that can be uniquely identified with "v12.3", but we
do not want to rebase a tag.

It would have been a different story if we had a problem when a tag
is given to "--onto <there>", but I do not think this topic is about
that case.

Having said that, even if we decide that we shouldn't accept the tag
object and require peeled form to avoid mistakes (instead of
silently peeling the tag ourselves), I do agree that

>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>

is a bad error message for this.  It should be something like

	error: cannot rebase a tag

perhaps.

But if we auto-peeled in an old version, I do not mind this series
(but let's drop pointless "clean-up" that is not, like what was
pointed out by Réne).  In such a case, the first paragraph should
say, instead of "should checkout", that "we used to do X, but commit
Y broke us and now we die with an error message".

Thanks.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-13 22:58     ` Junio C Hamano
@ 2021-09-14  3:42     ` Elijah Newren
  2021-09-14  9:48     ` Sergey Organov
  2 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-14  3:42 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Johannes Schindelin, Phillip Wood

On Mon, Sep 13, 2021 at 8:47 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'

s/stated/started/

> should checkout the commit pointed to by <tag-object>. Instead it gives
>
>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
>     trying to write non-commit object
>     710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
> This is because when we parse the command line arguments although we
> check that the tag points to a commit we remember the oid of the tag
> and try and checkout that object rather than the commit it points
> to. Fix this by using lookup_commit_reference_by_name() when parsing
> the command line.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> ---
>  builtin/rebase.c        | 14 ++++++++------
>  t/t3407-rebase-abort.sh | 18 ++++++++++++++----
>  2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 74663208468..2b70a196f9a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1903,13 +1903,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                         die_if_checked_out(buf.buf, 1);
>                         options.head_name = xstrdup(buf.buf);
>                 /* If not is it a valid ref (branch or commit)? */
> -               } else if (!get_oid(branch_name, &options.orig_head) &&
> -                          lookup_commit_reference(the_repository,
> -                                                  &options.orig_head))
> +               } else {
> +                       struct commit *commit =
> +                               lookup_commit_reference_by_name(branch_name);
> +                       if (!commit)
> +                               die(_("no such branch/commit '%s'"),
> +                                   branch_name);
> +                       oidcpy(&options.orig_head, &commit->object.oid);
>                         options.head_name = NULL;
> -               else
> -                       die(_("no such branch/commit '%s'"),
> -                           branch_name);
> +               }
>         } else if (argc == 0) {
>                 /* Do not need to switch branches, we are already on it. */
>                 options.head_name =
> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
> index 162112ba5ea..ebbaed147a6 100755
> --- a/t/t3407-rebase-abort.sh
> +++ b/t/t3407-rebase-abort.sh
> @@ -11,18 +11,18 @@ test_expect_success setup '
>         test_commit a a a &&
>         git branch to-rebase &&
>
> -       test_commit b a b &&
> -       test_commit c a c &&
> +       test_commit --annotate b a b &&
> +       test_commit --annotate c a c &&
>
>         git checkout to-rebase &&
>         test_commit "merge should fail on this" a d d &&
> -       test_commit "merge should fail on this, too" a e pre-rebase
> +       test_commit --annotate "merge should fail on this, too" a e pre-rebase
>  '
>
>  # Check that HEAD is equal to "pre-rebase" and the current branch is
>  # "to-rebase"
>  check_head() {
> -       test_cmp_rev HEAD pre-rebase &&
> +       test_cmp_rev HEAD pre-rebase^{commit} &&
>         test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
>  }
>
> @@ -67,6 +67,16 @@ testrebase() {
>                 test_path_is_missing "$state_dir"
>         '
>
> +       test_expect_success "rebase$type --abort when checking out a tag" '
> +               test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
> +               git reset --hard a -- &&
> +               test_must_fail git rebase$type --onto b c pre-rebase &&
> +               test_cmp_rev HEAD b^{commit} &&
> +               git rebase --abort &&
> +               test_cmp_rev HEAD pre-rebase^{commit} &&
> +               ! git symbolic-ref HEAD
> +       '
> +
>         test_expect_success "rebase$type --abort does not update reflog" '
>                 # Clean up the state from the previous one
>                 git reset --hard pre-rebase &&
> --
> gitgitgadget

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 00/11] rebase: dereference tags
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (10 preceding siblings ...)
  2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-14  4:02   ` Elijah Newren
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
  12 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-14  4:02 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Johannes Schindelin, Phillip Wood

On Mon, Sep 13, 2021 at 8:46 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Thanks to Ævar and Johannes for their comments.
>
>  * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
>  * Fixed the quoting for the title of the "rebase --quit" tests.
>  * Reworked the last commit to handle the error case first (suggested by
>    Ævar)
>  * Tweaked the commit messages for patches 8 & 11
>  * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
>    2021-09-08) to avoid a merge conflict that upset gitgitgadget
>
> Cover letter for V1:
>
> Aborting a rebase stated with git rebase <upstream> <tag-object> should
> checkout the commit pointed to by . Instead it gives
>
>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
>
> The fix for that is in the last patch, the rest of the patches are cleanups
> to t3407 and builtin/rebase.c

Might make sense to split this into 3 separate series (t3407 cleanups,
builtin/rebase.c cleanups, and the handling of tag objects).  But
anyway, reading over the 11 patches, the only issue I noticed was what
appears to be a simple typo.

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-13 22:58     ` Junio C Hamano
  2021-09-14  3:42     ` Elijah Newren
@ 2021-09-14  9:48     ` Sergey Organov
  2021-09-14  9:58       ` Phillip Wood
  2 siblings, 1 reply; 55+ messages in thread
From: Sergey Organov @ 2021-09-14  9:48 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, Phillip Wood

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
> should checkout the commit pointed to by <tag-object>. Instead it gives
>
>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
>     trying to write non-commit object
>     710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
> This is because when we parse the command line arguments although we
> check that the tag points to a commit we remember the oid of the tag
> and try and checkout that object rather than the commit it points
> to. Fix this by using lookup_commit_reference_by_name() when parsing
> the command line.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

[...]

> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
> index 162112ba5ea..ebbaed147a6 100755
> --- a/t/t3407-rebase-abort.sh
> +++ b/t/t3407-rebase-abort.sh
> @@ -11,18 +11,18 @@ test_expect_success setup '
>  	test_commit a a a &&
>  	git branch to-rebase &&
>  
> -	test_commit b a b &&
> -	test_commit c a c &&
> +	test_commit --annotate b a b &&
> +	test_commit --annotate c a c &&
>  
>  	git checkout to-rebase &&
>  	test_commit "merge should fail on this" a d d &&
> -	test_commit "merge should fail on this, too" a e pre-rebase
> +	test_commit --annotate "merge should fail on this, too" a e pre-rebase
>  '

These two do not seem to belong to this particular commit?

>  
>  # Check that HEAD is equal to "pre-rebase" and the current branch is
>  # "to-rebase"
>  check_head() {
> -	test_cmp_rev HEAD pre-rebase &&
> +	test_cmp_rev HEAD pre-rebase^{commit} &&
>  	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
>  }


-- Sergey Organov

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-14  9:48     ` Sergey Organov
@ 2021-09-14  9:58       ` Phillip Wood
  0 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14  9:58 UTC (permalink / raw)
  To: Sergey Organov, Phillip Wood via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

Hi Sergey

On 14/09/2021 10:48, Sergey Organov wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
>> should checkout the commit pointed to by <tag-object>. Instead it gives
>>
>>      error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
>>      trying to write non-commit object
>>      710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>>
>> This is because when we parse the command line arguments although we
>> check that the tag points to a commit we remember the oid of the tag
>> and try and checkout that object rather than the commit it points
>> to. Fix this by using lookup_commit_reference_by_name() when parsing
>> the command line.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> [...]
> 
>> diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
>> index 162112ba5ea..ebbaed147a6 100755
>> --- a/t/t3407-rebase-abort.sh
>> +++ b/t/t3407-rebase-abort.sh
>> @@ -11,18 +11,18 @@ test_expect_success setup '
>>   	test_commit a a a &&
>>   	git branch to-rebase &&
>>   
>> -	test_commit b a b &&
>> -	test_commit c a c &&
>> +	test_commit --annotate b a b &&
>> +	test_commit --annotate c a c &&
>>   
>>   	git checkout to-rebase &&
>>   	test_commit "merge should fail on this" a d d &&
>> -	test_commit "merge should fail on this, too" a e pre-rebase
>> +	test_commit --annotate "merge should fail on this, too" a e pre-rebase
>>   '
> 
> These two do not seem to belong to this particular commit?

They do, the annotated tag is used in the new test added in this commit 
which tests that we peel tags.

Best Wishes

Phillip
>>   # Check that HEAD is equal to "pre-rebase" and the current branch is
>>   # "to-rebase"
>>   check_head() {
>> -	test_cmp_rev HEAD pre-rebase &&
>> +	test_cmp_rev HEAD pre-rebase^{commit} &&
>>   	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
>>   }
> 
> 
> -- Sergey Organov
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-13 22:58     ` Junio C Hamano
@ 2021-09-14 10:17       ` Phillip Wood
  2021-09-14 13:27         ` Phillip Wood
  0 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:17 UTC (permalink / raw)
  To: Junio C Hamano, Phillip Wood via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

Hi Junio

On 13/09/2021 23:58, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
>> should checkout the commit pointed to by <tag-object>. Instead it gives
> 
> I am not sure if "should checkout the commit pointed to by." is a
> good description.  It does not seem to be sufficiently justified.

My logic was that as we handle commits here it would make sense to 
handle tags as well - I discovered that this did not work when I 
happened to use an annotated tag as the <branch> argument to rebase the 
commits pointed to by the tag and was surprised it did not work when we 
happily accept tags for <upstream> and --onto.

> Did we auto-peel in scripted version of "git rebase" and is this a
> regression when the command was rewritten in C?

As far as I can tell we have never peeled tags here

> If that is not the case, this topic is perhaps slightly below
> borderline "meh" to me.  The optional "first switch to this <branch>
> before doing anything" command-line argument in
> 
>      git rebase [--onto <there>] <upstream> [<branch>]
> 
> was meant to give a branch, and because we treat detached HEAD as
> almost first-class citizen when dealing with branch-ish things, we
> allowed
> 
> 	git rebase master my-topic^0
> 
> to try rebasing my-topic on detached HEAD without losing the
> original.  In other words, you had to be explicit that you meant the
> commit object, not a ref that points at it, to trigger this "rebase
> detached" feature.  The same thing for tags.
> 
> 	git rebase master v12.3^0
> 
> would be a proper request to rebase the history leading to that
> commit.  Without the peeling, it appears the user is asking to
> update the ref that can be uniquely identified with "v12.3", but we
> do not want to rebase a tag.

I wrote this patch as I felt it was an artificial distinction to require 
that <branch> is a branch-ish thing rather than a commit-ish thing. 
Rebase already peels <upstream> and --onto so it feels inconsistent not 
to do it for <branch>. I guess the counter argument to that is users may 
be confused and start complaining that the tag itself is not rebased.

> It would have been a different story if we had a problem when a tag
> is given to "--onto <there>", but I do not think this topic is about
> that case.

No "--onto <tag>" works fine. We also accept a tag object for upstream 
without requiring the user to peel it for us.

> Having said that, even if we decide that we shouldn't accept the tag
> object and require peeled form to avoid mistakes (instead of
> silently peeling the tag ourselves), I do agree that
> 
>>      error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>>
> 
> is a bad error message for this.  It should be something like
> 
> 	error: cannot rebase a tag
> 
> perhaps.

We could do that if we're worried that users would be confused by the 
tag not being rebased if we started automatically peeling <branch>. (I'm 
kind of leaning in that direction at the moment having read your email)

Best Wishes

Phillip

> But if we auto-peeled in an old version, I do not mind this series
> (but let's drop pointless "clean-up" that is not, like what was
> pointed out by Réne).  In such a case, the first paragraph should
> say, instead of "should checkout", that "we used to do X, but commit
> Y broke us and now we die with an error message".
> 
> Thanks.
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
  2021-09-13 22:40       ` Junio C Hamano
@ 2021-09-14 10:31         ` Phillip Wood
  0 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:31 UTC (permalink / raw)
  To: Junio C Hamano, René Scharfe
  Cc: Phillip Wood via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

On 13/09/2021 23:40, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> There is already an strbuf that can be reused for creating messages.
>>
>> Reminds me of a terrible joke from elementary school: In Peter's class
>> everybody is called Klaus, except Franz -- his name is Michael.
>>
>> Why would we want to use the same variable for multiple purposes?  This
>> makes the code harder to read.  And the allocation overhead for these
>> few cases should be negligible.

For better or worse reusing the same strbuf is a common pattern and when 
I saw the code switch to using a different variable I wondered if it was 
because the value of buf was being used later. It is also confusing to 
free msg in a different place to all the other variables. rebase_cmd() 
being so long does not help (msg is defined 763 lines before its first 
use) as it makes it harder to see what is going on.

>> The most important question: Is this patch really needed to support
>> tags (the purpose of this series)?
>>
>>> msg is not freed if there is an error and there is a logic error where
>>> we call strbuf_release(&msg) followed by strbuf_reset(&msg) and
>>> strbuf_addf(&msg).
>>
>> strbuf_reset() after strbuf_release() is redundant but legal.

It is confusing to the reader though, I spent time checking why the 
strbuf_release() call was there before concluding it was a mistake.

> All good points.
> 
> I do not care too deeply either way, in the sense that it probably
> is better for this function to have two variables (with at least one
> of them having a meaningful name "msg" that tells readers what it is
> about), if the original submission to rewrite "rebase" in C used a
> single strbuf for both of these and given it a name (like "tmp")
> that makes it clear that the buffer is merely a temporary area
> without any longer term significance, I probably wouldn't have told
> the submitter to rewrite it to use separate strbuf variables.
> 
> But if existing code already uses two variables, with at least one
> of them having a meaningful name that tells what it is used for, I
> see no reason why we want to rewrite it to use a single one.

In a short function where it is easy to see what happens between the 
variable declaration and its use I'd agree but here everything is so 
spread out I actually found the switch to a second variable confusing.

Best Wishes

Phillip

> Thanks.
> 

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 08/11] rebase: remove redundant strbuf
  2021-09-13 18:34     ` René Scharfe
  2021-09-13 22:40       ` Junio C Hamano
@ 2021-09-14 10:33       ` Phillip Wood
  1 sibling, 0 replies; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 10:33 UTC (permalink / raw)
  To: René Scharfe, Phillip Wood via GitGitGadget, git
  Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

Hi René

Thanks for looking at this series

On 13/09/2021 19:34, René Scharfe wrote:
> Am 13.09.21 um 17:19 schrieb Phillip Wood via GitGitGadget:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> I'd rather see the use of that multi-purpose "buf" reduced, e.g. we
> could simplify path-building like this in a few cases:
> 
> -       strbuf_reset(&buf);
> -       strbuf_addf(&buf, "%s/applying", apply_dir());
> -       if(file_exists(buf.buf))
> +       if (file_exists(mkpath("%s/applying", apply_dir())))
> 
> Sure, this looks a bit lispy, but still better than the old code
> because there is no state to carry around and reset when "buf" is
> repurposed.

That's a nice suggestion, I think it's much clearer which file we're 
checking for.

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-14 10:17       ` Phillip Wood
@ 2021-09-14 13:27         ` Phillip Wood
  2021-09-14 16:29           ` Junio C Hamano
  0 siblings, 1 reply; 55+ messages in thread
From: Phillip Wood @ 2021-09-14 13:27 UTC (permalink / raw)
  To: Junio C Hamano, Phillip Wood via GitGitGadget
  Cc: git, Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

On 14/09/2021 11:17, Phillip Wood wrote:
> Hi Junio
> 
> On 13/09/2021 23:58, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
>>> should checkout the commit pointed to by <tag-object>. Instead it gives
>>
>> I am not sure if "should checkout the commit pointed to by." is a
>> good description.  It does not seem to be sufficiently justified.
> 
> My logic was that as we handle commits here it would make sense to 
> handle tags as well - I discovered that this did not work when I 
> happened to use an annotated tag as the <branch> argument to rebase the 
> commits pointed to by the tag and was surprised it did not work when we 
> happily accept tags for <upstream> and --onto.
> 
>> Did we auto-peel in scripted version of "git rebase" and is this a
>> regression when the command was rewritten in C?
> 
> As far as I can tell we have never peeled tags here

That's a bit misleading. We have never peeled a tag given as <branch> 
when we parse it. In the scripted version we just passed the tag oid 
along to rev-list, checkout and reset and they peeled it. So I think 
this is actually a regression in the builtin rebase. I'll update the 
commit message to reflect that unless we feel that allowing a tag for 
<branch> is a mistake and we should be erroring out to avoid the 
possible confusion of the tag not being rebased, only the commits it 
points to.

Sorry for the confusion

Phillip

>> If that is not the case, this topic is perhaps slightly below
>> borderline "meh" to me.  The optional "first switch to this <branch>
>> before doing anything" command-line argument in
>>
>>      git rebase [--onto <there>] <upstream> [<branch>]
>>
>> was meant to give a branch, and because we treat detached HEAD as
>> almost first-class citizen when dealing with branch-ish things, we
>> allowed
>>
>>     git rebase master my-topic^0
>>
>> to try rebasing my-topic on detached HEAD without losing the
>> original.  In other words, you had to be explicit that you meant the
>> commit object, not a ref that points at it, to trigger this "rebase
>> detached" feature.  The same thing for tags.
>>
>>     git rebase master v12.3^0
>>
>> would be a proper request to rebase the history leading to that
>> commit.  Without the peeling, it appears the user is asking to
>> update the ref that can be uniquely identified with "v12.3", but we
>> do not want to rebase a tag.
> 
> I wrote this patch as I felt it was an artificial distinction to require 
> that <branch> is a branch-ish thing rather than a commit-ish thing. 
> Rebase already peels <upstream> and --onto so it feels inconsistent not 
> to do it for <branch>. I guess the counter argument to that is users may 
> be confused and start complaining that the tag itself is not rebased.
> 
>> It would have been a different story if we had a problem when a tag
>> is given to "--onto <there>", but I do not think this topic is about
>> that case.
> 
> No "--onto <tag>" works fine. We also accept a tag object for upstream 
> without requiring the user to peel it for us.
> 
>> Having said that, even if we decide that we shouldn't accept the tag
>> object and require peeled form to avoid mistakes (instead of
>> silently peeling the tag ourselves), I do agree that
>>
>>>      error: update_ref failed for ref 'HEAD': cannot update ref 
>>> 'HEAD': trying to write non-commit object       
>>> 710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>>>
>>
>> is a bad error message for this.  It should be something like
>>
>>     error: cannot rebase a tag
>>
>> perhaps.
> 
> We could do that if we're worried that users would be confused by the 
> tag not being rebased if we started automatically peeling <branch>. (I'm 
> kind of leaning in that direction at the moment having read your email)
> 
> Best Wishes
> 
> Phillip
> 
>> But if we auto-peeled in an old version, I do not mind this series
>> (but let's drop pointless "clean-up" that is not, like what was
>> pointed out by Réne).  In such a case, the first paragraph should
>> say, instead of "should checkout", that "we used to do X, but commit
>> Y broke us and now we die with an error message".
>>
>> Thanks.
>>

^ permalink raw reply	[flat|nested] 55+ messages in thread

* Re: [PATCH v2 11/11] rebase: dereference tags
  2021-09-14 13:27         ` Phillip Wood
@ 2021-09-14 16:29           ` Junio C Hamano
  0 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2021-09-14 16:29 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Phillip Wood via GitGitGadget, git,
	Ævar Arnfjörð Bjarmason, Johannes Schindelin,
	Phillip Wood

Phillip Wood <phillip.wood123@gmail.com> writes:

>>> Did we auto-peel in scripted version of "git rebase" and is this a
>>> regression when the command was rewritten in C?
>> As far as I can tell we have never peeled tags here
>
> That's a bit misleading. We have never peeled a tag given as <branch>
> when we parse it. In the scripted version we just passed the tag oid 
> along to rev-list, checkout and reset and they peeled it. So I think
> this is actually a regression in the builtin rebase. I'll update the 
> commit message to reflect that unless we feel that allowing a tag for
> <branch> is a mistake and we should be erroring out to avoid the 
> possible confusion of the tag not being rebased, only the commits it
> points to.

OK, so this is a regression fix.  That makes the change much simpler
to sell.  I'd expect that the description would be along the lines
of something like this, perhaps.

    A rebase started with 'git rebase <A> <B>' is conceptually to
    first checkout <B> and run 'git rebase <A>' starting from that
    state.  'git rebase --abort' in the middle of such a rebase
    should take us back to the state we checked out <B>.

    This used to work, even when <B> is a tag that points at a
    commit, until Git X.Y.Z when the command was reimplemented in C.
    The command now complains that the tag object itself cannot be
    checked out, which may be technically correct but is not what
    the user asked to do.

    Fix this old regression by doing ....

Thanks for digging (and fixing, of course).


^ permalink raw reply	[flat|nested] 55+ messages in thread

* [PATCH v3 00/10] rebase: dereference tags
  2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
                     ` (11 preceding siblings ...)
  2021-09-14  4:02   ` [PATCH v2 00/11] " Elijah Newren
@ 2021-09-21 10:23   ` Phillip Wood via GitGitGadget
  2021-09-21 10:23     ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
                       ` (10 more replies)
  12 siblings, 11 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood

Thanks for the comments on V2. Here are the changes from that version:

 * dropped the controversial cleanup that was patch 8
 * reworded patch 10 as suggested by Junio

Cover letter for v2: Thanks to Ævar and Johannes for their comments.

 * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
 * Fixed the quoting for the title of the "rebase --quit" tests.
 * Reworked the last commit to handle the error case first (suggested by
   Ævar)
 * Tweaked the commit messages for patches 8 & 11
 * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
   2021-09-08) to avoid a merge conflict that upset gitgitgadget

Cover letter for V1: Aborting a rebase stated with git rebase <upstream>
<tag-object> should checkout the commit pointed to by . Instead it gives

    error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'


The fix for that is in the last patch, the rest of the patches are cleanups
to t3407 and builtin/rebase.c

Phillip Wood (10):
  t3407: run tests in $TEST_DIRECTORY
  t3407: use test_commit
  t3407: use test_cmp_rev
  t3407: rename a variable
  t3407: use test_path_is_missing
  t3407: strengthen rebase --abort tests
  t3407: rework rebase --quit tests
  rebase: use our standard error return value
  rebase: use lookup_commit_reference_by_name()
  rebase: dereference tags

 builtin/rebase.c        |  49 ++++++++-----------
 t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
 2 files changed, 67 insertions(+), 87 deletions(-)


base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1033

Range-diff vs v2:

  1:  bac009d8543 =  1:  bac009d8543 t3407: run tests in $TEST_DIRECTORY
  2:  abfffb31a56 =  2:  abfffb31a56 t3407: use test_commit
  3:  7755ce17fef =  3:  7755ce17fef t3407: use test_cmp_rev
  4:  38eee11baf5 =  4:  38eee11baf5 t3407: rename a variable
  5:  61a37c89f1e =  5:  61a37c89f1e t3407: use test_path_is_missing
  6:  6866630528b =  6:  6866630528b t3407: strengthen rebase --abort tests
  7:  fd55a3196b1 =  7:  fd55a3196b1 t3407: rework rebase --quit tests
  8:  ad3c4efc027 <  -:  ----------- rebase: remove redundant strbuf
  9:  ad940b633d0 !  8:  d3af62c746c rebase: use our standard error return value
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       		} else if (!(options.flags & REBASE_NO_QUIET))
       			; /* be quiet */
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     - 		reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
     - 			   RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
     + 			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
       			   DEFAULT_REFLOG_ACTION);
     + 		strbuf_release(&msg);
      -		ret = !!finish_rebase(&options);
      +		ret = finish_rebase(&options);
       		goto cleanup;
 10:  bc103e703e8 =  9:  4eedd3ae766 rebase: use lookup_commit_reference_by_name()
 11:  951de6bb199 ! 10:  55a6250ab38 rebase: dereference tags
     @@ Metadata
       ## Commit message ##
          rebase: dereference tags
      
     -    Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
     -    should checkout the commit pointed to by <tag-object>. Instead it gives
     +    A rebase started with 'git rebase <A> <B>' is conceptually to first
     +    checkout <B> and run 'git rebase <A>' starting from that state.  'git
     +    rebase --abort' in the middle of such a rebase should take us back to
     +    the state we checked out <B>.
      
     -        error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
     -        trying to write non-commit object
     -        710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
     +    This used to work, even when <B> is a tag that points at a commit,
     +    until Git 2.20.0 when the command was reimplemented in C.  The command
     +    now complains that the tag object itself cannot be checked out, which
     +    may be technically correct but is not what the user asked to do.
      
     -    This is because when we parse the command line arguments although we
     -    check that the tag points to a commit we remember the oid of the tag
     -    and try and checkout that object rather than the commit it points
     -    to. Fix this by using lookup_commit_reference_by_name() when parsing
     -    the command line.
     +    Fix this old regression by using lookup_commit_reference_by_name()
     +    when parsing <B>. The scripted version did not need to peel the tag
     +    because the commands it passed the tag to (e.g 'git reset') peeled the
     +    tag themselves.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
          ---

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 55+ messages in thread

* [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
@ 2021-09-21 10:23     ` Phillip Wood via GitGitGadget
  2021-09-21 10:23     ` [PATCH v3 02/10] t3407: use test_commit Phillip Wood via GitGitGadget
                       ` (9 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Commit 97b88dd58c ("git-rebase.sh: Fix --merge --abort failures when
path contains whitespace", 2008-05-04) started running these tests in
a subdirectory with a space in its name. At that time $TEST_DIRECTORY
did not contain a space but shortly after in 4a7aaccd83 ("Rename the
test trash directory to contain spaces.", 2008-05-04) $TEST_DIRECTORY
was changed to contain a space so we no longer need to run these tests
in a subdirectory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7c381fbc89a..c747bd31d76 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -7,13 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 
-### Test that we handle space characters properly
-work_dir="$(pwd)/test dir"
-
 test_expect_success setup '
-	mkdir -p "$work_dir" &&
-	cd "$work_dir" &&
-	git init &&
 	echo a > a &&
 	git add a &&
 	git commit -m a &&
@@ -37,7 +31,6 @@ testrebase() {
 	dotest=$2
 
 	test_expect_success "rebase$type --abort" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -48,7 +41,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -61,7 +53,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -77,7 +68,6 @@ testrebase() {
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		git reflog show to-rebase > reflog_before &&
@@ -89,7 +79,6 @@ testrebase() {
 	'
 
 	test_expect_success 'rebase --abort can not be used with other options' '
-		cd "$work_dir" &&
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
@@ -103,7 +92,6 @@ testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
 test_expect_success 'rebase --apply --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --apply main &&
@@ -115,7 +103,6 @@ test_expect_success 'rebase --apply --quit' '
 '
 
 test_expect_success 'rebase --merge --quit' '
-	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --merge main &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 02/10] t3407: use test_commit
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
  2021-09-21 10:23     ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
@ 2021-09-21 10:23     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 03/10] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
                       ` (8 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:23 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Simplify the setup by using test_commit. Note that this replaces the
branch "pre-rebase" with a tag of the same name. "pre-rebase" is only
used as a fixed reference point so it makes sense to use a tag rather
than a branch.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index c747bd31d76..0ad21966bc5 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -8,22 +8,15 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo a > a &&
-	git add a &&
-	git commit -m a &&
+	test_commit a a a &&
 	git branch to-rebase &&
 
-	echo b > a &&
-	git commit -a -m b &&
-	echo c > a &&
-	git commit -a -m c &&
+	test_commit b a b &&
+	test_commit c a c &&
 
 	git checkout to-rebase &&
-	echo d > a &&
-	git commit -a -m "merge should fail on this" &&
-	echo e > a &&
-	git commit -a -m "merge should fail on this, too" &&
-	git branch pre-rebase
+	test_commit "merge should fail on this" a d d &&
+	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
 testrebase() {
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 03/10] t3407: use test_cmp_rev
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
  2021-09-21 10:23     ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
  2021-09-21 10:23     ` [PATCH v3 02/10] t3407: use test_commit Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 04/10] t3407: rename a variable Phillip Wood via GitGitGadget
                       ` (7 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This provides better diagnostics if a test fails

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 0ad21966bc5..39076ac9462 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -29,7 +29,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -39,9 +39,9 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$dotest" &&
 		test_must_fail git rebase --skip &&
-		test $(git rev-parse HEAD) = $(git rev-parse main) &&
+		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -54,9 +54,9 @@ testrebase() {
 		echo d >> a &&
 		git add a &&
 		test_must_fail git rebase --continue &&
-		test $(git rev-parse HEAD) != $(git rev-parse main) &&
+		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
-		test $(git rev-parse to-rebase) = $(git rev-parse pre-rebase) &&
+		test_cmp_rev to-rebase pre-rebase &&
 		test ! -d "$dotest"
 	'
 
@@ -91,7 +91,7 @@ test_expect_success 'rebase --apply --quit' '
 	test_path_is_dir .git/rebase-apply &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-apply
 '
 
@@ -102,7 +102,7 @@ test_expect_success 'rebase --merge --quit' '
 	test_path_is_dir .git/rebase-merge &&
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
-	test $(git rev-parse HEAD) = $head_before &&
+	test_cmp_rev HEAD $head_before &&
 	test ! -d .git/rebase-merge
 '
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 04/10] t3407: rename a variable
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (2 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 03/10] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 05/10] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
                       ` (6 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

$dotest holds the name of the rebase state directory and was named
after the state directory used at the time the test was added. As we
no longer use that name rename the variable to reflect its purpose.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 39076ac9462..2c70230a4eb 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -21,35 +21,35 @@ test_expect_success setup '
 
 testrebase() {
 	type=$1
-	dotest=$2
+	state_dir=$2
 
 	test_expect_success "rebase$type --abort" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
-		test_path_is_dir "$dotest" &&
+		test_path_is_dir "$state_dir" &&
 		echo c > a &&
 		echo d >> a &&
 		git add a &&
@@ -57,7 +57,7 @@ testrebase() {
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$dotest"
+		test ! -d "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 05/10] t3407: use test_path_is_missing
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (3 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 04/10] t3407: rename a variable Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 06/10] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
                       ` (5 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

At the end of the test we expect the state directory to be missing,
but the tests only check it is not a directory.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 2c70230a4eb..7eba9ec1619 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -30,7 +30,7 @@ testrebase() {
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --skip" '
@@ -42,7 +42,7 @@ testrebase() {
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort after --continue" '
@@ -57,7 +57,7 @@ testrebase() {
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
 		test_cmp_rev to-rebase pre-rebase &&
-		test ! -d "$state_dir"
+		test_path_is_missing "$state_dir"
 	'
 
 	test_expect_success "rebase$type --abort does not update reflog" '
@@ -92,7 +92,7 @@ test_expect_success 'rebase --apply --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-apply
+	test_path_is_missing .git/rebase-apply
 '
 
 test_expect_success 'rebase --merge --quit' '
@@ -103,7 +103,7 @@ test_expect_success 'rebase --merge --quit' '
 	head_before=$(git rev-parse HEAD) &&
 	git rebase --quit &&
 	test_cmp_rev HEAD $head_before &&
-	test ! -d .git/rebase-merge
+	test_path_is_missing .git/rebase-merge
 '
 
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 06/10] t3407: strengthen rebase --abort tests
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (4 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 05/10] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 07/10] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
                       ` (4 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The existing tests only check that HEAD points to the correct
commit after aborting, they do not check that the original branch
is checked out.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 7eba9ec1619..f8264449b6c 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -19,6 +19,13 @@ test_expect_success setup '
 	test_commit "merge should fail on this, too" a e pre-rebase
 '
 
+# Check that HEAD is equal to "pre-rebase" and the current branch is
+# "to-rebase"
+check_head() {
+	test_cmp_rev HEAD pre-rebase &&
+	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
+}
+
 testrebase() {
 	type=$1
 	state_dir=$2
@@ -29,7 +36,7 @@ testrebase() {
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$state_dir" &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -41,7 +48,7 @@ testrebase() {
 		test_must_fail git rebase --skip &&
 		test_cmp_rev HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
@@ -56,7 +63,7 @@ testrebase() {
 		test_must_fail git rebase --continue &&
 		test_cmp_rev ! HEAD main &&
 		git rebase --abort &&
-		test_cmp_rev to-rebase pre-rebase &&
+		check_head &&
 		test_path_is_missing "$state_dir"
 	'
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 07/10] t3407: rework rebase --quit tests
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (5 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 06/10] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 08/10] rebase: use our standard error return value Phillip Wood via GitGitGadget
                       ` (3 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

9512177b68 ("rebase: add --quit to cleanup rebase, leave everything
else untouched", 2016-11-12) seems to have copied the --abort tests
but added two separate tests for the two rebase backends rather than
adding a single test into the existing testrebase() function.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3407-rebase-abort.sh | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index f8264449b6c..162112ba5ea 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -86,31 +86,21 @@ testrebase() {
 		test_must_fail git rebase --abort -v &&
 		git rebase --abort
 	'
+
+	test_expect_success "rebase$type --quit" '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		# Clean up the state from the previous one
+		git reset --hard pre-rebase &&
+		test_must_fail git rebase$type main &&
+		test_path_is_dir $state_dir &&
+		head_before=$(git rev-parse HEAD) &&
+		git rebase --quit &&
+		test_cmp_rev HEAD $head_before &&
+		test_path_is_missing .git/rebase-apply
+	'
 }
 
 testrebase " --apply" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
-test_expect_success 'rebase --apply --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --apply main &&
-	test_path_is_dir .git/rebase-apply &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-apply
-'
-
-test_expect_success 'rebase --merge --quit' '
-	# Clean up the state from the previous one
-	git reset --hard pre-rebase &&
-	test_must_fail git rebase --merge main &&
-	test_path_is_dir .git/rebase-merge &&
-	head_before=$(git rev-parse HEAD) &&
-	git rebase --quit &&
-	test_cmp_rev HEAD $head_before &&
-	test_path_is_missing .git/rebase-merge
-'
-
 test_done
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 08/10] rebase: use our standard error return value
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (6 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 07/10] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
                       ` (2 subsequent siblings)
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Git uses −1 to signal an error. The builtin rebase converts these to
+1 all over the place using !! (presumably because the in the scripted
version an error was signalled by +1). This is confusing and clutters
the code, we only need to convert the value when the function returns.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6138009d6e4..444e169ef74 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1574,7 +1574,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("could not move back to %s"),
 			    oid_to_hex(&options.orig_head));
 		remove_branch_state(the_repository, 0);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 	case ACTION_QUIT: {
@@ -1583,11 +1583,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			struct replay_opts replay = REPLAY_OPTS_INIT;
 
 			replay.action = REPLAY_INTERACTIVE_REBASE;
-			ret = !!sequencer_remove_state(&replay);
+			ret = sequencer_remove_state(&replay);
 		} else {
 			strbuf_reset(&buf);
 			strbuf_addstr(&buf, options.state_dir);
-			ret = !!remove_dir_recursively(&buf, 0);
+			ret = remove_dir_recursively(&buf, 0);
 			if (ret)
 				error(_("could not remove '%s'"),
 				       options.state_dir);
@@ -1959,7 +1959,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	if (require_clean_work_tree(the_repository, "rebase",
 				    _("Please commit or stash them."), 1, 1)) {
-		ret = 1;
+		ret = -1;
 		goto cleanup;
 	}
 
@@ -1994,7 +1994,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
 					       NULL, buf.buf,
 					       DEFAULT_REFLOG_ACTION) < 0) {
-					ret = !!error(_("could not switch to "
+					ret = error(_("could not switch to "
 							"%s"),
 						      options.switch_to);
 					goto cleanup;
@@ -2009,7 +2009,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			else
 				printf(_("Current branch %s is up to date.\n"),
 				       branch_name);
-			ret = !!finish_rebase(&options);
+			ret = finish_rebase(&options);
 			goto cleanup;
 		} else if (!(options.flags & REBASE_NO_QUIET))
 			; /* be quiet */
@@ -2087,7 +2087,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			   RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
 			   DEFAULT_REFLOG_ACTION);
 		strbuf_release(&msg);
-		ret = !!finish_rebase(&options);
+		ret = finish_rebase(&options);
 		goto cleanup;
 	}
 
@@ -2101,7 +2101,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	options.revisions = revisions.buf;
 
 run_rebase:
-	ret = !!run_specific_rebase(&options, action);
+	ret = run_specific_rebase(&options, action);
 
 cleanup:
 	strbuf_release(&buf);
@@ -2112,5 +2112,5 @@ cleanup:
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
-	return ret;
+	return !!ret;
 }
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name()
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (7 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 08/10] rebase: use our standard error return value Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-21 10:24     ` [PATCH v3 10/10] rebase: dereference tags Phillip Wood via GitGitGadget
  2021-09-24  1:26     ` [PATCH v3 00/10] " Elijah Newren
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

peel_committish() appears to have been copied from the scripted rebase
but it duplicates the functionality of
lookup_commit_reference_by_name() so lets use that instead.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 444e169ef74..0ace9e0a8ec 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -762,17 +762,6 @@ static int finish_rebase(struct rebase_options *opts)
 	return ret;
 }
 
-static struct commit *peel_committish(const char *name)
-{
-	struct object *obj;
-	struct object_id oid;
-
-	if (get_oid(name, &oid))
-		return NULL;
-	obj = parse_object(the_repository, &oid);
-	return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
-}
-
 static void add_var(struct strbuf *buf, const char *name, const char *value)
 {
 	if (!value)
@@ -1845,7 +1834,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			if (!strcmp(options.upstream_name, "-"))
 				options.upstream_name = "@{-1}";
 		}
-		options.upstream = peel_committish(options.upstream_name);
+		options.upstream =
+			lookup_commit_reference_by_name(options.upstream_name);
 		if (!options.upstream)
 			die(_("invalid upstream '%s'"), options.upstream_name);
 		options.upstream_arg = options.upstream_name;
@@ -1888,7 +1878,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.onto = lookup_commit_or_die(&merge_base,
 						    options.onto_name);
 	} else {
-		options.onto = peel_committish(options.onto_name);
+		options.onto =
+			lookup_commit_reference_by_name(options.onto_name);
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 55+ messages in thread

* [PATCH v3 10/10] rebase: dereference tags
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (8 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
@ 2021-09-21 10:24     ` Phillip Wood via GitGitGadget
  2021-09-24  1:26     ` [PATCH v3 00/10] " Elijah Newren
  10 siblings, 0 replies; 55+ messages in thread
From: Phillip Wood via GitGitGadget @ 2021-09-21 10:24 UTC (permalink / raw)
  To: git
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Johannes Schindelin, René Scharfe, Elijah Newren,
	Sergey Organov, Phillip Wood, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

A rebase started with 'git rebase <A> <B>' is conceptually to first
checkout <B> and run 'git rebase <A>' starting from that state.  'git
rebase --abort' in the middle of such a rebase should take us back to
the state we checked out <B>.

This used to work, even when <B> is a tag that points at a commit,
until Git 2.20.0 when the command was reimplemented in C.  The command
now complains that the tag object itself cannot be checked out, which
may be technically correct but is not what the user asked to do.

Fix this old regression by using lookup_commit_reference_by_name()
when parsing <B>. The scripted version did not need to peel the tag
because the commands it passed the tag to (e.g 'git reset') peeled the
tag themselves.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
---
 builtin/rebase.c        | 14 ++++++++------
 t/t3407-rebase-abort.sh | 18 ++++++++++++++----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0ace9e0a8ec..b4433ee7978 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1904,13 +1904,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
-		} else if (!get_oid(branch_name, &options.orig_head) &&
-			   lookup_commit_reference(the_repository,
-						   &options.orig_head))
+		} else {
+			struct commit *commit =
+				lookup_commit_reference_by_name(branch_name);
+			if (!commit)
+				die(_("no such branch/commit '%s'"),
+				    branch_name);
+			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
-		else
-			die(_("no such branch/commit '%s'"),
-			    branch_name);
+		}
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */
 		options.head_name =
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 162112ba5ea..ebbaed147a6 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -11,18 +11,18 @@ test_expect_success setup '
 	test_commit a a a &&
 	git branch to-rebase &&
 
-	test_commit b a b &&
-	test_commit c a c &&
+	test_commit --annotate b a b &&
+	test_commit --annotate c a c &&
 
 	git checkout to-rebase &&
 	test_commit "merge should fail on this" a d d &&
-	test_commit "merge should fail on this, too" a e pre-rebase
+	test_commit --annotate "merge should fail on this, too" a e pre-rebase
 '
 
 # Check that HEAD is equal to "pre-rebase" and the current branch is
 # "to-rebase"
 check_head() {
-	test_cmp_rev HEAD pre-rebase &&
+	test_cmp_rev HEAD pre-rebase^{commit} &&
 	test "$(git symbolic-ref HEAD)" = refs/heads/to-rebase
 }
 
@@ -67,6 +67,16 @@ testrebase() {
 		test_path_is_missing "$state_dir"
 	'
 
+	test_expect_success "rebase$type --abort when checking out a tag" '
+		test_when_finished "git symbolic-ref HEAD refs/heads/to-rebase" &&
+		git reset --hard a -- &&
+		test_must_fail git rebase$type --onto b c pre-rebase &&
+		test_cmp_rev HEAD b^{commit} &&
+		git rebase --abort &&
+		test_cmp_rev HEAD pre-rebase^{commit} &&
+		! git symbolic-ref HEAD
+	'
+
 	test_expect_success "rebase$type --abort does not update reflog" '
 		# Clean up the state from the previous one
 		git reset --hard pre-rebase &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 55+ messages in thread

* Re: [PATCH v3 00/10] rebase: dereference tags
  2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
                       ` (9 preceding siblings ...)
  2021-09-21 10:24     ` [PATCH v3 10/10] rebase: dereference tags Phillip Wood via GitGitGadget
@ 2021-09-24  1:26     ` Elijah Newren
  10 siblings, 0 replies; 55+ messages in thread
From: Elijah Newren @ 2021-09-24  1:26 UTC (permalink / raw)
  To: Phillip Wood via GitGitGadget
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Johannes Schindelin, René Scharfe,
	Sergey Organov, Phillip Wood

On Tue, Sep 21, 2021 at 3:24 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Thanks for the comments on V2. Here are the changes from that version:
>
>  * dropped the controversial cleanup that was patch 8
>  * reworded patch 10 as suggested by Junio
>
> Cover letter for v2: Thanks to Ævar and Johannes for their comments.

This version looks good to me.

>
>  * Changed "! test_cmp_rev" to "test_cmp_rev !" (suggested by Ævar)
>  * Fixed the quoting for the title of the "rebase --quit" tests.
>  * Reworked the last commit to handle the error case first (suggested by
>    Ævar)
>  * Tweaked the commit messages for patches 8 & 11
>  * Rebased onto 31e4a0db03 ("Merge branch 'ab/rebase-fatal-fatal-fix'",
>    2021-09-08) to avoid a merge conflict that upset gitgitgadget
>
> Cover letter for V1: Aborting a rebase stated with git rebase <upstream>
> <tag-object> should checkout the commit pointed to by . Instead it gives
>
>     error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD': trying to write non-commit object       710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>
>
> The fix for that is in the last patch, the rest of the patches are cleanups
> to t3407 and builtin/rebase.c
>
> Phillip Wood (10):
>   t3407: run tests in $TEST_DIRECTORY
>   t3407: use test_commit
>   t3407: use test_cmp_rev
>   t3407: rename a variable
>   t3407: use test_path_is_missing
>   t3407: strengthen rebase --abort tests
>   t3407: rework rebase --quit tests
>   rebase: use our standard error return value
>   rebase: use lookup_commit_reference_by_name()
>   rebase: dereference tags
>
>  builtin/rebase.c        |  49 ++++++++-----------
>  t/t3407-rebase-abort.sh | 105 ++++++++++++++++++----------------------
>  2 files changed, 67 insertions(+), 87 deletions(-)
>
>
> base-commit: 31e4a0db0337e2aa972d9b9f11a332dff7c4cbcb
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1033%2Fphillipwood%2Fwip%2Frebase-handle-tags-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1033/phillipwood/wip/rebase-handle-tags-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1033
>
> Range-diff vs v2:
>
>   1:  bac009d8543 =  1:  bac009d8543 t3407: run tests in $TEST_DIRECTORY
>   2:  abfffb31a56 =  2:  abfffb31a56 t3407: use test_commit
>   3:  7755ce17fef =  3:  7755ce17fef t3407: use test_cmp_rev
>   4:  38eee11baf5 =  4:  38eee11baf5 t3407: rename a variable
>   5:  61a37c89f1e =  5:  61a37c89f1e t3407: use test_path_is_missing
>   6:  6866630528b =  6:  6866630528b t3407: strengthen rebase --abort tests
>   7:  fd55a3196b1 =  7:  fd55a3196b1 t3407: rework rebase --quit tests
>   8:  ad3c4efc027 <  -:  ----------- rebase: remove redundant strbuf
>   9:  ad940b633d0 !  8:  d3af62c746c rebase: use our standard error return value
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>                 } else if (!(options.flags & REBASE_NO_QUIET))
>                         ; /* be quiet */
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      -          reset_head(the_repository, NULL, "Fast-forwarded", options.head_name,
>      -                     RESET_HEAD_REFS_ONLY, "HEAD", buf.buf,
>      +                     RESET_HEAD_REFS_ONLY, "HEAD", msg.buf,
>                            DEFAULT_REFLOG_ACTION);
>      +          strbuf_release(&msg);
>       -         ret = !!finish_rebase(&options);
>       +         ret = finish_rebase(&options);
>                 goto cleanup;
>  10:  bc103e703e8 =  9:  4eedd3ae766 rebase: use lookup_commit_reference_by_name()
>  11:  951de6bb199 ! 10:  55a6250ab38 rebase: dereference tags
>      @@ Metadata
>        ## Commit message ##
>           rebase: dereference tags
>
>      -    Aborting a rebase stated with 'git rebase <upstream> <tag-object>'
>      -    should checkout the commit pointed to by <tag-object>. Instead it gives
>      +    A rebase started with 'git rebase <A> <B>' is conceptually to first
>      +    checkout <B> and run 'git rebase <A>' starting from that state.  'git
>      +    rebase --abort' in the middle of such a rebase should take us back to
>      +    the state we checked out <B>.
>
>      -        error: update_ref failed for ref 'HEAD': cannot update ref 'HEAD':
>      -        trying to write non-commit object
>      -        710d743b2b9892457fdcc3970f397e6ec07447e0 to branch 'HEAD'
>      +    This used to work, even when <B> is a tag that points at a commit,
>      +    until Git 2.20.0 when the command was reimplemented in C.  The command
>      +    now complains that the tag object itself cannot be checked out, which
>      +    may be technically correct but is not what the user asked to do.
>
>      -    This is because when we parse the command line arguments although we
>      -    check that the tag points to a commit we remember the oid of the tag
>      -    and try and checkout that object rather than the commit it points
>      -    to. Fix this by using lookup_commit_reference_by_name() when parsing
>      -    the command line.
>      +    Fix this old regression by using lookup_commit_reference_by_name()
>      +    when parsing <B>. The scripted version did not need to peel the tag
>      +    because the commands it passed the tag to (e.g 'git reset') peeled the
>      +    tag themselves.
>
>           Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>           ---
>
> --
> gitgitgadget

^ permalink raw reply	[flat|nested] 55+ messages in thread

end of thread, other threads:[~2021-09-24  1:27 UTC | newest]

Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-08  9:49 [PATCH 00/11] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
2021-09-08 10:41   ` Ævar Arnfjörð Bjarmason
2021-09-08  9:49 ` [PATCH 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
2021-09-08 10:39   ` Ævar Arnfjörð Bjarmason
2021-09-08  9:49 ` [PATCH 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
2021-09-08 10:40   ` Ævar Arnfjörð Bjarmason
2021-09-08 13:42     ` Phillip Wood
2021-09-08  9:49 ` [PATCH 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
2021-09-08 10:42   ` Ævar Arnfjörð Bjarmason
2021-09-08  9:49 ` [PATCH 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
2021-09-09 10:35   ` Johannes Schindelin
2021-09-08  9:49 ` [PATCH 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
2021-09-08  9:49 ` [PATCH 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-08 10:45   ` Ævar Arnfjörð Bjarmason
2021-09-13 15:19 ` [PATCH v2 00/11] " Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 01/11] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 02/11] t3407: use test_commit Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 03/11] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 04/11] t3407: rename a variable Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 05/11] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 06/11] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 07/11] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 08/11] rebase: remove redundant strbuf Phillip Wood via GitGitGadget
2021-09-13 18:34     ` René Scharfe
2021-09-13 22:40       ` Junio C Hamano
2021-09-14 10:31         ` Phillip Wood
2021-09-14 10:33       ` Phillip Wood
2021-09-13 15:19   ` [PATCH v2 09/11] rebase: use our standard error return value Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 10/11] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
2021-09-13 15:19   ` [PATCH v2 11/11] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-13 22:58     ` Junio C Hamano
2021-09-14 10:17       ` Phillip Wood
2021-09-14 13:27         ` Phillip Wood
2021-09-14 16:29           ` Junio C Hamano
2021-09-14  3:42     ` Elijah Newren
2021-09-14  9:48     ` Sergey Organov
2021-09-14  9:58       ` Phillip Wood
2021-09-14  4:02   ` [PATCH v2 00/11] " Elijah Newren
2021-09-21 10:23   ` [PATCH v3 00/10] " Phillip Wood via GitGitGadget
2021-09-21 10:23     ` [PATCH v3 01/10] t3407: run tests in $TEST_DIRECTORY Phillip Wood via GitGitGadget
2021-09-21 10:23     ` [PATCH v3 02/10] t3407: use test_commit Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 03/10] t3407: use test_cmp_rev Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 04/10] t3407: rename a variable Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 05/10] t3407: use test_path_is_missing Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 06/10] t3407: strengthen rebase --abort tests Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 07/10] t3407: rework rebase --quit tests Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 08/10] rebase: use our standard error return value Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 09/10] rebase: use lookup_commit_reference_by_name() Phillip Wood via GitGitGadget
2021-09-21 10:24     ` [PATCH v3 10/10] rebase: dereference tags Phillip Wood via GitGitGadget
2021-09-24  1:26     ` [PATCH v3 00/10] " Elijah Newren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.