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

On Thu, 14 Jan 2021 at 00:44, Taylor Blau [off-list ref] wrote:

On Fri, Jan 08, 2021 at 02:53:43PM +0530, Charvi Mendiratta wrote:

quoted

This makes it easier to use and reuse the comments.

Mentored-by: Christian Couder [off-list ref]
Mentored-by: Phillip Wood [off-list ref]
Signed-off-by: Charvi Mendiratta <redacted>
---
 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)

quoted

 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