From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 08 Dec 2025 07:07:50 GMT
Subject: Re: [PATCH v2 1/2] sequencer: extract revert message formatting into shared function
Message-ID: <aTZ5RrjnwJ2ZnT7A@pks.im>
In-Reply-To: <ac12100d-4aba-4d15-8bcf-c50e6100c95e@gmail.com>

On Mon, Dec 08, 2025 at 04:30:58AM +0530, Siddharth Asthana wrote:
> 
> On 05/12/25 17:03, Patrick Steinhardt wrote:
> > On Wed, Dec 03, 2025 at 01:46:10AM +0530, Siddharth Asthana wrote:
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 5476d39ba9..9f621aef4b 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -2365,22 +2365,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);
> > >   		}
> > > -		strbuf_addstr(&ctx->message, "\nThis reverts commit ");
> > >   		refer_to_commit(opts, &ctx->message, commit);
> > >   		if (commit->parents && commit->parents->next) {
> > Is there any reason why we don't also handle `refer_to_commit()` in that
> > new function?
> 
> 
> The `refer_to_commit()` function depends on `struct replay_opts` and its
> `commit_use_reference` flag, which controls whether to use abbreviated
> commit info ("%h (%s, %ad)") or the full OID. This is specific to
> sequencer.c's interactive workflow where users can choose the reference
> style via --reference.
> 
> In replay.c, we always use the full OID via `oid_to_hex()` since it's
> designed for non-interactive server-side operations without the
> `replay_opts` framework. Including `refer_to_commit()` would require either
> passing `replay_opts` to the shared function (leaking sequencer internals)
> or adding a format parameter which feels like over-engineering for current
> needs.
> 
> Happy to reconsider if you think there's a cleaner way to share this.

A simple alternative might be to convert the `struct replay_opts`
parameter into a `flags` field that tells the function whether it is
expected to use the object ID or whether it should try using the
abbreviated commit info instead.

Patrick