git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 7/7] rebase -i: fix adding failed command to the todo list

From
PGPhillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date
Sep 6, 2023, 15:22 UTC
Message-ID
<8d5f6d51e1959c7908ce268692c1ae9d693b93c5.1694013772.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
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

This happens because save_todo() updates the "done" file with the previous command whenever "git-rebase-todo" is updated. When we add the failed pick back into "git-rebase-todo" we do not want to update "done". Fix this by adding a "reschedule" parameter to save_todo() which prevents the "done" file from being updated 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 | 29 ++++++++++++++++++-----------
 t/t3430-rebase-merges.sh      | 22 ++++++++++++++++------
 3 files changed, 40 insertions(+), 23 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 38b0f213157..175addb7ca6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3397,7 +3397,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);
@@ -3407,7 +3408,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);
@@ -3420,7 +3421,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;
@@ -4733,7 +4734,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) {
@@ -4814,8 +4815,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 a8ad398956a..71da9c465a1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1277,19 +1277,24 @@ 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 &&
-	test_cmp_rev REBASE_HEAD I &&
-	rm file6 &&
+	test_cmp_rev HEAD B &&
+	test_cmp_rev REBASE_HEAD C &&
+	head -n3 todo >expect &&
+	test_cmp expect .git/rebase-merge/done &&
+	rm file2 &&
 	test_path_is_missing .git/rebase-merge/patch &&
 	echo changed >file1 &&
 	git add file1 &&
@@ -1297,7 +1302,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 804ff819782..0b0877b9846 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' '

-- 
gitgitgadget
Previous: Phillip Wood via GitGitGadgetNext: Junio C Hamano
Message 77 of 80 in “rebase -i: do not update "done" when rescheduling command”
  1. rebase -i: do not update "done" when rescheduling commandPhillip Wood via GitGitGadget, Mar 19, 2023
  2. Stefan HallerMar 20, 2023
  3. Junio C HamanoMar 20, 2023
  4. Phillip WoodMar 24, 2023
  5. Junio C HamanoMar 24, 2023
  6. Phillip WoodMar 24, 2023
  7. Johannes SchindelinMar 27, 2023
  8. Phillip WoodAug 3, 2023
  9. Johannes SchindelinAug 23, 2023
  10. 0/6 rebase -i: impove handling of failed commandsPhillip Wood via GitGitGadget, Apr 21, 2023
  11. 1/6 rebase -i: move unlink() callsPhillip Wood via GitGitGadget, Apr 21, 2023
  12. Junio C HamanoApr 21, 2023
  13. Phillip WoodApr 27, 2023
  14. 2/6 rebase -i: remove patch file after conflict resolutionPhillip Wood via GitGitGadget, Apr 21, 2023
  15. Junio C HamanoApr 21, 2023
  16. Phillip WoodApr 27, 2023
  17. Glen ChooJun 21, 2023
  18. Phillip WoodJul 14, 2023
  19. Junio C HamanoJul 14, 2023
  20. Phillip WoodJul 17, 2023
  21. 3/6 sequencer: factor out part of pick_commits()Phillip Wood via GitGitGadget, Apr 21, 2023
  22. Eric SunshineApr 21, 2023
  23. Junio C HamanoApr 21, 2023
  24. Phillip WoodApr 21, 2023
  25. Junio C HamanoApr 21, 2023
  26. 4/6 rebase --continue: refuse to commit after failed commandPhillip Wood via GitGitGadget, Apr 21, 2023
  27. Eric SunshineApr 21, 2023
  28. Junio C HamanoApr 21, 2023
  29. Glen ChooJun 21, 2023
  30. 5/6 rebase: fix rewritten list for failed pickPhillip Wood via GitGitGadget, Apr 21, 2023
  31. Glen ChooJun 21, 2023
  32. Phillip WoodJul 25, 2023
  33. Glen ChooJul 25, 2023
  34. Phillip WoodJul 26, 2023
  35. Glen ChooJul 26, 2023
  36. Phillip WoodJul 28, 2023
  37. 6/6 rebase -i: fix adding failed command to the todo listPhillip Wood via GitGitGadget, Apr 21, 2023
  38. Glen ChooJun 21, 2023
  39. Junio C HamanoApr 21, 2023
  40. Glen ChooJun 21, 2023
  41. 0/7 rebase -i: impove handling of failed commandsPhillip Wood via GitGitGadget, Aug 1, 2023
  42. 2/7 rebase -i: remove patch file after conflict resolutionPhillip Wood via GitGitGadget, Aug 1, 2023
  43. Junio C HamanoAug 1, 2023
  44. Phillip WoodAug 1, 2023
  45. 1/7 rebase -i: move unlink() callsPhillip Wood via GitGitGadget, Aug 1, 2023
  46. Junio C HamanoAug 1, 2023
  47. Phillip WoodAug 1, 2023
  48. Junio C HamanoAug 1, 2023
  49. 3/7 sequencer: use rebase_path_message()Phillip Wood via GitGitGadget, Aug 1, 2023
  50. Junio C HamanoAug 1, 2023
  51. Phillip WoodAug 1, 2023
  52. Junio C HamanoAug 2, 2023
  53. 4/7 sequencer: factor out part of pick_commits()Phillip Wood via GitGitGadget, Aug 1, 2023
  54. Johannes SchindelinAug 23, 2023
  55. 5/7 rebase: fix rewritten list for failed pickPhillip Wood via GitGitGadget, Aug 1, 2023
  56. Johannes SchindelinAug 23, 2023
  57. Phillip WoodSep 4, 2023
  58. 6/7 rebase --continue: refuse to commit after failed commandPhillip Wood via GitGitGadget, Aug 1, 2023
  59. Johannes SchindelinAug 23, 2023
  60. Phillip WoodSep 4, 2023
  61. Johannes SchindelinSep 5, 2023
  62. Junio C HamanoSep 5, 2023
  63. Phillip WoodSep 5, 2023
  64. 7/7 rebase -i: fix adding failed command to the todo listPhillip Wood via GitGitGadget, Aug 1, 2023
  65. Junio C HamanoAug 2, 2023
  66. Phillip WoodAug 3, 2023
  67. Phillip WoodAug 9, 2023
  68. Glen ChooAug 7, 2023
  69. Phillip WoodAug 9, 2023
  70. 0/7 rebase -i: impove handling of failed commandsPhillip Wood via GitGitGadget, Sep 6, 2023
  71. 3/7 sequencer: use rebase_path_message()Phillip Wood via GitGitGadget, Sep 6, 2023
  72. 2/7 rebase -i: remove patch file after conflict resolutionPhillip Wood via GitGitGadget, Sep 6, 2023
  73. 1/7 rebase -i: move unlink() callsPhillip Wood via GitGitGadget, Sep 6, 2023
  74. 4/7 sequencer: factor out part of pick_commits()Phillip Wood via GitGitGadget, Sep 6, 2023
  75. 5/7 rebase: fix rewritten list for failed pickPhillip Wood via GitGitGadget, Sep 6, 2023
  76. 6/7 rebase --continue: refuse to commit after failed commandPhillip Wood via GitGitGadget, Sep 6, 2023
  77. 7/7 rebase -i: fix adding failed command to the todo listPhillip Wood via GitGitGadget, Sep 6, 2023
  78. Junio C HamanoSep 6, 2023
  79. Johannes SchindelinSep 7, 2023
  80. Junio C HamanoSep 7, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.