Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments

[Taylor Blau <me@ttaylorr.com>]

On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:
> This makes it easier to use and reuse the comments.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Charvi Mendiratta <charvi077@gmail.com>
> ---
>  sequencer.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index cdafc2e0e8..b9295b5a02 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1736,6 +1736,15 @@ static size_t subject_length(const char *body)
>  	return blank_line ? len : i;
>  }
>
> +static const char first_commit_msg_str[] =
> +N_("This is the 1st commit message:");
> +static const char nth_commit_msg_fmt[] =
> +N_("This is the commit message #%d:");
> +static const char skip_nth_commit_msg_fmt[] =
> +N_("The commit message #%d will be skipped:");
> +static const char combined_commit_msg_str[] =
> +N_("This is a combination of %d commits.");
> +

Two nit-picks here. The line break after the '=' is a little awkward,
since two of these lines are less than 80 characters combined, and the
other two are just slightly longer than 80 characters. The other nitpick
is that its typical to see 'char *foo' instead of 'char foo[]'.

So, I'd write these as:

    static const char *first_commit_msg_str = N_("This is the 1st commit message:");
    static const char *nth_commit_msg_fmt = N_("This is the commit message #%d:");
    static const char *skip_nth_commit_msg_fmt = N_("The commit message #%d will be skipped:");
    static const char *combined_commit_msg_str = N_("This is a combination of %d commits.");

I also noticed that you suffix these with _fmt or _str depending on
whether or not there are arguments that get filled in later on. That
makes 'combined_commit_msg_str' labeled incorrectly (it should be
'combined_commit_msg_fmt').

I'm curious to see where down the road these messages will be used over
again, but I'm sure that that is coming in subsequent patch(es).

 static void append_squash_message(struct strbuf *buf, const char *body,
 				  struct replay_opts *opts)
>  static void append_squash_message(struct strbuf *buf, const char *body,
>  				  struct replay_opts *opts)
>  {
> @@ -1745,7 +1754,7 @@ static void append_squash_message(struct strbuf *buf, const char *body,
>  	if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
>  		commented_len = subject_length(body);
>  	strbuf_addf(buf, "\n%c ", comment_line_char);
> -	strbuf_addf(buf, _("This is the commit message #%d:"),
> +	strbuf_addf(buf, _(nth_commit_msg_fmt),
>  		    ++opts->current_fixup_count + 1);

This and the three below uses are good, since they still translate
these messages. Nice.

Thanks,
Taylor