From: Siddharth Asthana Date: Fri, 06 Mar 2026 05:00:54 GMT Subject: Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function Message-ID: In-Reply-To: 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 >> """) 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, 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 */ >