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

From: Charvi Mendiratta <charvi077@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Junio C Hamano <gitster@pobox.com>,
	Johannes.Schindelin@gmx.de
Subject: Re: [RFC PATCH 5/9] sequencer: use const variable for commit message comments
Date: Thu, 14 Jan 2021 14:27:17 +0530	[thread overview]
Message-ID: <CAPSFM5dje9wi-aSmT+6+1jrxvB9+MHyQuRLY-zataWjy_UFdWA@mail.gmail.com> (raw)
In-Reply-To: <X/9GrVjBmnJUZcGn@nand.local>

On Thu, 14 Jan 2021 at 00:44, Taylor Blau <me@ttaylorr.com> wrote:
>
> 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.");
>

Okay, I will change it and write in the same line,

Also, I agree with Junio and Christian to use array instead of pointer
here as it will take the extra memory for pointer.

> 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 admit, I was not aware about the difference of _fmt or _str but now I got
this and will change it.

> 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 for the reviews,

Thanks and Regards,
Charvi

> Thanks,
> Taylor