[WIP][PATCH] rebase: update the list of rewritten commits when amending pick

[Phillip Wood <phillip.wood123@gmail.com>]

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

If HEAD is amended during an exec command then the amended commit is missing
from the list of rewritten commits. The commonest way for this to happen is if a
commit is amended to fix a test failure when running `git rebase --exec "make
test"` but it can also happen if the exec command calls `git commit --amend`
directly. Amending commits with exec commands was discussed on the mailing list
recently where someone wanted to reset the author before submitting patches
upstream[1].

[1] https://public-inbox.org/git/CABPp-BEYRmhrb4Tx3bGzkx8y53T_0BYhLE5J0cEmxj18WtZs9A@mail.gmail.com/#t

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

This is what I've been using for a couple of years to update the list
of rewritten commits when running "git commit --amend" during a
rebase. I think it changes which commit gets used as the rewritten one
if you split a commit with 'edit', the patch is so old I cannot
remember if there were any other corner cases.


 builtin/commit.c             |   2 +-
 sequencer.c                  | 119 +++++++++++++++++++++++++++++------
 sequencer.h                  |   6 +-
 t/lib-rebase.sh              |   2 +-
 t/t5407-post-rewrite-hook.sh |  70 ++++++++++++++++++++-
 5 files changed, 176 insertions(+), 23 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index ae7aaf6dc6..9b6f3d8b6b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1697,7 +1697,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
 	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
-		commit_post_rewrite(the_repository, current_head, &oid);
+		commit_post_rewrite(the_repository, current_head, &oid, NULL);
 	}
 	if (!quiet) {
 		unsigned int flags = 0;
diff --git a/sequencer.c b/sequencer.c
index b395dd6e11..5d68e7341d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -129,6 +129,7 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
 static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
 static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 	"rebase-merge/rewritten-pending")
+static GIT_PATH_FUNC(rebase_path_rewritten_head, "rebase-merge/rewritten-head")
 
 /*
  * The path of the file containig the OID of the "squash onto" commit, i.e.
@@ -159,6 +160,9 @@ static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts")
 static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate")
 static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec")
 
+static void write_rewritten_head(struct object_id *rewritten_head);
+static void read_rewritten_head(struct object_id *rewritten_head);
+
 static int git_sequencer_config(const char *k, const char *v, void *cb)
 {
 	struct replay_opts *opts = cb;
@@ -953,12 +957,12 @@ static int run_git_commit(struct repository *r,
 			  unsigned int flags)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int res = 0;
 
 	if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
 		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
 		const char *author = NULL;
 		struct object_id root_commit, *cache_tree_oid;
-		int res = 0;
 
 		if (is_rebase_i(opts)) {
 			author = read_author_ident(&script);
@@ -1004,6 +1008,9 @@ static int run_git_commit(struct repository *r,
 			     gpg_opt, gpg_opt);
 	}
 
+	if (is_rebase_i(opts) && (flags & AMEND_MSG))
+		write_rewritten_head(&opts->rewritten_head);
+
 	argv_array_push(&cmd.args, "commit");
 
 	if (!(flags & VERIFY_MSG))
@@ -1032,9 +1039,14 @@ static int run_git_commit(struct repository *r,
 		argv_array_push(&cmd.args, "--allow-empty-message");
 
 	if (is_rebase_i(opts) && !(flags & EDIT_MSG))
-		return run_command_silent_on_success(&cmd);
+		res = run_command_silent_on_success(&cmd);
 	else
-		return run_command(&cmd);
+		res = run_command(&cmd);
+
+	if (is_rebase_i(opts) && !res && (flags & AMEND_MSG))
+		read_rewritten_head(&opts->rewritten_head);
+
+	return res;
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
@@ -1177,12 +1189,42 @@ static int run_rewrite_hook(const struct object_id *oldoid,
 	return finish_command(&proc);
 }
 
+static void update_rewritten(const struct repository *r,
+			     const struct object_id *old_head,
+			     const struct object_id *new_head,
+			     struct object_id *rewritten_head)
+{
+	struct object_id oid;
+
+	if (!rewritten_head) {
+		read_rewritten_head(&oid);
+		rewritten_head = &oid;
+	}
+	if (oideq(old_head, rewritten_head)) {
+		FILE *fp;
+		fp = fopen_or_warn(rebase_path_rewritten_list(), "a");
+		if (fp) {
+			fprintf(fp, "%s %s\n",
+			    oid_to_hex(old_head), oid_to_hex(new_head));
+			fclose(fp);
+		}
+		oidcpy(rewritten_head, new_head);
+	}
+	if (rewritten_head == &oid)
+		write_rewritten_head(rewritten_head);
+
+	return;
+}
+
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *old_head,
-			 const struct object_id *new_head)
+			 const struct object_id *new_head,
+			 struct object_id *rewritten_head)
 {
 	struct notes_rewrite_cfg *cfg;
 
+	update_rewritten(r, &old_head->object.oid, new_head, rewritten_head);
+
 	cfg = init_copy_notes_for_rewrite("amend");
 	if (cfg) {
 		/* we are amending, so old_head is not NULL */
