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

[Toon Claes <toon@iotcl.com>]

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]?

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

-- 
Cheers,
Toon