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

Commit Message

Phillip Wood April 18, 2024, 1:14 p.m. UTC

From: Phillip Wood <[e-mail]>

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 <[e-mail]>
---
 sequencer.c | 96 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 50 insertions(+), 46 deletions(-)

Comments

Junio C Hamano April 18, 2024, 9:03 p.m. UTC | #1

Phillip Wood <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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.

Patch

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);