From: Siddharth Asthana Date: Fri, 06 Mar 2026 04:31:03 GMT Subject: Re: [PATCH v3 1/2] sequencer: extract revert message formatting into shared function Message-ID: <71de4ade-62fd-4e66-b225-d87d3d5b97fe@gmail.com> In-Reply-To: <87wm07e4ck.fsf@iotcl.com> On 20/02/26 22:31, Toon Claes wrote: > 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]? You're right, the split is awkward. I tried to keep sequencer_format_revert_header() minimal so it didn't pull in replay_opts or refer_to_commit(), but the NULL oid path is confusing. Phillip posted a cleaner approach in his reply to this patch -- he moves everything (title, body, refer_to_commit, merge-parent handling) into one sequencer_format_revert_message() with a bool use_commit_reference. That eliminates the NULL oid entirely and addresses Patrick's suggestion at the same time. I will go with that for v4. > > [1]: https://lore.kernel.org/git/aTZ5RrjnwJ2ZnT7A@pks.im/ >