@@ -1473,7 +1515,8 @@ static int try_to_commit(struct repository *r,
 	}
 
 	if (flags & AMEND_MSG)
-		commit_post_rewrite(r, current_head, oid);
+		commit_post_rewrite(r, current_head, oid,
+				    &opts->rewritten_head);
 
 out:
 	free_commit_extra_headers(extra);
@@ -1731,7 +1774,7 @@ static int update_squash_messages(struct repository *r,
 	return res;
 }
 
-static void flush_rewritten_pending(void)
+static void flush_rewritten_pending(struct object_id *rewritten_head)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id newoid;
@@ -1752,12 +1795,14 @@ static void flush_rewritten_pending(void)
 		}
 		fclose(out);
 		unlink(rebase_path_rewritten_pending());
+		oidcpy(rewritten_head, &newoid);
 	}
 	strbuf_release(&buf);
 }
 
 static void record_in_rewritten(struct object_id *oid,
-		enum todo_command next_command)
+				enum todo_command next_command,
+				struct object_id *rewritten_head)
 {
 	FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
 
@@ -1768,7 +1813,7 @@ static void record_in_rewritten(struct object_id *oid,
 	fclose(out);
 
 	if (!is_fixup(next_command))
-		flush_rewritten_pending();
+		flush_rewritten_pending(rewritten_head);
 }
 
 static int do_pick_commit(struct repository *r,
@@ -2510,6 +2555,11 @@ static int read_populate_opts(struct replay_opts *opts)
 	if (is_rebase_i(opts)) {
 		struct strbuf buf = STRBUF_INIT;
 
+		if (file_exists(rebase_path_rewritten_head()))
+			read_rewritten_head(&opts->rewritten_head);
+		else
+			opts->rewritten_head = null_oid;
+
 		if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
 			if (!starts_with(buf.buf, "-S"))
 				strbuf_reset(&buf);
@@ -3065,6 +3115,7 @@ static int error_with_patch(struct repository *r,
 			    struct replay_opts *opts,
 			    int exit_code, int to_amend)
 {
+	write_rewritten_head(&opts->rewritten_head);
 	if (commit) {
 		if (make_patch(r, commit, opts))
 			return -1;
@@ -3119,12 +3170,14 @@ static int error_failed_squash(struct repository *r,
 	return error_with_patch(r, commit, subject, subject_len, opts, 1, 0);
 }
 
-static int do_exec(struct repository *r, const char *command_line)
+static int do_exec(struct repository *r, const char *command_line,
+		   struct object_id *rewritten_head)
 {
 	struct argv_array child_env = ARGV_ARRAY_INIT;
 	const char *child_argv[] = { NULL, NULL };
 	int dirty, status;
 
+	write_rewritten_head(rewritten_head);
 	fprintf(stderr, "Executing: %s\n", command_line);
 	child_argv[0] = command_line;
 	argv_array_pushf(&child_env, "GIT_DIR=%s", absolute_path(get_git_dir()));
@@ -3133,6 +3186,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	status = run_command_v_opt_cd_env(child_argv, RUN_USING_SHELL, NULL,
 					  child_env.argv);
 
+	read_rewritten_head(rewritten_head);
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
 		return error(_("could not read index"));
@@ -3331,10 +3385,12 @@ static int do_reset(struct repository *r,
 		ret = error(_("could not write index"));
 	free((void *)desc.buffer);
 
-	if (!ret)
+	if (!ret) {
 		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
 						len, name), "HEAD", &oid,
 				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+		oidcpy(&opts->rewritten_head, &oid);
+	}
 
 	strbuf_release(&ref_name);
 	return ret;
@@ -3862,6 +3918,7 @@ static int pick_commits(struct repository *r,
 			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK) {
+				write_rewritten_head(&opts->rewritten_head);
 				if (!opts->verbose)
 					term_clear_line();
 				return stopped_at_head(r);
@@ -3900,7 +3957,8 @@ static int pick_commits(struct repository *r,
 			}
 			if (is_rebase_i(opts) && !res)
 				record_in_rewritten(&item->commit->object.oid,
-					peek_command(todo_list, 1));
+						    peek_command(todo_list, 1),
+						    &opts->rewritten_head);
 			if (res && is_fixup(item->command)) {
 				if (res == 1)
 					intend_to_amend();
@@ -3935,7 +3993,7 @@ static int pick_commits(struct repository *r,
 			if (!opts->verbose)
 				term_clear_line();
 			*end_of_arg = '\0';
-			res = do_exec(r, arg);
+			res = do_exec(r, arg, &opts->rewritten_head);
 			*end_of_arg = saved;
 
 			if (res) {
@@ -3965,7 +4023,8 @@ static int pick_commits(struct repository *r,
 				reschedule = 1;
 			else if (item->commit)
 				record_in_rewritten(&item->commit->object.oid,
-						    peek_command(todo_list, 1));
+						    peek_command(todo_list, 1),
+						    &opts->rewritten_head);
 			if (res > 0)
 				/* failed with merge conflicts */
 				return error_with_patch(r, item->commit,
@@ -4062,7 +4121,7 @@ static int pick_commits(struct repository *r,
 				log_tree_diff_flush(&log_tree_opt);
 			}
 		}
-		flush_rewritten_pending();
+		flush_rewritten_pending(&opts->rewritten_head);
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
 			struct child_process child = CHILD_PROCESS_INIT;
@@ -4299,7 +4358,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 
 		if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) &&
 		    !get_oid_committish(buf.buf, &oid))
-			record_in_rewritten(&oid, peek_command(&todo_list, 0));
+			record_in_rewritten(&oid, peek_command(&todo_list, 0),
+					    &opts->rewritten_head);
 		strbuf_release(&buf);
 	}
 
@@ -5024,7 +5084,8 @@ int check_todo_list_from_file(struct repository *r)
 /* skip picking commits whose parents are unchanged */
 static int skip_unnecessary_picks(struct repository *r,
 				  struct todo_list *todo_list,
-				  struct object_id *base_oid)
+				  struct object_id *base_oid,
+				  struct object_id *rewritten_head)
 {
 	struct object_id *parent_oid;
 	int i;
@@ -5062,8 +5123,15 @@ static int skip_unnecessary_picks(struct repository *r,
 		todo_list->current = 0;
 		todo_list->done_nr += i;
 
-		if (is_fixup(peek_command(todo_list, 0)))
-			record_in_rewritten(base_oid, peek_command(todo_list, 0));
+		if (is_fixup(peek_command(todo_list, 0))) {
+			record_in_rewritten(base_oid, peek_command(todo_list, 0),
+					    rewritten_head);
+			oidcpy(rewritten_head, &null_oid);
+		} else {
+			oidcpy(rewritten_head, base_oid);
+		}
+	} else {
+		oidcpy(rewritten_head, base_oid);
 	}
 
 	return 0;
@@ -5129,7 +5197,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		return -1;
 	}
 
-	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
+	if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid,
+						     &opts->rewritten_head)) {
 		todo_list_release(&new_todo);
 		return error(_("could not skip unnecessary pick commands"));
 	}
@@ -5315,3 +5384,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 
 	return 0;
 }
+static void write_rewritten_head(struct object_id *rewritten_head)
+{
+	const char *hex = oid_to_hex(rewritten_head);
+	write_message(hex, strlen(hex), rebase_path_rewritten_head(), 1);
+}
+
+static void read_rewritten_head(struct object_id *rewritten_head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	read_oneliner(&buf, rebase_path_rewritten_head(), 0);
+	get_oid(buf.buf, rewritten_head);
+}
diff --git a/sequencer.h b/sequencer.h
index 574260f621..91834cfe1c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -63,6 +63,9 @@ struct replay_opts {
 	struct object_id squash_onto;
 	int have_squash_onto;
 
+	/* Used to update rewritten-list */
+	struct object_id rewritten_head;
+
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
 };
@@ -188,7 +191,8 @@ int update_head_with_reflog(const struct commit *old_head,
 			    struct strbuf *err);
 void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
-			 const struct object_id *new_head);
+			 const struct object_id *new_head,
+			 struct object_id *rewritten_head);
 
 int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
 				 const char *commit);
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6d87961e41..9c0016848d 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -49,7 +49,7 @@ set_fake_editor () {
 		case $line in
 		pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m)
 			action="$line";;
