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

["Phillip Wood via GitGitGadget" <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