[PATCH 0/5] rebase -m: fix --signoff with conflicts

[PATCH 0/5] rebase -m: fix --signoff with conflicts

[Phillip Wood]

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

When rebasing with "--signoff" the commit created by "rebase --continue"
after resolving conflicts or editing a commit fails to add the
"Signed-off-by:" trailer. This happens because the message from the
original commit is reused instead of the one that would have been used
if the sequencer had not stopped for the user interaction. This series
fixes that by introducing an strbuf to hold the message which we then
write to rebase_path_message() when stopping for user interaction.

The patches are structured as follows:

Patches 1–3 add private "struct replay_ctx" to "struct replay_opts"
and move the private members from the latter into the former. These
changes are largely mechanical.

Patch 4 adds strbuf to "struct replay_ctx" to hold the commit
message. This change is also largely mechanical.

Patch 5 fixes the bug by using the changes in patch 4 to write the
correct message to disc when stopping for user interaction.

This series is based on a merge of 'maint' and 'pw/t3428-cleanup'. In
principle it would be passible to avoid the refactoring in patches 1–3
and add a new member to "struct replay_opts" instead. However the
changes in those patches are largely mechanical so should be low-risk
and they pave the way for more improvements and bug fixes in the
future.


base-commit: 7bd541c104a2e383dd250f5f8e514785c4cabbc0
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-fix-signoff%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/7bd541c10...4c8f88437
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-fix-signoff/v1

Phillip Wood (5):
  sequencer: always free "struct replay_opts"
  sequencer: start removing private fields from public API
  sequencer: move current fixups to private context
  sequencer: store commit message in private context
  rebase -m: fix --signoff with conflicts

 sequencer.c               | 250 ++++++++++++++++++++++++--------------
 sequencer.h               |  11 +-
 t/t3428-rebase-signoff.sh |  96 ++++++++++++---
 t/t3434-rebase-i18n.sh    |   2 +-
 4 files changed, 243 insertions(+), 116 deletions(-)


base-commit: 7bd541c104a2e383dd250f5f8e514785c4cabbc0
-- 
2.44.0.661.ge68cfcc6c2f

[PATCH 1/5] sequencer: always free "struct replay_opts"

[Phillip Wood]

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

sequencer_post_commit_cleanup() initializes an instance of "struct
replay_opts" but does not call replay_opts_release(). Currently this
does not leak memory because the code paths called don't allocate any of
the struct members. That will change in the next commit so add call to
replay_opts_release() to prevent a memory leak in "git commit" that
breaks all of the leak free tests.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..e4146b4cdfa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2823,12 +2823,14 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 			NULL, REF_NO_DEREF);
 
 	if (!need_cleanup)
-		return;
+		goto out;
 
 	if (!have_finished_the_last_pick())
-		return;
+		goto out;
 
 	sequencer_remove_state(&opts);
+out:
+	replay_opts_release(&opts);
 }
 
 static void todo_list_write_total_nr(struct todo_list *todo_list)
-- 
2.44.0.661.ge68cfcc6c2f

[PATCH 2/5] sequencer: start removing private fields from public API

[Phillip Wood]

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

"struct replay_opts" has a number of fields that are for internal
use. While they are marked as private having them in a public struct is
a distraction for callers and means that every time the internal details
are changed we have to recompile all the files that include sequencer.h
even though the public API is unchanged. This commit starts the process
of removing the private fields by adding an opaque pointer to a "struct
replay_ctx" to "struct replay_opts" and moving the "reflog_message"
member to the new private struct.

The sequencer currently updates the state files on disc each time it
processes a command in the todo list. This is an artifact of the
scripted implementation and makes the code hard to reason about as it is
not possible to get a complete view of the state in memory. In the
future we will add new members to "struct replay_ctx" to remedy this and
avoid writing state to disc unless the sequencer stops for user
interaction.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 36 ++++++++++++++++++++++++++++++------
 sequencer.h |  6 +++++-
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e4146b4cdfa..6ddf6a8aa1e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -207,6 +207,24 @@ static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-res
 static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
 static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
 
