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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

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.

Thanks,
Siddharth


>
> Patrick