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

[Siddharth Asthana <siddharthasthana31@gmail.com>]

On 20/02/26 22:31, Toon Claes wrote:
> Siddharth Asthana <siddharthasthana31@gmail.com> 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
>> "<subject>"") and revert-of-revert cases ("Reapply "<subject>"").
>> 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 <siddharthasthana31@gmail.com>
>> ---
>>   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/
>