Subject: Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function

From: Phillip Wood <[email-masked]>
Date: Thu, 26 Feb 2026 14:27:54 +0000

Hi Siddharth

On 18/02/2026 23:42, Siddharth Asthana wrote:
> The logic for formatting revert commit messages (handling "Revert" and
> "Reapply" cases) is currently duplicated between sequencer.c and will be
> needed by builtin/replay.c.
> 
> Extract this logic into a new sequencer_format_revert_header() function
> that can be shared. The function handles both regular reverts ("Revert
> "<subject>"") and revert-of-revert cases ("Reapply "<subject>"").
> When an oid is provided, the function appends the full commit hash and
> period; otherwise the caller should append the commit reference.
> 
> Update do_pick_commit() to use the new helper, eliminating code
> duplication while preserving the special handling for commit_use_reference.

I agree with the other comments that this ends up being a bit awkward, I think
something like the diff below which moves all of the revert message formatting
into a helper function would be a better approach. Note that I've also added a
repository argument to refer_to_commit(). You might want to do that in a
separate commit, but I think it is worth doing if we're adding more callers.
I've also just used a bool for the use_commit_reference flag, if we want to add
more flags in the future we can convert it to an unsigned int when we do that.

Thanks

Phillip

---- 8< ----
diff --git a/sequencer.c b/sequencer.c
index a3eb39bb252..30f6da6f959 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2198,21 +2198,55 @@ static int should_edit(struct replay_opts *opts) {
  	return opts->edit;
  }
  
-static void refer_to_commit(struct replay_opts *opts,
-			    struct strbuf *msgbuf, struct commit *commit)
+static void refer_to_commit(struct repository*r, struct strbuf *msgbuf,
+		const struct commit *commit, bool use_commit_reference)
  {
-	if (opts->commit_use_reference) {
+	if (use_commit_reference) {
  		struct pretty_print_context ctx = {
  			.abbrev = DEFAULT_ABBREV,
  			.date_mode.type = DATE_SHORT,
  		};
-		repo_format_commit_message(the_repository, commit,
+		repo_format_commit_message(r, commit,
  					   "%h (%s, %ad)", msgbuf, &ctx);
  	} else {
  		strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
  	}
  }
  
+void sequencer_format_revert_message(struct repository *r, const char *subject,
+			const struct commit *commit, const struct commit *parent,
+			bool use_commit_reference, struct strbuf *message)
+{
+	const char *orig_subject;
+
+	if (use_commit_reference) {
+			strbuf_commented_addf(message, comment_line_str,
+				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+		} else if (skip_prefix(subject, "Revert \"", &orig_subject) &&
+			   /*
+			    * We don't touch pre-existing repeated reverts, because
+			    * theoretically these can be nested arbitrarily deeply,
+			    * thus requiring excessive complexity to deal with.
+			    */
+			   !starts_with(orig_subject, "Revert \"")) {
+			strbuf_addstr(message, "Reapply \"");
+			strbuf_addstr(message, orig_subject);
+			strbuf_addstr(message, "\n");
+		} else {
+			strbuf_addstr(message, "Revert \"");
+			strbuf_addstr(message, subject);
+			strbuf_addstr(message, "\"\n");
+		}
+		strbuf_addstr(message, "\nThis reverts commit ");
+		refer_to_commit(r, message, commit, use_commit_reference);
+
+		if (commit->parents && commit->parents->next) {
+			strbuf_addstr(message, ", reversing\nchanges made to ");
+			refer_to_commit(r, message, parent, use_commit_reference);
+		}
+		strbuf_addstr(message, ".\n");
+}
+
  static const char *sequencer_reflog_action(struct replay_opts *opts)
  {
  	if (!opts->reflog_action) {
@@ -2356,38 +2390,13 @@ static int do_pick_commit(struct repository *r,
  	 */
  
  	if (command == TODO_REVERT) {
-		const char *orig_subject;
-
  		base = commit;
  		base_label = msg.label;
  		next = parent;
  		next_label = msg.parent_label;
-		if (opts->commit_use_reference) {
-			strbuf_commented_addf(&ctx->message, comment_line_str,
-				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
-		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
-			   /*
-			    * We don't touch pre-existing repeated reverts, because
-			    * theoretically these can be nested arbitrarily deeply,
-			    * thus requiring excessive complexity to deal with.
-			    */
-			   !starts_with(orig_subject, "Revert \"")) {
-			strbuf_addstr(&ctx->message, "Reapply \"");
-			strbuf_addstr(&ctx->message, orig_subject);
-			strbuf_addstr(&ctx->message, "\n");
-		} else {
-			strbuf_addstr(&ctx->message, "Revert \"");
-			strbuf_addstr(&ctx->message, msg.subject);
-			strbuf_addstr(&ctx->message, "\"\n");
-		}
-		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
-		refer_to_commit(opts, &ctx->message, commit);
-
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&ctx->message, ", reversing\nchanges made to ");
-			refer_to_commit(opts, &ctx->message, parent);
-		}
-		strbuf_addstr(&ctx->message, ".\n");
+		sequencer_format_revert_message(r,msg.subject, commit, parent,
+						opts->commit_use_reference,
+						&ctx->message);
  	} else {
  		const char *p;
  
diff --git a/sequencer.h b/sequencer.h
index 719684c8a9f..a61ec6d81d4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -271,4 +271,8 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
   */
  int sequencer_get_update_refs_state(const char *wt_dir, struct string_list *refs);
  
+void sequencer_format_revert_message(struct repository *r, const char *subject,
+				     const struct commit *commit, const struct commit *parent,
+				     bool use_commit_reference, struct strbuf *message);
+
  #endif /* SEQUENCER_H */