[PATCH v2 4/6] rebase --continue: refuse to commit after failed command

From: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date: Apr 21, 2023, 14:57 UTC
Message-ID: <9356d14b09a468d8ef2884cd7d76e59ec5c16691.1682089075.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.1492.v2.git.1682089074.gitgitgadget@gmail.com>

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

If a commit cannot be picked because it would overwrite an untracked file then "git rebase --continue" should refuse to commit any staged changes as the commit was not picked. Do this by using the existing check for a missing author script in run_git_commit() which prevents "rebase --continue" from committing staged changes after failed exec commands.

When fast-forwarding it is not necessary to write the author script as we're reusing an existing commit, not creating a new one. If a fast-forwarded commit is modified by an "edit" or "reword" command then the modification is committed with "git commit --amend" which reuses the author of the commit being amended so the author script is not needed. baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, 2021-08-20) changed run_git_commit() to allow a missing author script when rewording a commit. This changes extends that to allow a missing author script whenever the commit is being amended.

If we're not fast-forwarding then we must remove the author script if the pick fails.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                   | 10 +++++-----
 t/t3404-rebase-interactive.sh |  8 ++++++++
 t/t3430-rebase-merges.sh      |  4 +++-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 2d463818dd1..55bf0a72c3a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1055,7 +1055,7 @@ static int run_git_commit(const char *defmsg,
 
 	if (is_rebase_i(opts) &&
 	    ((opts->committer_date_is_author_date && !opts->ignore_date) ||
-	     !(!defmsg && (flags & AMEND_MSG))) &&
+	     !(flags & AMEND_MSG)) &&
 	    read_env_script(&cmd.env)) {
 		const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
@@ -2216,8 +2216,6 @@ static int do_pick_commit(struct repository *r,
 	if (opts->allow_ff && !is_fixup(command) &&
 	    ((parent && oideq(&parent->object.oid, &head)) ||
 	     (!parent && unborn))) {
-		if (is_rebase_i(opts))
-			write_author_script(msg.message);
 		res = fast_forward_to(r, &commit->object.oid, &head, unborn,
 			opts);
 		if (res || command != TODO_REWORD)
@@ -2324,9 +2322,10 @@ static int do_pick_commit(struct repository *r,
 		 command == TODO_REVERT) {
 		res = do_recursive_merge(r, base, next, base_label, next_label,
 					 &head, &msgbuf, opts);
-		if (res < 0)
+		if (res < 0) {
+			unlink(rebase_path_author_script());
 			goto leave;
-
+		}
 		res |= write_message(msgbuf.buf, msgbuf.len,
 				     git_path_merge_msg(r), 0);
 	} else {
@@ -4141,6 +4140,7 @@ static int do_merge(struct repository *r,
 	if (ret < 0) {
 		error(_("could not even attempt to merge '%.*s'"),
 		      merge_arg_len, arg);
+		unlink(rebase_path_author_script());
 		goto leave_merge;
 	}
 	/*
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff0afad63e2..c1fe55dc2c1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1288,6 +1288,12 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
 	test_must_fail git rebase --continue &&
 	test_cmp_rev HEAD F &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/author-script &&
+	echo changed >file1 &&
+	git add file1 &&
+	test_must_fail git rebase --continue 2>err &&
+	grep "error: you have staged changes in your working tree" err &&
+	git reset --hard HEAD &&
 	git rebase --continue &&
 	test_cmp_rev HEAD I
 '
@@ -1306,6 +1312,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)'
 	test_must_fail git rebase --continue &&
 	test_cmp_rev HEAD F &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/author-script &&
 	git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
 	git reset --hard original-branch2
@@ -1324,6 +1331,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
 	test_must_fail git rebase --continue &&
 	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
 	rm file6 &&
+	test_path_is_missing .git/rebase-merge/author-script &&
 	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 f03599c63b9..360ec787ffd 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -168,13 +168,15 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' '
 	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/author-script &&
 
 	: 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/patch &&
+	test_path_is_file .git/rebase-merge/author-script
 '
 
 test_expect_success 'failed `merge <branch>` does not crash' '

-- 
gitgitgadget