diff mbox series

[v2,6/6] rebase -i: fix adding failed command to the todo list

Message ID a836b049b900fa9d7c03ed5426a28b5cc754d4c5.1682089075.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase -i: impove handling of failed commands | expand

Commit Message

Phillip Wood April 21, 2023, 2:57 p.m. UTC 
From: Phillip Wood <phillip.wood@dunelm.org.uk>

When rebasing commands are moved from the todo list in "git-rebase-todo"
to the "done" file (which is used by "git status" to show the recently
executed commands) just before they are executed. This means that if a
command fails because it would overwrite an untracked file it has to be
added back into the todo list before the rebase stops for the user to
fix the problem.

Unfortunately when a failed command is added back into the todo list
the command preceding it is erroneously appended to the "done" file.
This means that when rebase stops after "pick B" fails the "done"
file contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

Fix this by not updating the "done" file when adding a failed command
back into the "git-rebase-todo" file. A couple of the existing tests are
modified to improve their coverage as none of them trigger this bug or
check the "done" file.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 12 ++++++------
 t/t3404-rebase-interactive.sh | 27 +++++++++++++++++----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 39 insertions(+), 22 deletions(-)

Comments

Glen Choo June 21, 2023, 8:59 p.m. UTC | #1 
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> When rebasing commands are moved from the todo list in "git-rebase-todo"
> to the "done" file (which is used by "git status" to show the recently
> executed commands) just before they are executed. This means that if a
> command fails because it would overwrite an untracked file it has to be
> added back into the todo list before the rebase stops for the user to
> fix the problem.
>
> Unfortunately when a failed command is added back into the todo list
> the command preceding it is erroneously appended to the "done" file.
> This means that when rebase stops after "pick B" fails the "done"
> file contains
>
> 	pick A
> 	pick B
> 	pick A
>
> instead of
>
> 	pick A
> 	pick B

It's very interesting that an _earlier_ line would get appended on the
_other_ side, I'd expect that even if lines get duplicated, their
relative order would be preserved. I was not able to trace the sequence
of events that got us here, so I could not make a connection from that
to the solution. I didn't dig deeply into this patch either, so I will
refrain from commenting on the solution.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index a657167befd..653c19bc9c8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1276,18 +1276,23 @@ test_expect_success 'todo count' '
>  '
>  
>  test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
> -	git checkout --force branch2 &&
> +	git checkout --force A &&
>  	git clean -f &&
> +	cat >todo <<-EOF &&
> +	exec >file2
> +	pick $(git rev-parse B) B
> +	pick $(git rev-parse C) C
> +	pick $(git rev-parse D) D
> +	exec cat .git/rebase-merge/done >actual
> +	EOF
>  	(
> -		set_fake_editor &&
> -		FAKE_LINES="edit 1 2" git rebase -i A
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i A
>  	) &&
> -	test_cmp_rev HEAD F &&
> -	test_path_is_missing file6 &&
> -	>file6 &&
> -	test_must_fail git rebase --continue &&
> -	test_cmp_rev HEAD F &&
> -	rm file6 &&
> +	test_cmp_rev HEAD B &&
> +	head -n3 todo >expect &&
> +	test_cmp expect .git/rebase-merge/done &&
> +	rm file2 &&

I didn't look into how the test works, but I confirmed that it tests the
exact scenario described in the commit message.
diff mbox series

Patch

diff --git a/sequencer.c b/sequencer.c
index db2daecb23e..9769dde00e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3379,7 +3379,8 @@  give_advice:
 	return -1;
 }
 
