[PATCH 0/3] sequencer: fix cleanup of REBASE_HEAD and AUTO_MERGE

[PATCH 0/3] sequencer: fix cleanup of REBASE_HEAD and AUTO_MERGE

[Phillip Wood]

We had an error report [1] that REBASE_HEAD was still present after
a rebase had finished. While looking at that I noticed the same could
happen to AUTO_MERGE. These patches ensure those refs are removed by
"git rebase --continue" so that they are not left behind after the
rebase has finished.

Thanks to Long for the bug report and Matt for the confirmation

[1] https://lore.kernel.org/git/edfc868b-6be6-4871-a539-e70791db216d@mail.ru

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fsequencer-delete-REBASE_HEAD%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/7e34d8174...2fcb42a08
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/sequencer-delete-REBASE_HEAD/v1


Phillip Wood (3):
  test_ref_missing(): improve error message
  sequencer: remove REBASE_HEAD at the end of a rebase
  sequencer: remove AUTO_MERGE at the end of a rebase

 sequencer.c                | 13 ++++++++-----
 t/t3403-rebase-skip.sh     |  6 +++++-
 t/t3407-rebase-abort.sh    |  4 ++++
 t/t3418-rebase-continue.sh | 10 ++++++++--
 t/test-lib-functions.sh    |  6 ++++--
 5 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.56.0.336.g97505d19352

[PATCH 1/3] test_ref_missing(): improve error message

[Phillip Wood]

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

If the ref being checked exists, then test_ref_missing() does not print
the name of the ref in the error message. So, if there is more than
one call to test_ref_missing() in a test, it is hard to understand
which one failed by looking at the test output. Fix that by printing
the name of the ref that erroneously exists.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/test-lib-functions.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 09a41d51adc..b56b4f0a6c8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -292,18 +292,20 @@ test_ref_exists () {
 # a reference. This is preferable to `! test_ref_exists` as this function is
 # able to distinguish actually-missing references from other, generic errors.
 test_ref_missing () {
+	local refname
+	eval refname="\$$#"
 	test_ref_exists "$@"
 	case "$?" in
 	2)
 		# This is the good case.
 		return 0
 		;;
 	0)
-		echo >&4 "test_ref_missing: reference exists"
+		echo >&4 "test_ref_missing: '$refname' exists"
 		return 1
 		;;
 	*)
-		echo >&4 "test_ref_missing: generic error"
+		echo >&4 "test_ref_missing: generic error while checking '$refname'"
 		return 1
 		;;
 	esac
-- 
2.56.0.336.g97505d19352

[PATCH 2/3] sequencer: remove REBASE_HEAD at the end of a rebase

[Phillip Wood]

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

When "git rebase" stops for an "edit" command, a rescheduled command,
or for the user to resolve conflicts, it creates a ref called
REBASE_HEAD pointing to the commit that is being picked. The ref is
deleted at the start of each iteration of the loop that picks commits,
but not after the loop ends. That means if there are conflicts on the
last commit to be picked, REBASE_HEAD is left behind after the rebase
finishes. Fix that by deleting REBASE_HEAD where it is created in
do_commit() if the commit succeeds, and before we enter the loop in
pick_commits() so that it is removed when continuing a rebase after
a conflict resolution or failed commit.

Several existing tests are extended to check that REBASE_HEAD is
present when we stop for conflicts and is absent when the rebase
finishes.

Reported-by: Long 76 <long76.git@mail.ru>
Reported-by: Matt Hunter <m@lfurio.us>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                | 9 ++++++---
 t/t3403-rebase-skip.sh     | 4 +++-
 t/t3407-rebase-abort.sh    | 2 ++
 t/t3418-rebase-continue.sh | 8 ++++++--
 4 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 2ac78e15b89..2d6a5f525c7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1781,7 +1781,10 @@ static int do_commit(struct repository *r,
 		if (is_rebase_i(opts) && oid)
 			if (write_rebase_head(oid))
 			    return -1;
-		return run_git_commit(msg_file, reflog_action, opts, flags);
+		res = run_git_commit(msg_file, reflog_action, opts, flags);
+		if (!res && is_rebase_i(opts) && oid)
+			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
+					NULL, REF_NO_DEREF);
 	}
 
 	return res;