+/*
+ * A 'struct replay_ctx' represents the private state of the sequencer.
+ */
+struct replay_ctx {
+	/*
+	 * Stores the reflog message that will be used when creating a
+	 * commit. Points to a static buffer and should not be free()'d.
+	 */
+	const char *reflog_message;
+};
+
+struct replay_ctx* replay_ctx_new(void)
+{
+	struct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));
+
+	return ctx;
+}
+
 /**
  * A 'struct update_refs_record' represents a value in the update-refs
  * list. We use a string_list to map refs to these (before, after) pairs.
@@ -377,6 +395,7 @@ void replay_opts_release(struct replay_opts *opts)
 	if (opts->revs)
 		release_revisions(opts->revs);
 	free(opts->revs);
+	free(opts->ctx);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
@@ -1054,6 +1073,7 @@ static int run_git_commit(const char *defmsg,
 			  struct replay_opts *opts,
 			  unsigned int flags)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
@@ -1071,7 +1091,7 @@ static int run_git_commit(const char *defmsg,
 			     gpg_opt, gpg_opt);
 	}
 
-	strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", opts->reflog_message);
+	strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", ctx->reflog_message);
 
 	if (opts->committer_date_is_author_date)
 		strvec_pushf(&cmd.env, "GIT_COMMITTER_DATE=%s",
@@ -1457,6 +1477,7 @@ static int try_to_commit(struct repository *r,
 			 struct replay_opts *opts, unsigned int flags,
 			 struct object_id *oid)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	struct object_id tree;
 	struct commit *current_head = NULL;
 	struct commit_list *parents = NULL;
@@ -1618,7 +1639,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	if (update_head_with_reflog(current_head, oid, opts->reflog_message,
+	if (update_head_with_reflog(current_head, oid, ctx->reflog_message,
 				    msg, &err)) {
 		res = error("%s", err.buf);
 		goto out;
@@ -4725,11 +4746,12 @@ static int pick_one_commit(struct repository *r,
 			   struct replay_opts *opts,
 			   int *check_todo, int* reschedule)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	int res;
 	struct todo_item *item = todo_list->items + todo_list->current;
 	const char *arg = todo_item_get_arg(todo_list, item);
 	if (is_rebase_i(opts))
-		opts->reflog_message = reflog_message(
+		ctx->reflog_message = reflog_message(
 			opts, command_to_string(item->command), NULL);
 
 	res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
@@ -4786,9 +4808,10 @@ static int pick_commits(struct repository *r,
 			struct todo_list *todo_list,
 			struct replay_opts *opts)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	int res = 0, reschedule = 0;
 
-	opts->reflog_message = sequencer_reflog_action(opts);
+	ctx->reflog_message = sequencer_reflog_action(opts);
 	if (opts->allow_ff)
 		assert(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
@@ -5205,6 +5228,7 @@ static int commit_staged_changes(struct repository *r,
 
 int sequencer_continue(struct repository *r, struct replay_opts *opts)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	struct todo_list todo_list = TODO_LIST_INIT;
 	int res;
 
@@ -5224,7 +5248,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
 			unlink(rebase_path_dropped());
 		}
 
-		opts->reflog_message = reflog_message(opts, "continue", NULL);
+		ctx->reflog_message = reflog_message(opts, "continue", NULL);
 		if (commit_staged_changes(r, opts, &todo_list)) {
 			res = -1;
 			goto release_todo_list;
@@ -5276,7 +5300,7 @@ static int single_pick(struct repository *r,
 			TODO_PICK : TODO_REVERT;
 	item.commit = cmit;
 
-	opts->reflog_message = sequencer_reflog_action(opts);
+	opts->ctx->reflog_message = sequencer_reflog_action(opts);
 	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }
 
diff --git a/sequencer.h b/sequencer.h
index dcef7bb99c0..069f06400d2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -29,6 +29,9 @@ enum commit_msg_cleanup_mode {
 	COMMIT_MSG_CLEANUP_ALL
 };
 
+struct replay_ctx;
+struct replay_ctx* replay_ctx_new(void);
+
 struct replay_opts {
 	enum replay_action action;
 
@@ -78,13 +81,14 @@ struct replay_opts {
 	struct rev_info *revs;
 
 	/* Private use */
-	const char *reflog_message;
+	struct replay_ctx *ctx;
 };
 #define REPLAY_OPTS_INIT {			\
 	.edit = -1,				\
 	.action = -1,				\
 	.current_fixups = STRBUF_INIT,		\
 	.xopts = STRVEC_INIT,			\
+	.ctx = replay_ctx_new(),		\
 }
 
 /*
-- 
2.44.0.661.ge68cfcc6c2f

[PATCH 3/5] sequencer: move current fixups to private context

[Phillip Wood]

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

The list of current fixups is an implementation detail of the sequencer
and so it should not be stored in the public options struct.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 89 ++++++++++++++++++++++++++++++++++-------------------
 sequencer.h |  5 ---
 2 files changed, 57 insertions(+), 37 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 6ddf6a8aa1e..edc49c94cce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -211,17 +211,29 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redu
  * A 'struct replay_ctx' represents the private state of the sequencer.
  */
 struct replay_ctx {
+	/*
+	 * The list of completed fixup and squash commands in the
+	 * current chain.
+	 */
+	struct strbuf current_fixups;
 	/*
 	 * Stores the reflog message that will be used when creating a
 	 * commit. Points to a static buffer and should not be free()'d.
 	 */
 	const char *reflog_message;
+	/*
+	 * The number of completed fixup and squash commands in the
+	 * current chain.
+	 */
+	int current_fixup_count;
 };
 
 struct replay_ctx* replay_ctx_new(void)
 {
 	struct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));
 
+	strbuf_init(&ctx->current_fixups, 0);
+
 	return ctx;
 }
 
@@ -384,17 +396,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
+static void replay_ctx_release(struct replay_ctx *ctx)
+{
+	strbuf_release(&ctx->current_fixups);
+}
+
 void replay_opts_release(struct replay_opts *opts)
 {
+	struct replay_ctx *ctx = opts->ctx;
+
 	free(opts->gpg_sign);
 	free(opts->reflog_action);
 	free(opts->default_strategy);
 	free(opts->strategy);
 	strvec_clear (&opts->xopts);
-	strbuf_release(&opts->current_fixups);
 	if (opts->revs)
 		release_revisions(opts->revs);
 	free(opts->revs);
+	replay_ctx_release(ctx);
 	free(opts->ctx);
 }
 
@@ -1876,10 +1895,10 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
 }
 
 /* Does the current fixup chain contain a squash command? */
