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

[PATCH v2 5/6] rebase: fix rewritten list for failed pick

From
PGPhillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date
Apr 21, 2023, 14:57 UTC
Message-ID
<f8e64c1b631116367e6e68fcfde711b507a03a94.1682089075.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1492.v2.git.1682089074.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 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 the way this is done results in the failed pick being recorded as rewritten.

Fix this by not calling error_with_patch() for failed commands. The pick has failed so there is nothing to commit and therefore we do not want to set up the message file for committing staged changes when the rebase continues. This change means we no-longer write a patch for the failed command or display the error message printed by error_with_patch(). As the command has failed the patch isn't really useful in that case and REBASE_HEAD is still written so the user can inspect the commit associated with the failed command. Unless the user has disabled it we print an advice message that is more helpful than the message from error_with_patch(). If the advice is disabled the user will still see the messages from the merge machinery detailing the problem.

To simplify writing REBASE_HEAD in this case pick_one_commit() is modified to avoid duplicating the code that adds the failed command back into the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 19 +++++++------------
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 t/t3430-rebase-merges.sh      | 11 ++++++++---
 t/t5407-post-rewrite-hook.sh  | 11 +++++++++++
 4 files changed, 38 insertions(+), 15 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 55bf0a72c3a..db2daecb23e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4141,6 +4141,7 @@ static int do_merge(struct repository *r,
 		error(_("could not even attempt to merge '%.*s'"),
 		      merge_arg_len, arg);
 		unlink(rebase_path_author_script());
+		unlink(git_path_merge_msg(r));
 		goto leave_merge;
 	}
 	/*
@@ -4631,7 +4632,7 @@ N_("Could not execute the todo command\n"
 static int pick_one_commit(struct repository *r,
 			   struct todo_list *todo_list,
 			   struct replay_opts *opts,
-			   int *check_todo)
+			   int *check_todo, int* reschedule)
 {
 	int res;
 	struct todo_item *item = todo_list->items + todo_list->current;
@@ -4644,12 +4645,8 @@ static int pick_one_commit(struct repository *r,
 			     check_todo);
 	if (is_rebase_i(opts) && res < 0) {
 		/* Reschedule */
-		advise(_(rescheduled_advice),
-		       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))
-			return -1;
+		*reschedule = 1;
+		return -1;
 	}
 	if (item->command == TODO_EDIT) {
 		struct commit *commit = item->commit;
@@ -4749,7 +4746,8 @@ static int pick_commits(struct repository *r,
 			}
 		}
 		if (item->command <= TODO_SQUASH) {
-			res = pick_one_commit(r, todo_list, opts, &check_todo);
+			res = pick_one_commit(r, todo_list, opts, &check_todo,
+					      &reschedule);
 			if (!res && item->command == TODO_EDIT)
 				return 0;
 		} else if (item->command == TODO_EXEC) {
@@ -4803,10 +4801,7 @@ static int pick_commits(struct repository *r,
 			if (save_todo(todo_list, opts))
 				return -1;
 			if (item->commit)
-				return error_with_patch(r,
-							item->commit,
-							arg, item->arg_len,
-							opts, res, 0);
+				write_rebase_head(&item->commit->object.oid);
 		} else if (is_rebase_i(opts) && check_todo && !res &&
 			   reread_todo_if_changed(r, todo_list, opts)) {
 			return -1;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c1fe55dc2c1..a657167befd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1289,6 +1289,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
 	test_cmp_rev HEAD F &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/author-script &&
+	test_path_is_missing .git/rebase-merge/patch &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/rebase-merge/message &&
+	test_path_is_missing .git/rebase-merge/stopped-sha &&
 	echo changed >file1 &&
 	git add file1 &&
 	test_must_fail git rebase --continue 2>err &&
@@ -1313,6 +1317,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
 	test_cmp_rev HEAD F &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/author-script &&
+	test_path_is_missing .git/rebase-merge/patch &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/rebase-merge/message &&
+	test_path_is_missing .git/rebase-merge/stopped-sha &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
 	git reset --hard original-branch2
@@ -1332,6 +1340,10 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
 	rm file6 &&
 	test_path_is_missing .git/rebase-merge/author-script &&
+	test_path_is_missing .git/rebase-merge/patch &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/rebase-merge/message &&
+	test_path_is_missing .git/rebase-merge/stopped-sha &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I
 '
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 360ec787ffd..18a0bc8fafb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -167,16 +167,21 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	test_must_fail git rebase -ir HEAD &&
 	grep "^merge -C .* G$" .git/rebase-merge/done &&
 	grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
-	test_path_is_file .git/rebase-merge/patch &&
+	test_path_is_missing .git/rebase-merge/patch &&
 	test_path_is_missing .git/rebase-merge/author-script &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/rebase-merge/message &&
+	test_path_is_missing .git/rebase-merge/stopped-sha &&
 
 	: fail because of merge conflict &&
-	rm G.t .git/rebase-merge/patch &&
 	git reset --hard conflicting-G &&
 	test_must_fail git rebase --continue &&
 	! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo &&
 	test_path_is_file .git/rebase-merge/patch &&
-	test_path_is_file .git/rebase-merge/author-script
+	test_path_is_file .git/rebase-merge/author-script &&
+	test_path_is_file .git/MERGE_MSG &&
+	test_path_is_file .git/rebase-merge/message &&
+	test_path_is_file .git/rebase-merge/stopped-sha
 '
 
 test_expect_success 'failed `merge <branch>` does not crash' '
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 5f3ff051ca2..c490a5137fe 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -173,6 +173,17 @@ test_fail_interactive_rebase () {
 	)
 }
 
+test_expect_success 'git rebase with failed pick' '
+	test_fail_interactive_rebase "exec_>bar pick 1" --onto C A E &&
+	rm bar &&
+	git rebase --continue &&
+	echo rebase >expected.args &&
+	cat >expected.data <<-EOF &&
+	$(git rev-parse E) $(git rev-parse HEAD)
+	EOF
+	verify_hook_input
+'
+
 test_expect_success 'git rebase -i (unchanged)' '
 	git reset --hard D &&
 	clear_hook_input &&

-- 
gitgitgadget
Previous: Glen ChooNext: Glen Choo
Message 30 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.