@@ -5178,6 +5181,8 @@ static int pick_commits(struct repository *r,
 	unlink(rebase_path_amend());
 	unlink(rebase_path_patch());
 	unlink(git_path_merge_labels(r));
+	refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD", NULL,
+			REF_NO_DEREF);
 
 	while (todo_list->current < todo_list->nr) {
 		struct todo_item *item = todo_list->items + todo_list->current;
@@ -5206,8 +5211,6 @@ static int pick_commits(struct repository *r,
 			unlink(git_path_merge_head(r));
 			refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
 					NULL, REF_NO_DEREF);
-			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
-					NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK) {
 				if (!opts->verbose)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index a1911c4a9d6..a2a5ac8ae01 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -97,10 +97,12 @@ test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
-test_expect_success 'skipping final pick removes .git/MERGE_MSG' '
+test_expect_success 'skipping final pick removes REBASE_HEAD & .git/MERGE_MSG' '
 	test_must_fail git rebase --onto hello reverted-goodbye^ \
 		reverted-goodbye &&
+	test_cmp_rev reverted-goodbye REBASE_HEAD &&
 	git rebase --skip &&
+	test_ref_missing REBASE_HEAD &&
 	test_path_is_missing .git/MERGE_MSG
 '
 
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 9f49c4228b6..1703b5ae9b5 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -35,7 +35,9 @@ testrebase() {
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$state_dir" &&
+		test_cmp_rev d REBASE_HEAD &&
 		git rebase --abort &&
+		test_ref_missing REBASE_HEAD &&
 		check_head &&
 		test_path_is_missing "$state_dir"
 	'
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 8c43ef64fce..f35cd130921 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -28,7 +28,9 @@ test_expect_success 'merge based rebase --continue with works with touched file'
 
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
 	test-tool chmtime =-60 F1 &&
-	git rebase --continue
+	test_cmp_rev commit-new-file-F2 REBASE_HEAD &&
+	git rebase --continue &&
+	test_ref_missing REBASE_HEAD
 '
 
 test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
@@ -37,8 +39,10 @@ test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
 	test_must_fail git rebase --onto main HEAD^ &&
 	git read-tree --reset -u HEAD &&
 	test_path_is_file .git/MERGE_MSG &&
+	test_cmp_rev commit-new-file-F2-on-topic-branch REBASE_HEAD &&
 	git rebase --continue &&
-	test_path_is_missing .git/MERGE_MSG
+	test_path_is_missing .git/MERGE_MSG &&
+	test_ref_missing REBASE_HEAD
 '
 
 test_expect_success 'apply based rebase --continue works with touched file' '
-- 
2.56.0.336.g97505d19352

[PATCH 3/3] sequencer: remove AUTO_MERGE at the end of a rebase

[Phillip Wood]

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

If there are conflicts when picking a commit, the merge machinery
creates a ref called AUTO_MERGE that the user can use to compare their
conflict resolution to the conflicted result. The sequencer tries to
remove that ref on every iteration of the loop that picks commits,
but it is only written if we stop for conflicts, so it only needs
to be removed when we enter the loop, not on every iteration. The
ref is not removed after the loop has finished, which means that if
the last pick has conflicts it is left behind after the rebase has
finished. Fix this by removing the ref before the start of the loop
in pick_commits() as the last commit did for REBASE_HEAD. The tests
modified by the last commit are extended to check AUTO_MERGE as well.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                | 4 ++--
 t/t3403-rebase-skip.sh     | 2 ++
 t/t3407-rebase-abort.sh    | 2 ++
 t/t3418-rebase-continue.sh | 2 ++
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 2d6a5f525c7..7a88b6530bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5181,6 +5181,8 @@ static int pick_commits(struct repository *r,
 	unlink(rebase_path_amend());
 	unlink(rebase_path_patch());
 	unlink(git_path_merge_labels(r));
+	refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE", NULL,
+			REF_NO_DEREF);
 	refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD", NULL,
 			REF_NO_DEREF);
 
@@ -5209,8 +5211,6 @@ static int pick_commits(struct repository *r,
 			}
 			unlink(rebase_path_author_script());
 			unlink(git_path_merge_head(r));
-			refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
-					NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK) {
 				if (!opts->verbose)
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index a2a5ac8ae01..d4fcfe567c4 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -100,8 +100,10 @@ test_debug 'gitk --all & sleep 1'
 test_expect_success 'skipping final pick removes REBASE_HEAD & .git/MERGE_MSG' '
 	test_must_fail git rebase --onto hello reverted-goodbye^ \
 		reverted-goodbye &&
+	git show-ref --verify AUTO_MERGE &&
 	test_cmp_rev reverted-goodbye REBASE_HEAD &&
 	git rebase --skip &&
+	test_ref_missing AUTO_MERGE &&
 	test_ref_missing REBASE_HEAD &&
 	test_path_is_missing .git/MERGE_MSG
 '
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 1703b5ae9b5..fdd9a2b9f61 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -35,8 +35,10 @@ testrebase() {
 		git reset --hard pre-rebase &&
 		test_must_fail git rebase$type main &&
 		test_path_is_dir "$state_dir" &&
+		git show-ref --verify AUTO_MERGE &&
 		test_cmp_rev d REBASE_HEAD &&
 		git rebase --abort &&
+		test_ref_missing AUTO_MERGE &&
 		test_ref_missing REBASE_HEAD &&
 		check_head &&
 		test_path_is_missing "$state_dir"
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index f35cd130921..1c26bf5de54 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -39,9 +39,11 @@ test_expect_success 'merge based rebase --continue removes .git/MERGE_MSG' '
 	test_must_fail git rebase --onto main HEAD^ &&
 	git read-tree --reset -u HEAD &&
 	test_path_is_file .git/MERGE_MSG &&
+	git show-ref --verify AUTO_MERGE &&
 	test_cmp_rev commit-new-file-F2-on-topic-branch REBASE_HEAD &&
 	git rebase --continue &&
 	test_path_is_missing .git/MERGE_MSG &&
+	test_ref_missing AUTO_MERGE &&
 	test_ref_missing REBASE_HEAD
 '
 
-- 
2.56.0.336.g97505d19352

Re: [PATCH 1/3] test_ref_missing(): improve error message

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the ref being checked exists, then test_ref_missing() does not print
> the name of the ref in the error message. So, if there is more than
> one call to test_ref_missing() in a test, it is hard to understand
> which one failed by looking at the test output. Fix that by printing
> the name of the ref that erroneously exists.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/test-lib-functions.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Makes sense.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 09a41d51adc..b56b4f0a6c8 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -292,18 +292,20 @@ test_ref_exists () {
>  # a reference. This is preferable to `! test_ref_exists` as this function is
>  # able to distinguish actually-missing references from other, generic errors.
>  test_ref_missing () {
> +	local refname
> +	eval refname="\$$#"

OK, test_ref_exists is defined immediately before, so it probably is
safe to assume that the last parameter is the refname argument like
this patch does.  And it would make the failure output much easier
to understand.

> -		echo >&4 "test_ref_missing: reference exists"
> +		echo >&4 "test_ref_missing: '$refname' exists"

Thanks.

Re: [PATCH 2/3] sequencer: remove REBASE_HEAD at the end of a rebase

[Junio C Hamano]

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

> Several existing tests are extended to check that REBASE_HEAD is
> present when we stop for conflicts and is absent when the rebase
> finishes.

Good.

> diff --git a/sequencer.c b/sequencer.c
> index 2ac78e15b89..2d6a5f525c7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1781,7 +1781,10 @@ static int do_commit(struct repository *r,
>  		if (is_rebase_i(opts) && oid)
>  			if (write_rebase_head(oid))
>  			    return -1;
> -		return run_git_commit(msg_file, reflog_action, opts, flags);
> +		res = run_git_commit(msg_file, reflog_action, opts, flags);
> +		if (!res && is_rebase_i(opts) && oid)
> +			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
> +					NULL, REF_NO_DEREF);
>  	}
>  
>  	return res;
> @@ -5178,6 +5181,8 @@ static int pick_commits(struct repository *r,
>  	unlink(rebase_path_amend());
>  	unlink(rebase_path_patch());
>  	unlink(git_path_merge_labels(r));
> +	refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD", NULL,
> +			REF_NO_DEREF);
>  
>  	while (todo_list->current < todo_list->nr) {
>  		struct todo_item *item = todo_list->items + todo_list->current;
> @@ -5206,8 +5211,6 @@ static int pick_commits(struct repository *r,
>  			unlink(git_path_merge_head(r));
>  			refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
>  					NULL, REF_NO_DEREF);
> -			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
> -					NULL, REF_NO_DEREF);
>  
>  			if (item->command == TODO_BREAK) {
>  				if (!opts->verbose)

The patch is a bit dense (not a fault of the patch, but the blame
largely lies on the overly long functions involved with multi-level
callchain), but the idea is that the existing removal in the loop
inside pick_commits() may not trigger if the user resumes (via
"--continue" or "--skip") with an exhausted todo_list and that would
leave the REBASE_HEAD file behind, so we remove at the beginning of
pick_commits() before entering the loop, and also in do_commit(),
which is called deep inside the callchain from the loop to handle
one step, immediately before it returns on successful commit.

Which makes perfect sense.  The new tests seems to cover these cases
adequately, too.

Thanks.

Re: [PATCH 3/3] sequencer: remove AUTO_MERGE at the end of a rebase

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If there are conflicts when picking a commit, the merge machinery
> creates a ref called AUTO_MERGE that the user can use to compare their
> conflict resolution to the conflicted result. The sequencer tries to
> remove that ref on every iteration of the loop that picks commits,
> but it is only written if we stop for conflicts, so it only needs
> to be removed when we enter the loop, not on every iteration. The
> ref is not removed after the loop has finished, which means that if
> the last pick has conflicts it is left behind after the rebase has
> finished. Fix this by removing the ref before the start of the loop
> in pick_commits() as the last commit did for REBASE_HEAD. The tests
> modified by the last commit are extended to check AUTO_MERGE as well.

Nice observation.  The patch is surprisingly short and to the point.

Overall looking very good.

Thanks.