-		exec_*|x_*|break|b)
+		exec_*|x_*|break|b|reset_*|t_*)
 			echo "$line" | sed 's/_/ /g' >> "$1";;
 		"#")
 			echo '# comment' >> "$1";;
diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index 7344253bfb..18773709a3 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -14,7 +14,11 @@ test_expect_success 'setup' '
 	git checkout A^0 &&
 	test_commit E bar E &&
 	test_commit F foo F &&
-	git checkout master
+	git checkout master &&
+
+	write_script amend-head <<-\EOS
+	git commit --amend --only --allow-empty -m "$1"
+	EOS
 '
 
 mkdir .git/hooks
@@ -263,4 +267,68 @@ test_expect_success 'git rebase -i (exec)' '
 	verify_hook_input
 '
 
+test_expect_success 'git rebase -i (exec amends commit)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	test_must_fail env FAKE_LINES="1 \
+		exec_./amend-head_edited-1a \
+		exec_./amend-head_edited-1b \
+		2 \
+		exec_false \
+		3 \
+		break" git rebase -i A &&
+	./amend-head edited-2 &&
+	git rebase --continue &&
+	./amend-head edited-3 &&
+	git rebase --continue &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n%s %s\n%s %s\n%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B        HEAD@{6} \
+				HEAD@{6} HEAD^^   \
+				C        HEAD@{4} \
+				HEAD@{4} HEAD^    \
+				D        HEAD@{2} \
+				HEAD@{2} HEAD) &&
+
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (exec amends onto)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="exec_./amend-head_edited 1 \
+		exec_git_commit_--allow-empty_-m_empty \
+		exec_./amend-head_edited-empty" git rebase -i B &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B HEAD^^ \
+				C HEAD^) &&
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (fixup after exec)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="1 exec_true fixup 2 squash 3" git rebase -i A &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse B        HEAD@{2} \
+				HEAD@{2} HEAD     \
+				C        HEAD     \
+				D        HEAD) &&
+	verify_hook_input
+'
+
+test_expect_success 'git rebase -i (exec after reset)' '
+	git reset --hard D &&
+	clear_hook_input &&
+	FAKE_LINES="reset_C \
+		exec_./amend-head_edited 3" git rebase -i A &&
+	echo rebase >expected.args &&
+	printf "%s %s\n%s %s\n" >expected.data \
+		$(git rev-parse C HEAD^ \
+				D HEAD) &&
+	verify_hook_input
+'
+
 test_done
-- 
2.30.1