-static int seen_squash(struct replay_opts *opts)
+static int seen_squash(struct replay_ctx *ctx)
 {
-	return starts_with(opts->current_fixups.buf, "squash") ||
-		strstr(opts->current_fixups.buf, "\nsquash");
+	return starts_with(ctx->current_fixups.buf, "squash") ||
+		strstr(ctx->current_fixups.buf, "\nsquash");
 }
 
 static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
@@ -1955,6 +1974,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 			 enum todo_command command, struct replay_opts *opts,
 			 unsigned flag)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	const char *fixup_msg;
 	size_t commented_len = 0, fixup_off;
 	/*
@@ -1963,21 +1983,21 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	 * squashing commit messages.
 	 */
 	if (starts_with(body, "amend!") ||
-	    ((command == TODO_SQUASH || seen_squash(opts)) &&
+	    ((command == TODO_SQUASH || seen_squash(ctx)) &&
 	     (starts_with(body, "squash!") || starts_with(body, "fixup!"))))
 		commented_len = commit_subject_length(body);
 
 	strbuf_addf(buf, "\n%c ", comment_line_char);
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
-		    ++opts->current_fixup_count + 1);
+		    ++ctx->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
 	strbuf_add_commented_lines(buf, body, commented_len, comment_line_char);
 	/* buf->buf may be reallocated so store an offset into the buffer */
 	fixup_off = buf->len;
 	strbuf_addstr(buf, body + commented_len);
 
 	/* fixup -C after squash behaves like squash */
