On Thu, 14 Jan 2021 at 00:44, Taylor Blau <me@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > Mentored-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> > Signed-off-by: Charvi Mendiratta <charvi077@xxxxxxxxx>
> > ---
> > 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
