From: Phillip Wood Date: Thu, 26 Feb 2026 14:27:54 GMT Subject: Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function Message-ID: In-Reply-To: <20260218234215.89326-2-siddharthasthana31@gmail.com> 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 > """) and revert-of-revert cases ("Reapply """). > 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 */