-	if (is_fixup_flag(command, flag) && !seen_squash(opts)) {
+	if (is_fixup_flag(command, flag) && !seen_squash(ctx)) {
 		/*
 		 * We're replacing the commit message so we need to
 		 * append the Signed-off-by: trailer if the user
@@ -2011,12 +2031,13 @@ static int update_squash_messages(struct repository *r,
 				  struct replay_opts *opts,
 				  unsigned flag)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	struct strbuf buf = STRBUF_INIT;
 	int res = 0;
 	const char *message, *body;
 	const char *encoding = get_commit_output_encoding();
 
-	if (opts->current_fixup_count > 0) {
+	if (ctx->current_fixup_count > 0) {
 		struct strbuf header = STRBUF_INIT;
 		char *eol;
 
@@ -2029,10 +2050,10 @@ static int update_squash_messages(struct repository *r,
 
 		strbuf_addf(&header, "%c ", comment_line_char);
 		strbuf_addf(&header, _(combined_commit_msg_fmt),
-			    opts->current_fixup_count + 2);
+			    ctx->current_fixup_count + 2);
 		strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len);
 		strbuf_release(&header);
-		if (is_fixup_flag(command, flag) && !seen_squash(opts))
+		if (is_fixup_flag(command, flag) && !seen_squash(ctx))
 			update_squash_message_for_fixup(&buf);
 	} else {
 		struct object_id head;
@@ -2079,7 +2100,7 @@ static int update_squash_messages(struct repository *r,
 	} else if (command == TODO_FIXUP) {
 		strbuf_addf(&buf, "\n%c ", comment_line_char);
 		strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
-			    ++opts->current_fixup_count + 1);
+			    ++ctx->current_fixup_count + 1);
 		strbuf_addstr(&buf, "\n\n");
 		strbuf_add_commented_lines(&buf, body, strlen(body),
 					   comment_line_char);
@@ -2093,12 +2114,12 @@ static int update_squash_messages(struct repository *r,
 	strbuf_release(&buf);
 
 	if (!res) {
-		strbuf_addf(&opts->current_fixups, "%s%s %s",
-			    opts->current_fixups.len ? "\n" : "",
+		strbuf_addf(&ctx->current_fixups, "%s%s %s",
+			    ctx->current_fixups.len ? "\n" : "",
 			    command_to_string(command),
 			    oid_to_hex(&commit->object.oid));
-		res = write_message(opts->current_fixups.buf,
-				    opts->current_fixups.len,
+		res = write_message(ctx->current_fixups.buf,
+				    ctx->current_fixups.len,
 				    rebase_path_current_fixups(), 0);
 	}
 
@@ -2176,6 +2197,7 @@ static int do_pick_commit(struct repository *r,
 			  struct replay_opts *opts,
 			  int final_fixup, int *check_todo)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	unsigned int flags = should_edit(opts) ? EDIT_MSG : 0;
 	const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);
 	struct object_id head;
@@ -2456,8 +2478,8 @@ static int do_pick_commit(struct repository *r,
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());
 		unlink(rebase_path_current_fixups());
-		strbuf_reset(&opts->current_fixups);
-		opts->current_fixup_count = 0;
+		strbuf_reset(&ctx->current_fixups);
+		ctx->current_fixup_count = 0;
 	}
 
 leave:
@@ -3019,6 +3041,8 @@ static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf)
 
 static int read_populate_opts(struct replay_opts *opts)
 {
+	struct replay_ctx *ctx = opts->ctx;
+
 	if (is_rebase_i(opts)) {
 		struct strbuf buf = STRBUF_INIT;
 		int ret = 0;
@@ -3078,13 +3102,13 @@ static int read_populate_opts(struct replay_opts *opts)
 		read_strategy_opts(opts, &buf);
 		strbuf_reset(&buf);
 
-		if (read_oneliner(&opts->current_fixups,
+		if (read_oneliner(&ctx->current_fixups,
 				  rebase_path_current_fixups(),
 				  READ_ONELINER_SKIP_IF_EMPTY)) {
-			const char *p = opts->current_fixups.buf;
-			opts->current_fixup_count = 1;
+			const char *p = ctx->current_fixups.buf;
+			ctx->current_fixup_count = 1;
 			while ((p = strchr(p, '\n'))) {
-				opts->current_fixup_count++;
+				ctx->current_fixup_count++;
 				p++;
 			}
 		}
@@ -5066,6 +5090,7 @@ static int commit_staged_changes(struct repository *r,
 				 struct replay_opts *opts,
 				 struct todo_list *todo_list)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
 	unsigned int final_fixup = 0, is_clean;
 
@@ -5102,7 +5127,7 @@ static int commit_staged_changes(struct repository *r,
 		 * the commit message and if there was a squash, let the user
 		 * edit it.
 		 */
-		if (!is_clean || !opts->current_fixup_count)
+		if (!is_clean || !ctx->current_fixup_count)
 			; /* this is not the final fixup */
 		else if (!oideq(&head, &to_amend) ||
 			 !file_exists(rebase_path_stopped_sha())) {
@@ -5111,20 +5136,20 @@ static int commit_staged_changes(struct repository *r,
 				unlink(rebase_path_fixup_msg());
 				unlink(rebase_path_squash_msg());
 				unlink(rebase_path_current_fixups());
-				strbuf_reset(&opts->current_fixups);
-				opts->current_fixup_count = 0;
+				strbuf_reset(&ctx->current_fixups);
+				ctx->current_fixup_count = 0;
 			}
 		} else {
 			/* we are in a fixup/squash chain */
-			const char *p = opts->current_fixups.buf;
-			int len = opts->current_fixups.len;
+			const char *p = ctx->current_fixups.buf;
+			int len = ctx->current_fixups.len;
 
-			opts->current_fixup_count--;
+			ctx->current_fixup_count--;
 			if (!len)
 				BUG("Incorrect current_fixups:\n%s", p);
 			while (len && p[len - 1] != '\n')
 				len--;
-			strbuf_setlen(&opts->current_fixups, len);
+			strbuf_setlen(&ctx->current_fixups, len);
 			if (write_message(p, len, rebase_path_current_fixups(),
 					  0) < 0)
 				return error(_("could not write file: '%s'"),
@@ -5141,7 +5166,7 @@ static int commit_staged_changes(struct repository *r,
 			 * actually need to re-commit with a cleaned up commit
 			 * message.
 			 */
-			if (opts->current_fixup_count > 0 &&
+			if (ctx->current_fixup_count > 0 &&
 			    !is_fixup(peek_command(todo_list, 0))) {
 				final_fixup = 1;
 				/*
@@ -5214,14 +5239,14 @@ static int commit_staged_changes(struct repository *r,
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());
 	}
-	if (opts->current_fixup_count > 0) {
+	if (ctx->current_fixup_count > 0) {
 		/*
 		 * Whether final fixup or not, we just cleaned up the commit
 		 * message...
 		 */
 		unlink(rebase_path_current_fixups());
-		strbuf_reset(&opts->current_fixups);
-		opts->current_fixup_count = 0;
+		strbuf_reset(&ctx->current_fixups);
+		ctx->current_fixup_count = 0;
 	}
 	return 0;
 }
diff --git a/sequencer.h b/sequencer.h
index 069f06400d2..c15b9a64020 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -69,10 +69,6 @@ struct replay_opts {
 	/* Reflog */
 	char *reflog_action;
 
-	/* Used by fixup/squash */
-	struct strbuf current_fixups;
-	int current_fixup_count;
-
 	/* placeholder commit for -i --root */
 	struct object_id squash_onto;
 	int have_squash_onto;
@@ -86,7 +82,6 @@ struct replay_opts {
 #define REPLAY_OPTS_INIT {			\
 	.edit = -1,				\
 	.action = -1,				\
-	.current_fixups = STRBUF_INIT,		\
 	.xopts = STRVEC_INIT,			\
 	.ctx = replay_ctx_new(),		\
 }
-- 
2.44.0.661.ge68cfcc6c2f

[PATCH 4/5] sequencer: store commit message in private context

[Phillip Wood]

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

Add an strbuf to "struct replay_ctx" to hold the current commit
message. This does not change the behavior but it will allow us to fix a
bug with "git rebase --signoff" in the next commit. A future patch
series will use the changes here to avoid writing the commit message to
disc unless there are conflicts or the commit is being reworded.

The changes in do_pick_commit() are a mechanical replacement of "msgbuf"
with "ctx->message". In do_merge() the code to write commit message to
disc is factored out of the conditional now that both branches store the
message in the same buffer.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index edc49c94cce..31e4d3fdec0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -211,6 +211,11 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redu
  * A 'struct replay_ctx' represents the private state of the sequencer.
  */
 struct replay_ctx {
+	/*
+	 * The commit message that will be used except at the end of a
+	 * chain of fixup and squash commands.
+	 */
+	struct strbuf message;
 	/*
 	 * The list of completed fixup and squash commands in the
 	 * current chain.
@@ -226,13 +231,18 @@ struct replay_ctx {
 	 * current chain.
 	 */
 	int current_fixup_count;
+	/*
+	 * Whether message contains a commit message.
+	 */
+	unsigned have_message :1;
 };
 
 struct replay_ctx* replay_ctx_new(void)
 {
 	struct replay_ctx *ctx = xcalloc(1, sizeof(*ctx));
 
 	strbuf_init(&ctx->current_fixups, 0);
+	strbuf_init(&ctx->message, 0);
 
 	return ctx;
 }
@@ -399,6 +409,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 static void replay_ctx_release(struct replay_ctx *ctx)
 {
 	strbuf_release(&ctx->current_fixups);
+	strbuf_release(&ctx->message);
 }
 
 void replay_opts_release(struct replay_opts *opts)
@@ -2205,7 +2216,6 @@ static int do_pick_commit(struct repository *r,
 	const char *base_label, *next_label;
 	char *author = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
-	struct strbuf msgbuf = STRBUF_INIT;
 	int res, unborn = 0, reword = 0, allow, drop_commit;
 	enum todo_command command = item->command;
 	struct commit *commit = item->commit;
@@ -2304,7 +2314,7 @@ static int do_pick_commit(struct repository *r,
 		next = parent;
 		next_label = msg.parent_label;
 		if (opts->commit_use_reference) {
-			strbuf_addstr(&msgbuf,
+			strbuf_addstr(&ctx->message,
 				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
 		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
 			   /*
@@ -2313,21 +2323,21 @@ static int do_pick_commit(struct repository *r,
 			    * thus requiring excessive complexity to deal with.
 			    */
 			   !starts_with(orig_subject, "Revert \"")) {
-			strbuf_addstr(&msgbuf, "Reapply \"");
-			strbuf_addstr(&msgbuf, orig_subject);
+			strbuf_addstr(&ctx->message, "Reapply \"");
+			strbuf_addstr(&ctx->message, orig_subject);
 		} else {
-			strbuf_addstr(&msgbuf, "Revert \"");
-			strbuf_addstr(&msgbuf, msg.subject);
-			strbuf_addstr(&msgbuf, "\"");
+			strbuf_addstr(&ctx->message, "Revert \"");
+			strbuf_addstr(&ctx->message, msg.subject);
+			strbuf_addstr(&ctx->message, "\"");
 		}
-		strbuf_addstr(&msgbuf, "\n\nThis reverts commit ");
-		refer_to_commit(opts, &msgbuf, commit);
+		strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
+		refer_to_commit(opts, &ctx->message, commit);
 
 		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			refer_to_commit(opts, &msgbuf, parent);
+			strbuf_addstr(&ctx->message, ", reversing\nchanges made to ");
+			refer_to_commit(opts, &ctx->message, parent);
 		}
-		strbuf_addstr(&msgbuf, ".\n");
+		strbuf_addstr(&ctx->message, ".\n");
 	} else {
 		const char *p;
 
@@ -2336,21 +2346,22 @@ static int do_pick_commit(struct repository *r,
 		next = commit;
 		next_label = msg.label;
 
-		/* Append the commit log message to msgbuf. */
+		/* Append the commit log message to ctx->message. */
 		if (find_commit_subject(msg.message, &p))
-			strbuf_addstr(&msgbuf, p);
+			strbuf_addstr(&ctx->message, p);
 
 		if (opts->record_origin) {
-			strbuf_complete_line(&msgbuf);
-			if (!has_conforming_footer(&msgbuf, NULL, 0))
-				strbuf_addch(&msgbuf, '\n');
-			strbuf_addstr(&msgbuf, cherry_picked_prefix);
-			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-			strbuf_addstr(&msgbuf, ")\n");
+			strbuf_complete_line(&ctx->message);
+			if (!has_conforming_footer(&ctx->message, NULL, 0))
+				strbuf_addch(&ctx->message, '\n');
+			strbuf_addstr(&ctx->message, cherry_picked_prefix);
+			strbuf_addstr(&ctx->message, oid_to_hex(&commit->object.oid));
+			strbuf_addstr(&ctx->message, ")\n");
 		}
 		if (!is_fixup(command))
 			author = get_author(msg.message);
 	}
+	ctx->have_message = 1;
 
 	if (command == TODO_REWORD)
 		reword = 1;
@@ -2381,7 +2392,7 @@ static int do_pick_commit(struct repository *r,
 	}
 
 	if (opts->signoff && !is_fixup(command))
-		append_signoff(&msgbuf, 0, 0);
+		append_signoff(&ctx->message, 0, 0);
 
 	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
 		res = -1;
@@ -2390,17 +2401,17 @@ static int do_pick_commit(struct repository *r,
 		 !strcmp(opts->strategy, "ort") ||
 		 command == TODO_REVERT) {
 		res = do_recursive_merge(r, base, next, base_label, next_label,
-					 &head, &msgbuf, opts);
+					 &head, &ctx->message, opts);
 		if (res < 0)
 			goto leave;
 
-		res |= write_message(msgbuf.buf, msgbuf.len,
+		res |= write_message(ctx->message.buf, ctx->message.len,
 				     git_path_merge_msg(r), 0);
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		res = write_message(msgbuf.buf, msgbuf.len,
+		res = write_message(ctx->message.buf, ctx->message.len,
 				    git_path_merge_msg(r), 0);
 
 		commit_list_insert(base, &common);
@@ -2485,7 +2496,6 @@ static int do_pick_commit(struct repository *r,
 leave:
 	free_message(commit, &msg);
 	free(author);
-	strbuf_release(&msgbuf);
 	update_abort_safety_file();
 
 	return res;
@@ -3952,6 +3962,7 @@ static int do_merge(struct repository *r,
 		    const char *arg, int arg_len,
 		    int flags, int *check_todo, struct replay_opts *opts)
 {
+	struct replay_ctx *ctx = opts->ctx;
 	int run_commit_flags = 0;
 	struct strbuf ref_name = STRBUF_INIT;
 	struct commit *head_commit, *merge_commit, *i;
@@ -4080,40 +4091,31 @@ static int do_merge(struct repository *r,
 		write_author_script(message);
 		find_commit_subject(message, &body);
 		len = strlen(body);
-		ret = write_message(body, len, git_path_merge_msg(r), 0);
+		strbuf_add(&ctx->message, body, len);
 		repo_unuse_commit_buffer(r, commit, message);
-		if (ret) {
-			error_errno(_("could not write '%s'"),
-				    git_path_merge_msg(r));
-			goto leave_merge;
-		}
 	} else {
 		struct strbuf buf = STRBUF_INIT;
-		int len;
 
 		strbuf_addf(&buf, "author %s", git_author_info(0));
 		write_author_script(buf.buf);
-		strbuf_reset(&buf);
+		strbuf_release(&buf);
 
 		if (oneline_offset < arg_len) {
-			p = arg + oneline_offset;
-			len = arg_len - oneline_offset;
+			strbuf_add(&ctx->message, arg + oneline_offset,
+				   arg_len - oneline_offset);
 		} else {
-			strbuf_addf(&buf, "Merge %s '%.*s'",
+			strbuf_addf(&ctx->message, "Merge %s '%.*s'",
 				    to_merge->next ? "branches" : "branch",
 				    merge_arg_len, arg);
-			p = buf.buf;
-			len = buf.len;
-		}
-
-		ret = write_message(p, len, git_path_merge_msg(r), 0);
-		strbuf_release(&buf);
-		if (ret) {
-			error_errno(_("could not write '%s'"),
-				    git_path_merge_msg(r));
-			goto leave_merge;
 		}
 	}
+	ctx->have_message = 1;
+	if (write_message(ctx->message.buf, ctx->message.len,
+			  git_path_merge_msg(r), 0)) {
+		    ret = error_errno(_("could not write '%s'"),
+				      git_path_merge_msg(r));
+		    goto leave_merge;
+	}
 
 	if (strategy || to_merge->next) {
 		/* Octopus merge */
@@ -4885,6 +4887,8 @@ static int pick_commits(struct repository *r,
 				return stopped_at_head(r);
 			}
 		}
+		strbuf_reset(&ctx->message);
+		ctx->have_message = 0;
 		if (item->command <= TODO_SQUASH) {
 			res = pick_one_commit(r, todo_list, opts, &check_todo,
 					      &reschedule);
-- 
2.44.0.661.ge68cfcc6c2f

[PATCH 5/5] rebase -m: fix --signoff with conflicts

[Phillip Wood]

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

When rebasing with "--signoff" the commit created by "rebase --continue"
after resolving conflicts or editing a commit fails to add the
"Signed-off-by:" trailer. This happens because the message from the
original commit is reused instead of the one that would have been used
if the sequencer had not stopped for the user interaction. The correct
message is stored in ctx->message and so with a couple of exceptions
this is written to rebase_path_message() when stopping for user
interaction instead. The exceptions are (i) "fixup" and "squash"
commands where the file is written by error_failed_squash() and (ii)
"edit" commands that are fast-forwarded where the original message is
still reused. The latter is safe because "--signoff" will never
fast-forward.

Note this introduces a change in behavior as the message file now
contains conflict comments. This is safe because commit_staged_changes()
passes an explicit cleanup flag when not editing the message and when
the message is being edited it will be cleaned up automatically. This
means user now sees the same message comments in editor with "rebase
--continue" as they would if they ran "git commit" themselves before
continuing the rebase. It also matches the behavior of "git
cherry-pick", "git merge" etc. which all list the files with merge
conflicts.

The tests are extended to check that all commits made after continuing a
rebase have a "Signed-off-by:" trailer. Sadly there are a couple of
leaks in apply.c which I've not been able to track down that mean this
test file is no-longer leak free when testing "git rebase --apply
--signoff" with conflicts.

Reported-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c               | 23 +++++++---
 t/t3428-rebase-signoff.sh | 96 ++++++++++++++++++++++++++++++++-------
 t/t3434-rebase-i18n.sh    |  2 +-
 3 files changed, 97 insertions(+), 24 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 31e4d3fdec0..e29a01944be 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3636,13 +3636,24 @@ static int error_with_patch(struct repository *r,
 			    struct replay_opts *opts,
 			    int exit_code, int to_amend)
 {
-	if (commit) {
-		if (make_patch(r, commit, opts))
+	struct replay_ctx *ctx = opts->ctx;
+
+	/*
+	 * Write the commit message to be used by "git rebase
+	 * --continue". If a "fixup" or "squash" command has conflicts
+	 * then we will have already written rebase_path_message() in
+	 * error_failed_squash(). If an "edit" command was
+	 * fast-forwarded then we don't have a message in ctx->message
+	 * and rely on make_patch() to write rebase_path_message()
+	 * instead.
+	 */
+	if (ctx->have_message && !file_exists(rebase_path_message()) &&
+	    write_message(ctx->message.buf, ctx->message.len,
+			  rebase_path_message(), 0))
+		return error(_("could not write commit message file"));
+
+	if (commit && make_patch(r, commit, opts))
 			return -1;
-	} else if (copy_file(rebase_path_message(),
-			     git_path_merge_msg(r), 0666))
-		return error(_("unable to copy '%s' to '%s'"),
-			     git_path_merge_msg(r), rebase_path_message());
 
 	if (to_amend) {
 		if (intend_to_amend())
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 1bebd1ce74a..6f57aed9fac 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -5,12 +5,17 @@ test_description='git rebase --signoff
 This test runs git rebase --signoff and make sure that it works.
 '
 
-TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
 
 test_expect_success 'setup' '
 	git commit --allow-empty -m "Initial empty commit" &&
 	test_commit first file a &&
+	test_commit second file &&
+	git checkout -b conflict-branch first &&
+	test_commit file-2 file-2 &&
+	test_commit conflict file &&
+	test_commit third file &&
 
 	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
 
@@ -28,6 +33,22 @@ test_expect_success 'setup' '
 	Signed-off-by: $ident
 	EOF
 
+	# Expected commit message after conflict resolution for rebase --signoff
+	cat >expected-signed-conflict <<-EOF &&
+	third
+
+	Signed-off-by: $ident
+
+	conflict
+
+	Signed-off-by: $ident
+
+	file-2
+
+	Signed-off-by: $ident
+
+	EOF
+
 	# Expected commit message after rebase without --signoff (or with --no-signoff)
 	cat >expected-unsigned <<-EOF &&
 	first
@@ -39,8 +60,12 @@ test_expect_success 'setup' '
 # We configure an alias to do the rebase --signoff so that
 # on the next subtest we can show that --no-signoff overrides the alias
 test_expect_success 'rebase --apply --signoff adds a sign-off line' '
-	git rbs --apply HEAD^ &&
-	test_commit_message HEAD expected-signed
+	test_must_fail git rbs --apply second third &&
+	git checkout --theirs file &&
+	git add file &&
+	git rebase --continue &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-signed-conflict actual
 '
 
 test_expect_success 'rebase --no-signoff does not add a sign-off line' '
@@ -51,28 +76,65 @@ test_expect_success 'rebase --no-signoff does not add a sign-off line' '
 
 test_expect_success 'rebase --exec --signoff adds a sign-off line' '
 	test_when_finished "rm exec" &&
-	git commit --amend -m "first" &&
-	git rebase --exec "touch exec" --signoff HEAD^ &&
+	git rebase --exec "touch exec" --signoff first^ first &&
 	test_path_is_file exec &&
 	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase --root --signoff adds a sign-off line' '
-	git commit --amend -m "first" &&
+	git checkout first &&
 	git rebase --root --keep-empty --signoff &&
 	test_commit_message HEAD^ expected-initial-signed &&
 	test_commit_message HEAD expected-signed
 '
 
-test_expect_success 'rebase -i --signoff fails' '
-	git commit --amend -m "first" &&
-	git rebase -i --signoff HEAD^ &&
-	test_commit_message HEAD expected-signed
-'
-
-test_expect_success 'rebase -m --signoff fails' '
-	git commit --amend -m "first" &&
-	git rebase -m --signoff HEAD^ &&
-	test_commit_message HEAD expected-signed
-'
+test_expect_success 'rebase -m --signoff adds a sign-off line' '
+	test_must_fail git rebase -m --signoff second third &&
+	git checkout --theirs file &&
+	git add file &&
+	GIT_EDITOR="sed -n /Conflicts:/,/^\\\$/p >actual" \
+		git rebase --continue &&
+	cat >expect <<-\EOF &&
+	# Conflicts:
+	#	file
+
+	EOF
+	test_cmp expect actual &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-signed-conflict actual
+'
+
+test_expect_success 'rebase -i --signoff adds a sign-off line when editing commit' '
+	(
+		set_fake_editor &&
+		FAKE_LINES="edit 1 edit 3 edit 2" \
+			git rebase -i --signoff first third
+	) &&
+	echo a >a &&
+	git add a &&
+	test_must_fail git rebase --continue &&
+	git checkout --ours file &&
+	echo b >a &&
+	git add a file &&
+	git rebase --continue &&
+	echo c >a &&
+	git add a &&
+	git log --format=%B -n3 >actual &&
+	cat >expect <<-EOF &&
+	conflict
+
+	Signed-off-by: $ident
+
+	third
+
+	Signed-off-by: $ident
+
+	file-2
+
+	Signed-off-by: $ident
+
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t3434-rebase-i18n.sh b/t/t3434-rebase-i18n.sh
index e6fef696bbc..a4e482d2cd0 100755
--- a/t/t3434-rebase-i18n.sh
+++ b/t/t3434-rebase-i18n.sh
@@ -71,7 +71,7 @@ test_rebase_continue_update_encode () {
 		git config i18n.commitencoding $new &&
 		test_must_fail git rebase -m main &&
 		test -f .git/rebase-merge/message &&
-		git stripspace <.git/rebase-merge/message >two.t &&
+		git stripspace -s <.git/rebase-merge/message >two.t &&
 		git add two.t &&
 		git rebase --continue &&
 		compare_msg $msgfile $old $new &&
-- 
2.44.0.661.ge68cfcc6c2f

Re: [PATCH 2/5] sequencer: start removing private fields from public API

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> +/*
> + * A 'struct replay_ctx' represents the private state of the sequencer.
> + */
> +struct replay_ctx {
> +	/*
> +	 * Stores the reflog message that will be used when creating a
> +	 * commit. Points to a static buffer and should not be free()'d.
> +	 */
> +	const char *reflog_message;
> +};
> +
> +struct replay_ctx* replay_ctx_new(void)

Style.

Also if you intend to make the structure opaque and private,
shouldn't you make this static to make it truly internal?  The
initializer of the containing replay_opts structure would be the
only thing that needs to know how to instantiate it, no?  Or does a
replay_ctx has reason to exist outside the context of an instance of
replay_opts?

i.e.

	static struct replay_ctx *replay_ctx_new(void)

The remainder seem mechanical so I'll skip them.

Re: [PATCH 3/5] sequencer: move current fixups to private context

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The list of current fixups is an implementation detail of the sequencer
> and so it should not be stored in the public options struct.

It feels curious how 2/5 and 3/5 treat "fixups" and "reflog"
differently.

I would have expected them to be either in a single patch, or if we
really wanted to split them for readability, do a preliminary change
in 2/5 that argues that "fixups" does not really belong to _opts but
is a private state just like "reflog" and moves it from "/* Used by
fixup/squash */" section to the "/* Private use */" section at the
end, and then introduce the _ctx in 3/5 to move these three
together.

The end result would be the same, but I would have found the flow of
thought more straight-forward if presented that way.

Re: [PATCH 4/5] sequencer: store commit message in private context

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add an strbuf to "struct replay_ctx" to hold the current commit
> message. This does not change the behavior but it will allow us to fix a
> bug with "git rebase --signoff" in the next commit. A future patch
> series will use the changes here to avoid writing the commit message to
> disc unless there are conflicts or the commit is being reworded.

Is the machinery stopping due to conflicts the only reason why we
may want to write the message out to the filesystem?  I am wondering
if there are hooks that kick in before each commit gets picked, and
observe what the message being prepared to be used for the commit
is.

By the way, while I personally am fond of the spelling "disc",
nobody in the recent commit uses it (I saw another reference or two
in earlier steps of this series).  Perhaps "disc" -> "file", because
many filesystems are no longer on a spinning medium these days?

> The changes in do_pick_commit() are a mechanical replacement of "msgbuf"
> with "ctx->message". In do_merge() the code to write commit message to
> disc is factored out of the conditional now that both branches store the
> message in the same buffer.

Nice.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 50 insertions(+), 46 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index edc49c94cce..31e4d3fdec0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -211,6 +211,11 @@ static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redu
>   * A 'struct replay_ctx' represents the private state of the sequencer.
>   */
>  struct replay_ctx {
> +	/*
> +	 * The commit message that will be used except at the end of a
> +	 * chain of fixup and squash commands.
> +	 */
> +	struct strbuf message;
>  	/*
>  	 * The list of completed fixup and squash commands in the
>  	 * current chain.
> @@ -226,13 +231,18 @@ struct replay_ctx {
>  	 * current chain.
>  	 */
>  	int current_fixup_count;
> +	/*
> +	 * Whether message contains a commit message.
> +	 */
> +	unsigned have_message :1;
>  };

OK.  Having this member means we specifically allow message.len==0
to be a valid commit log message.

If it weren't for alignment and padding concern, it would have been
much nicer to have this member next to the .message member, and then
we do not even need such comment, as long as the name of the member
is clear enough (say, .message_valid).  Alas, we do want to have the
larger stuff near the front of the struct, and this member has to
sit near the end, so we need a comment to tell readers the linkage
between the two.  I still do not see a need for a 3-line comment for
this member, though ;-)

> +	ctx->have_message = 1;
> +	if (write_message(ctx->message.buf, ctx->message.len,
> +			  git_path_merge_msg(r), 0)) {
> +		    ret = error_errno(_("could not write '%s'"),
> +				      git_path_merge_msg(r));
> +		    goto leave_merge;
> +	}

As we are writing the in-core message to disc, we know they are in
sync, in other words, if we wanted to "read" the message, we know we
do not have to go back to disc and instead use the in-core version.

So, when do we *not* have .have_message member true?  Before we
found out the original message by reading the commit?  In this step,
the .have_message member is only set and not acted upon, so it is a
bit hard to see how it is meant to be used before moving to the next
step.  Let's see how it goes.

Thanks.

Re: [PATCH 5/5] rebase -m: fix --signoff with conflicts

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> +	struct replay_ctx *ctx = opts->ctx;
> +
> +	/*
> +	 * Write the commit message to be used by "git rebase
> +	 * --continue". If a "fixup" or "squash" command has conflicts
> +	 * then we will have already written rebase_path_message() in
> +	 * error_failed_squash(). If an "edit" command was
> +	 * fast-forwarded then we don't have a message in ctx->message
> +	 * and rely on make_patch() to write rebase_path_message()
> +	 * instead.
> +	 */
> +	if (ctx->have_message && !file_exists(rebase_path_message()) &&
> +	    write_message(ctx->message.buf, ctx->message.len,
> +			  rebase_path_message(), 0))
> +		return error(_("could not write commit message file"));

Makes the readers wonder if there are cases where we have written to
disc, but .have_message is true and the on-disc contents and in-core
contents are different.  If a codepath that writes to the file had
the message in-core, and it needs to tweak the message (e.g., add a
sign-off trailer) before writing it out to the file but forgot to do
so, then later we have tweaked the message in-core, such a bug would
result in the necessary "tweak" we have done not appear on disc.
Optionally keeping an in-core copy of what we have on disc does not
directly reduces the possibility of introducing such a bug, and I am
wondering if we can do anything clever about it.

Thanks.