From: Toon Claes Date: Fri, 20 Feb 2026 17:01:47 GMT Subject: Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function Message-ID: <87wm07e4ck.fsf@iotcl.com> In-Reply-To: <20260218234215.89326-2-siddharthasthana31@gmail.com> Siddharth Asthana writes: > 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. > > Signed-off-by: Siddharth Asthana > --- > sequencer.c | 47 +++++++++++++++++++++++++++++++---------------- > sequencer.h | 11 +++++++++++ > 2 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 1f492f8460..b32347c853 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2356,8 +2356,6 @@ static int do_pick_commit(struct repository *r, > */ > > if (command == TODO_REVERT) { > - const char *orig_subject; > - > base = commit; > base_label = msg.label; > next = parent; > @@ -2365,22 +2363,10 @@ static int do_pick_commit(struct repository *r, > 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"); > + strbuf_addstr(&ctx->message, "\nThis reverts commit "); > } else { > - strbuf_addstr(&ctx->message, "Revert \""); > - strbuf_addstr(&ctx->message, msg.subject); > - strbuf_addstr(&ctx->message, "\"\n"); > + sequencer_format_revert_header(&ctx->message, msg.subject, NULL); > } > - strbuf_addstr(&ctx->message, "\nThis reverts commit "); > refer_to_commit(opts, &ctx->message, commit); I still find it somewhat confusing we have some the code that deals with `opts->commit_use_reference` partly in here and partly in sequencer_format_revert_header(). Part of the confusion comes from sequencer_format_revert_header() being called with NULL for the commit OID. Was is not possible to incorporate Patrick's suggestion[1]? [1]: https://lore.kernel.org/git/aTZ5RrjnwJ2ZnT7A@pks.im/ -- Cheers, Toon