-static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,
+		     int reschedule)
 {
 	struct lock_file todo_lock = LOCK_INIT;
 	const char *todo_path = get_todo_path(opts);
@@ -3389,7 +3390,7 @@  static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 	 * rebase -i writes "git-rebase-todo" without the currently executing
 	 * command, appending it to "done" instead.
 	 */
-	if (is_rebase_i(opts))
+	if (is_rebase_i(opts) && !reschedule)
 		next++;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
@@ -3402,7 +3403,7 @@  static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
 	if (commit_lock_file(&todo_lock) < 0)
 		return error(_("failed to finalize '%s'"), todo_path);
 
-	if (is_rebase_i(opts) && next > 0) {
+	if (is_rebase_i(opts) && !reschedule && next > 0) {
 		const char *done = rebase_path_done();
 		int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);
 		int ret = 0;
@@ -4716,7 +4717,7 @@  static int pick_commits(struct repository *r,
 		const char *arg = todo_item_get_arg(todo_list, item);
 		int check_todo = 0;
 
-		if (save_todo(todo_list, opts))
+		if (save_todo(todo_list, opts, reschedule))
 			return -1;
 		if (is_rebase_i(opts)) {
 			if (item->command != TODO_COMMENT) {
@@ -4797,8 +4798,7 @@  static int pick_commits(struct repository *r,
 			       get_item_line_length(todo_list,
 						    todo_list->current),
 			       get_item_line(todo_list, todo_list->current));
-			todo_list->current--;
-			if (save_todo(todo_list, opts))
+			if (save_todo(todo_list, opts, reschedule))
 				return -1;
 			if (item->commit)
 				write_rebase_head(&item->commit->object.oid);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a657167befd..653c19bc9c8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1276,18 +1276,23 @@  test_expect_success 'todo count' '
 '
 
 test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
-	git checkout --force branch2 &&
+	git checkout --force A &&
 	git clean -f &&
+	cat >todo <<-EOF &&
+	exec >file2
+	pick $(git rev-parse B) B
+	pick $(git rev-parse C) C
+	pick $(git rev-parse D) D
+	exec cat .git/rebase-merge/done >actual
+	EOF
 	(
-		set_fake_editor &&
-		FAKE_LINES="edit 1 2" git rebase -i A
+		set_replace_editor todo &&
+		test_must_fail git rebase -i A
 	) &&
-	test_cmp_rev HEAD F &&
-	test_path_is_missing file6 &&
-	>file6 &&
-	test_must_fail git rebase --continue &&
-	test_cmp_rev HEAD F &&
-	rm file6 &&
+	test_cmp_rev HEAD B &&
+	head -n3 todo >expect &&
+	test_cmp expect .git/rebase-merge/done &&
+	rm file2 &&
 	test_path_is_missing .git/rebase-merge/author-script &&
 	test_path_is_missing .git/rebase-merge/patch &&
 	test_path_is_missing .git/MERGE_MSG &&
@@ -1299,7 +1304,9 @@  test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
 	grep "error: you have staged changes in your working tree" err &&
 	git reset --hard HEAD &&
 	git rebase --continue &&
-	test_cmp_rev HEAD I
+	test_cmp_rev HEAD D &&
+	tail -n3 todo >>expect &&
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 18a0bc8fafb..86f4e0e4d6f 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -128,14 +128,24 @@  test_expect_success 'generate correct todo list' '
 '
 
 test_expect_success '`reset` refuses to overwrite untracked files' '
-	git checkout -b refuse-to-reset &&
+	git checkout B &&
 	test_commit dont-overwrite-untracked &&
-	git checkout @{-1} &&
-	: >dont-overwrite-untracked.t &&
-	echo "reset refs/tags/dont-overwrite-untracked" >script-from-scratch &&
+	cat >script-from-scratch <<-EOF &&
+	exec >dont-overwrite-untracked.t
+	pick $(git rev-parse B) B
+	reset refs/tags/dont-overwrite-untracked
+	pick $(git rev-parse C) C
+	exec cat .git/rebase-merge/done >actual
+	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
-	test_must_fail git rebase -ir HEAD &&
-	git rebase --abort
+	test_must_fail git rebase -ir A &&
+	test_cmp_rev HEAD B &&
+	head -n3 script-from-scratch >expect &&
+	test_cmp expect .git/rebase-merge/done &&
+	rm dont-overwrite-untracked.t &&
+	git rebase --continue &&
+	tail -n3 script-from-scratch >>expect &&
+	test_cmp expect actual
 '
 
 test_expect_success '`reset` rejects trees' '