| Message ID | 20210108092345.2178-6-charvi077@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase -i: add options to fixup command | expand |
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
Taylor Blau <me@ttaylorr.com> writes: > 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 actually am OK with []; it saves sizeof(char*) bytes from each of the variable, doesn't it? > 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'). Good eyes. Thanks.
On Wed, Jan 13, 2021 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote: > > Taylor Blau <me@ttaylorr.com> writes: [...] > > 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 actually am OK with []; it saves sizeof(char*) bytes from each of > the variable, doesn't it? Yeah, that's my understanding too.
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
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."); + 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); strbuf_addstr(buf, "\n\n"); strbuf_add_commented_lines(buf, body, commented_len); @@ -1774,7 +1783,7 @@ static int update_squash_messages(struct repository *r, buf.buf : strchrnul(buf.buf, '\n'); strbuf_addf(&header, "%c ", comment_line_char); - strbuf_addf(&header, _("This is a combination of %d commits."), + strbuf_addf(&header, _(combined_commit_msg_str), opts->current_fixup_count + 2); strbuf_splice(&buf, 0, eol - buf.buf, header.buf, header.len); strbuf_release(&header); @@ -1801,9 +1810,9 @@ static int update_squash_messages(struct repository *r, } strbuf_addf(&buf, "%c ", comment_line_char); - strbuf_addf(&buf, _("This is a combination of %d commits."), 2); + strbuf_addf(&buf, _(combined_commit_msg_str), 2); strbuf_addf(&buf, "\n%c ", comment_line_char); - strbuf_addstr(&buf, _("This is the 1st commit message:")); + strbuf_addstr(&buf, _(first_commit_msg_str)); strbuf_addstr(&buf, "\n\n"); strbuf_addstr(&buf, body); @@ -1819,7 +1828,7 @@ static int update_squash_messages(struct repository *r, append_squash_message(&buf, body, opts); } else if (command == TODO_FIXUP) { strbuf_addf(&buf, "\n%c ", comment_line_char); - strbuf_addf(&buf, _("The commit message #%d will be skipped:"), + strbuf_addf(&buf, _(skip_nth_commit_msg_fmt), ++opts->current_fixup_count + 1); strbuf_addstr(&buf, "\n\n"); strbuf_add_commented_lines(&buf, body, strlen(body));
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(-)