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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 26/02/26 19:57, Phillip Wood wrote:
> 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, this is much cleaner. Moving refer_to_commit() and the 
merge-parent handling into the same function gets rid of the awkward 
NULL oid path that Toon and Junio pointed out.

I will split the refer_to_commit() signature change (adding struct 
repository *r) into a preparatory commit as you suggested, then have the 
second commit introduce the full sequencer_format_revert_message() helper.

For replay, I will call it with use_commit_reference=false -- that gives 
the full OID through refer_to_commit() directly, no special causing needed.


> 
> 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 */
>