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

[Toon Claes <toon@iotcl.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
>
>> The logic for formatting revert commit messages (handling "Revert" and
>> "Reapply" cases, appending "This reverts commit <ref>.", and handling
>> merge-parent references) currently lives inline in do_pick_commit().
>> The upcoming replay --revert mode needs to reuse this logic.
>>
>> Extract all of this into a new sequencer_format_revert_message()
>> function. The function takes a repository, the subject line, commit,
>> parent, a use_commit_reference flag, and the output strbuf. It handles
>> both regular reverts ("Revert "<subject>"") and revert-of-revert cases
>> ("Reapply "<subject>""), and uses refer_to_commit() internally to
>> format the commit reference.
>>
>> Update refer_to_commit() to take a struct repository parameter instead
>> of relying on the_repository, and a bool instead of reading from
>> replay_opts directly. This makes it usable from the new shared function
>> without pulling in sequencer-specific state.

I wouldn't mind if you put removing the use of `the_repository` in a
separate commit.

>>
>> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>> ---
>>  sequencer.c | 78 +++++++++++++++++++++++++++++++----------------------
>>  sequencer.h | 14 ++++++++++
>>  2 files changed, 60 insertions(+), 32 deletions(-)
>
> Relative to the previous round, sequencer_format_revert_message()
> that does a bit more than sequencer_format_revert_header() we had
> makes the existing code easier to follow, even though the total
> codeflow amounts to the same thing.  A new caller that will use the
> function now has to do less.
>
> Also, even though this is an internal implementation detail,
> changing the list of parameters refer_to_commit() takes makes it
> easier to understand which part of the replay_opts structure is used
> (i.e., we only care about "do we use the commit reference, or not?"
> bit, and we have no interest in any other members of the struct).

Patrick suggested[1] to use flags, but it makes sense to keep it simple
for now.

[1]: https://lore.kernel.org/git/aTZ5RrjnwJ2ZnT7A@pks.im/

>
>> +/*
>> + * Formats a complete revert commit message following standard Git conventions.
>> + * Handles regular reverts ("Revert \"<subject>\""), revert of revert cases
>> + * ("Reapply \"<subject>\""), and the --reference style. Appends "This reverts
>> + * commit <ref>." using either the abbreviated or full commit reference
>> + * depending on use_commit_reference. Also handles merge-parent references.
>> + */

I think you're trying to put too much in the comments here. I would
suggest to be a bit more concise. Maybe something like:

/*
 * Format a revert commit message with appropriate "Revert" or "Reapply"
 * prefix and "This reverts commit <ref>." body. When use_commit_reference
 * is set, <ref> is an abbreviated hash with subject and date.
 */

>> +void sequencer_format_revert_message(struct repository *r,
>> +				     const char *subject,
>> +				     const struct commit *commit,
>> +				     const struct commit *parent,
>> +				     bool use_commit_reference,
>> +				     struct strbuf *message);
>> +
>>  #endif /* SEQUENCER_H */
>
> OK.
>

-- 
Cheers,
Toon