Re: [PATCH v2 1/2] sequencer: extract revert message formatting into shared function

> Patrick Steinhardt <ps@pks.im> writes:
>
>> 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.
> Even if it's non-interactive, I wonder if we should make it obey the
> config 'revert.reference' as well? To me it makes sense git-replay(1)
> and git-revert(1) give the same outcome if that config is set.

>
>>> 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.
> I was considering to add a bool for this option alone, but I agree flags
> is probably more future-proof.

>
> Patrick, I assume you don't mean to revamp the `struct replay_opts`
> completely, but only the parameter that would be passed into
> sequencer_format_revert_header() and refer_to_commit()?
>
> Siddharth, I see you have plenty of good reviews on this version of the
> series ([PATCH 2/2] in particular). I'd love to see you post v3. Or do
> you have any open questions you need answers to before you can send it
> out? Let me know if I can help with any decision-making.
>