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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 26/02/26 03:23, Junio C Hamano wrote:
> Toon Claes <toon@iotcl.com> writes:
> 
>>> -		} 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().
> 
> True.  Making sure plumbing commands are unaffected by random
> end-user configuration is a good thing, but I am not sure if this
> command is truly a plumbing.


With Phillip's sequencer_format_revert_message() approach, replay just 
passes use_commit_reference=false and gets the full OID path. The split 
logic goes away, so this concern is resolved regardless of how we 
classify replay.


> 
>> 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/