Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function
- From
Toon Claes <toon@iotcl.com>
- Date
- Feb 20, 2026, 17:01 UTC
- Message-ID
- <87wm07e4ck.fsf@iotcl.com>
- In-Reply-To
- <20260218234215.89326-2-siddharthasthana31@gmail.com>
Siddharth Asthana <siddharthasthana31@gmail.com> writes:
Show 55 quoted lines
> 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.
>
> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> ---
> 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