| Message ID | cf4e913a5a01cfb9e9b8b83b222cd4647fbc0bf2.1547797620.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Turn git add-i into built-in | expand |
Hi Slavica I think this round is looking good I've got a couple of comments about the translation of the help text but everything else looks fine to me now. In future when you're posting a new version it's helpful CC the people who commented on the previous version(s). On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote: > From: Slavica Djukic <slawica92@hotmail.com> > > Implement show-help command in add-interactive.c and use it in > builtin add--helper.c. > > Use command name "show-help" instead of "help": add--helper is > builtin, hence add--helper --help would be intercepted by > handle_builtin and re-routed to the help command, without ever > calling cmd_add__helper(). > > Signed-off-by: Slavica Djukic <slawica92@hotmail.com> > --- > add-interactive.c | 23 +++++++++++++++++++++++ > add-interactive.h | 4 +++- > builtin/add--helper.c | 7 ++++++- > 3 files changed, 32 insertions(+), 2 deletions(-) > > diff --git a/add-interactive.c b/add-interactive.c > index c55d934186..76c3f4c3eb 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -244,3 +244,26 @@ void add_i_print_modified(void) > free(files); > hashmap_free(&s.file_map, 1); > } > + > +void add_i_show_help(void) > +{ > + const char *help_color = get_color(COLOR_HELP); > + color_fprintf(stdout, help_color, "%s%s", _("status"), > + N_(" - show paths with changes")); > + printf("\n"); There seems to be a bit of confusion with the translation of these messages. "status" does not want to be translated so it shouldn't be in _() - it can just go in the format string as can the indentation and the "\n" (or we could use color_fprintf_ln() to automatically add a newline at the end. N_() is used to mark static strings for translation so the gettext utilities pick up the text to be translated but (because initializes for static variables must be compile-time constants) does not do anything when the program runs - if you have 'const char *s = N_(hello);' you have to do '_(s)' to get the translated version. Here we can just pass the untranslated string directly to gettext so it should be _("show paths with changes"). Putting all that together we get color_fprintf(stdout, help_color, "status - %s\n", _("show paths with changes"); Best Wishes Phillip > + color_fprintf(stdout, help_color, "%s%s", _("update"), > + N_(" - add working tree state to the staged set of changes")); > + printf("\n"); > + color_fprintf(stdout, help_color, "%s%s", _("revert"), > + N_(" - revert staged set of changes back to the HEAD version")); > + printf("\n"); > + color_fprintf(stdout, help_color, "%s%s", _("patch"), > + N_(" - pick hunks and update selectively")); > + printf("\n"); > + color_fprintf(stdout, help_color, "%s%s", _("diff"), > + N_(" - view diff between HEAD and index")); > + printf("\n"); > + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), > + N_(" - add contents of untracked files to the staged set of changes")); > + printf("\n"); > +} > diff --git a/add-interactive.h b/add-interactive.h > index 1f4747553c..46e17c5c71 100644 > --- a/add-interactive.h > +++ b/add-interactive.h > @@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata); > > void add_i_print_modified(void); > > -#endif > \ No newline at end of file > +void add_i_show_help(void); > + > +#endif > diff --git a/builtin/add--helper.c b/builtin/add--helper.c > index 43545d9af5..a3b3a68b68 100644 > --- a/builtin/add--helper.c > +++ b/builtin/add--helper.c > @@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = { > > enum cmd_mode { > DEFAULT = 0, > - STATUS > + STATUS, > + HELP > }; > > int cmd_add__helper(int argc, const char **argv, const char *prefix) > @@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) > struct option options[] = { > OPT_CMDMODE(0, "status", &mode, > N_("print status information with diffstat"), STATUS), > + OPT_CMDMODE(0, "show-help", &mode, > + N_("show help"), HELP), > OPT_END() > }; > > @@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) > > if (mode == STATUS) > add_i_print_modified(); > + else if (mode == HELP) > + add_i_show_help(); > else > usage_with_options(builtin_add_helper_usage, > options); >
Hi Phillip, On 18-Jan-19 12:20 PM, Phillip Wood wrote: > Hi Slavica > > I think this round is looking good I've got a couple of comments about > the translation of the help text but everything else looks fine to me > now. In future when you're posting a new version it's helpful CC the > people who commented on the previous version(s). Thanks for taking your time to review patches again. I'm sorry for omitting you in CC, but I've sent re-roll through GitGitGadget, and I guess I thought it would pick it up. I'll see what happened and keep that in mind. > > On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote: >> From: Slavica Djukic <slawica92@hotmail.com> >> >> Implement show-help command in add-interactive.c and use it in >> builtin add--helper.c. >> >> Use command name "show-help" instead of "help": add--helper is >> builtin, hence add--helper --help would be intercepted by >> handle_builtin and re-routed to the help command, without ever >> calling cmd_add__helper(). >> >> Signed-off-by: Slavica Djukic <slawica92@hotmail.com> >> --- >> add-interactive.c | 23 +++++++++++++++++++++++ >> add-interactive.h | 4 +++- >> builtin/add--helper.c | 7 ++++++- >> 3 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/add-interactive.c b/add-interactive.c >> index c55d934186..76c3f4c3eb 100644 >> --- a/add-interactive.c >> +++ b/add-interactive.c >> @@ -244,3 +244,26 @@ void add_i_print_modified(void) >> free(files); >> hashmap_free(&s.file_map, 1); >> } >> + >> +void add_i_show_help(void) >> +{ >> + const char *help_color = get_color(COLOR_HELP); >> + color_fprintf(stdout, help_color, "%s%s", _("status"), >> + N_(" - show paths with changes")); >> + printf("\n"); > There seems to be a bit of confusion with the translation of these > messages. "status" does not want to be translated so it shouldn't be in > _() - it can just go in the format string as can the indentation and the > "\n" (or we could use color_fprintf_ln() to automatically add a newline > at the end. N_() is used to mark static strings for translation so the > gettext utilities pick up the text to be translated but (because > initializes for static variables must be compile-time constants) does > not do anything when the program runs - if you have 'const char *s = > N_(hello);' you have to do '_(s)' to get the translated version. Here we > can just pass the untranslated string directly to gettext so it should > be _("show paths with changes"). Putting all that together we get > > color_fprintf(stdout, help_color, "status - %s\n", > _("show paths with changes"); I thought _() was for strings that were already translated, and N_() for strings that weren't. And I now see that I also tried to translate command names as well, just the opposite of what you suggested... Thanks for clarifying this. > > > Best Wishes > > Phillip > >> + color_fprintf(stdout, help_color, "%s%s", _("update"), >> + N_(" - add working tree state to the staged set of changes")); >> + printf("\n"); >> + color_fprintf(stdout, help_color, "%s%s", _("revert"), >> + N_(" - revert staged set of changes back to the HEAD version")); >> + printf("\n"); >> + color_fprintf(stdout, help_color, "%s%s", _("patch"), >> + N_(" - pick hunks and update selectively")); >> + printf("\n"); >> + color_fprintf(stdout, help_color, "%s%s", _("diff"), >> + N_(" - view diff between HEAD and index")); >> + printf("\n"); >> + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), >> + N_(" - add contents of untracked files to the staged set of changes")); >> + printf("\n"); >> +} >> diff --git a/add-interactive.h b/add-interactive.h >> index 1f4747553c..46e17c5c71 100644 >> --- a/add-interactive.h >> +++ b/add-interactive.h >> @@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata); >> >> void add_i_print_modified(void); >> >> -#endif >> \ No newline at end of file >> +void add_i_show_help(void); >> + >> +#endif >> diff --git a/builtin/add--helper.c b/builtin/add--helper.c >> index 43545d9af5..a3b3a68b68 100644 >> --- a/builtin/add--helper.c >> +++ b/builtin/add--helper.c >> @@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = { >> >> enum cmd_mode { >> DEFAULT = 0, >> - STATUS >> + STATUS, >> + HELP >> }; >> >> int cmd_add__helper(int argc, const char **argv, const char *prefix) >> @@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) >> struct option options[] = { >> OPT_CMDMODE(0, "status", &mode, >> N_("print status information with diffstat"), STATUS), >> + OPT_CMDMODE(0, "show-help", &mode, >> + N_("show help"), HELP), >> OPT_END() >> }; >> >> @@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) >> >> if (mode == STATUS) >> add_i_print_modified(); >> + else if (mode == HELP) >> + add_i_show_help(); >> else >> usage_with_options(builtin_add_helper_usage, >> options); >>
Hi Slavica On 18/01/2019 12:19, Slavica Đukić wrote: > Hi Phillip, > > On 18-Jan-19 12:20 PM, Phillip Wood wrote: >> Hi Slavica >> >> I think this round is looking good I've got a couple of comments about >> the translation of the help text but everything else looks fine to me >> now. In future when you're posting a new version it's helpful CC the >> people who commented on the previous version(s). > > > Thanks for taking your time to review patches again. I'm sorry for > omitting you > > in CC, but I've sent re-roll through GitGitGadget, and I guess I thought > it would pick it up. > > I'll see what happened and keep that in mind. I'm not sure what GitGitGadget does about CC'ing people but Johannes will know >> On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote: >>> From: Slavica Djukic <slawica92@hotmail.com> >>> >>> Implement show-help command in add-interactive.c and use it in >>> builtin add--helper.c. >>> >>> Use command name "show-help" instead of "help": add--helper is >>> builtin, hence add--helper --help would be intercepted by >>> handle_builtin and re-routed to the help command, without ever >>> calling cmd_add__helper(). >>> >>> Signed-off-by: Slavica Djukic <slawica92@hotmail.com> >>> --- >>> add-interactive.c | 23 +++++++++++++++++++++++ >>> add-interactive.h | 4 +++- >>> builtin/add--helper.c | 7 ++++++- >>> 3 files changed, 32 insertions(+), 2 deletions(-) >>> >>> diff --git a/add-interactive.c b/add-interactive.c >>> index c55d934186..76c3f4c3eb 100644 >>> --- a/add-interactive.c >>> +++ b/add-interactive.c >>> @@ -244,3 +244,26 @@ void add_i_print_modified(void) >>> free(files); >>> hashmap_free(&s.file_map, 1); >>> } >>> + >>> +void add_i_show_help(void) >>> +{ >>> + const char *help_color = get_color(COLOR_HELP); >>> + color_fprintf(stdout, help_color, "%s%s", _("status"), >>> + N_(" - show paths with changes")); >>> + printf("\n"); >> There seems to be a bit of confusion with the translation of these >> messages. "status" does not want to be translated so it shouldn't be in >> _() - it can just go in the format string as can the indentation and the >> "\n" (or we could use color_fprintf_ln() to automatically add a newline >> at the end. N_() is used to mark static strings for translation so the >> gettext utilities pick up the text to be translated but (because >> initializes for static variables must be compile-time constants) does >> not do anything when the program runs - if you have 'const char *s = >> N_(hello);' you have to do '_(s)' to get the translated version. Here we >> can just pass the untranslated string directly to gettext so it should >> be _("show paths with changes"). Putting all that together we get >> >> color_fprintf(stdout, help_color, "status - %s\n", >> _("show paths with changes"); > > > I thought _() was for strings that were already translated, > and N_() for strings that weren't. And I now see that I also tried to > translate command names as well, just the opposite of what you suggested... > Thanks for clarifying this. I hope my explanation made sense, feel free to email if you want to check anything. Having thought about it, I don't think we should add "\n" to the format string as it means the color will be reset after the new line, it should use color_fprintf_ln() instead which adds a new line after it has reset the color. Best Wishes Phillip >> Best Wishes >> >> Phillip >> >>> + color_fprintf(stdout, help_color, "%s%s", _("update"), >>> + N_(" - add working tree state to the staged set of changes")); >>> + printf("\n"); >>> + color_fprintf(stdout, help_color, "%s%s", _("revert"), >>> + N_(" - revert staged set of changes back to the HEAD version")); >>> + printf("\n"); >>> + color_fprintf(stdout, help_color, "%s%s", _("patch"), >>> + N_(" - pick hunks and update selectively")); >>> + printf("\n"); >>> + color_fprintf(stdout, help_color, "%s%s", _("diff"), >>> + N_(" - view diff between HEAD and index")); >>> + printf("\n"); >>> + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), >>> + N_(" - add contents of untracked files to the staged set of changes")); >>> + printf("\n"); >>> +} >>> diff --git a/add-interactive.h b/add-interactive.h >>> index 1f4747553c..46e17c5c71 100644 >>> --- a/add-interactive.h >>> +++ b/add-interactive.h >>> @@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata); >>> >>> void add_i_print_modified(void); >>> >>> -#endif >>> \ No newline at end of file >>> +void add_i_show_help(void); >>> + >>> +#endif >>> diff --git a/builtin/add--helper.c b/builtin/add--helper.c >>> index 43545d9af5..a3b3a68b68 100644 >>> --- a/builtin/add--helper.c >>> +++ b/builtin/add--helper.c >>> @@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = { >>> >>> enum cmd_mode { >>> DEFAULT = 0, >>> - STATUS >>> + STATUS, >>> + HELP >>> }; >>> >>> int cmd_add__helper(int argc, const char **argv, const char *prefix) >>> @@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) >>> struct option options[] = { >>> OPT_CMDMODE(0, "status", &mode, >>> N_("print status information with diffstat"), STATUS), >>> + OPT_CMDMODE(0, "show-help", &mode, >>> + N_("show help"), HELP), >>> OPT_END() >>> }; >>> >>> @@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) >>> >>> if (mode == STATUS) >>> add_i_print_modified(); >>> + else if (mode == HELP) >>> + add_i_show_help(); >>> else >>> usage_with_options(builtin_add_helper_usage, >>> options); >>>
Hi Phillip, On Fri, 18 Jan 2019, Phillip Wood wrote: > On 18/01/2019 12:19, Slavica Đukić wrote: > > > > On 18-Jan-19 12:20 PM, Phillip Wood wrote: > > > > > > I think this round is looking good I've got a couple of comments > > > about the translation of the help text but everything else looks > > > fine to me now. In future when you're posting a new version it's > > > helpful CC the people who commented on the previous version(s). > > > > > > Thanks for taking your time to review patches again. I'm sorry for > > omitting you > > > > in CC, but I've sent re-roll through GitGitGadget, and I guess I > > thought it would pick it up. > > > > I'll see what happened and keep that in mind. > > I'm not sure what GitGitGadget does about CC'ing people but Johannes > will know The idea is to have a footer (read: last line) of the PR description of the form: Cc: Name <email@example.org>, Other <other@example.org> i.e. a comma-separated list of recipients to Cc. Yes, it is under-documented, but I still need to implement more features before I can start to polish documentation. Ciao, Dscho > > > > On 18/01/2019 07:47, Slavica Djukic via GitGitGadget wrote: > > > > From: Slavica Djukic <slawica92@hotmail.com> > > > > > > > > Implement show-help command in add-interactive.c and use it in > > > > builtin add--helper.c. > > > > > > > > Use command name "show-help" instead of "help": add--helper is > > > > builtin, hence add--helper --help would be intercepted by > > > > handle_builtin and re-routed to the help command, without ever > > > > calling cmd_add__helper(). > > > > > > > > Signed-off-by: Slavica Djukic <slawica92@hotmail.com> > > > > --- > > > > add-interactive.c | 23 +++++++++++++++++++++++ > > > > add-interactive.h | 4 +++- > > > > builtin/add--helper.c | 7 ++++++- > > > > 3 files changed, 32 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/add-interactive.c b/add-interactive.c > > > > index c55d934186..76c3f4c3eb 100644 > > > > --- a/add-interactive.c > > > > +++ b/add-interactive.c > > > > @@ -244,3 +244,26 @@ void add_i_print_modified(void) > > > > free(files); > > > > hashmap_free(&s.file_map, 1); > > > > } > > > > + > > > > +void add_i_show_help(void) > > > > +{ > > > > + const char *help_color = get_color(COLOR_HELP); > > > > + color_fprintf(stdout, help_color, "%s%s", _("status"), > > > > + N_(" - show paths with changes")); > > > > + printf("\n"); > > > There seems to be a bit of confusion with the translation of these > > > messages. "status" does not want to be translated so it shouldn't be in > > > _() - it can just go in the format string as can the indentation and the > > > "\n" (or we could use color_fprintf_ln() to automatically add a newline > > > at the end. N_() is used to mark static strings for translation so the > > > gettext utilities pick up the text to be translated but (because > > > initializes for static variables must be compile-time constants) does > > > not do anything when the program runs - if you have 'const char *s = > > > N_(hello);' you have to do '_(s)' to get the translated version. Here we > > > can just pass the untranslated string directly to gettext so it should > > > be _("show paths with changes"). Putting all that together we get > > > > > > color_fprintf(stdout, help_color, "status - %s\n", > > > _("show paths with changes"); > > > > > > I thought _() was for strings that were already translated, > > and N_() for strings that weren't. And I now see that I also tried to > > translate command names as well, just the opposite of what you suggested... > > Thanks for clarifying this. > > I hope my explanation made sense, feel free to email if you want to check > anything. > > Having thought about it, I don't think we should add "\n" to the format string > as it means the color will be reset after the new line, it should use > color_fprintf_ln() instead which adds a new line after it has reset the color. > > Best Wishes > > Phillip > > > > Best Wishes > > > > > > Phillip > > > > > > > + color_fprintf(stdout, help_color, "%s%s", _("update"), > > > > + N_(" - add working tree state to the staged set of > > > > changes")); > > > > + printf("\n"); > > > > + color_fprintf(stdout, help_color, "%s%s", _("revert"), > > > > + N_(" - revert staged set of changes back to the HEAD > > > > version")); > > > > + printf("\n"); > > > > + color_fprintf(stdout, help_color, "%s%s", _("patch"), > > > > + N_(" - pick hunks and update selectively")); > > > > + printf("\n"); > > > > + color_fprintf(stdout, help_color, "%s%s", _("diff"), > > > > + N_(" - view diff between HEAD and index")); > > > > + printf("\n"); > > > > + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), > > > > + N_(" - add contents of untracked files to the staged set of > > > > changes")); > > > > + printf("\n"); > > > > +} > > > > diff --git a/add-interactive.h b/add-interactive.h > > > > index 1f4747553c..46e17c5c71 100644 > > > > --- a/add-interactive.h > > > > +++ b/add-interactive.h > > > > @@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, > > > > void *cbdata); > > > > > > > > void add_i_print_modified(void); > > > > > > > > -#endif > > > > \ No newline at end of file > > > > +void add_i_show_help(void); > > > > + > > > > +#endif > > > > diff --git a/builtin/add--helper.c b/builtin/add--helper.c > > > > index 43545d9af5..a3b3a68b68 100644 > > > > --- a/builtin/add--helper.c > > > > +++ b/builtin/add--helper.c > > > > @@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = > > > > { > > > > > > > > enum cmd_mode { > > > > DEFAULT = 0, > > > > - STATUS > > > > + STATUS, > > > > + HELP > > > > }; > > > > > > > > int cmd_add__helper(int argc, const char **argv, const char *prefix) > > > > @@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const > > > > char *prefix) > > > > struct option options[] = { > > > > OPT_CMDMODE(0, "status", &mode, > > > > N_("print status information with diffstat"), > > > > STATUS), > > > > + OPT_CMDMODE(0, "show-help", &mode, > > > > + N_("show help"), HELP), > > > > OPT_END() > > > > }; > > > > @@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, > > > > const char *prefix) > > > > > > > > if (mode == STATUS) > > > > add_i_print_modified(); > > > > + else if (mode == HELP) > > > > + add_i_show_help(); > > > > else > > > > usage_with_options(builtin_add_helper_usage, > > > > options); > > > > > > >
diff --git a/add-interactive.c b/add-interactive.c index c55d934186..76c3f4c3eb 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -244,3 +244,26 @@ void add_i_print_modified(void) free(files); hashmap_free(&s.file_map, 1); } + +void add_i_show_help(void) +{ + const char *help_color = get_color(COLOR_HELP); + color_fprintf(stdout, help_color, "%s%s", _("status"), + N_(" - show paths with changes")); + printf("\n"); + color_fprintf(stdout, help_color, "%s%s", _("update"), + N_(" - add working tree state to the staged set of changes")); + printf("\n"); + color_fprintf(stdout, help_color, "%s%s", _("revert"), + N_(" - revert staged set of changes back to the HEAD version")); + printf("\n"); + color_fprintf(stdout, help_color, "%s%s", _("patch"), + N_(" - pick hunks and update selectively")); + printf("\n"); + color_fprintf(stdout, help_color, "%s%s", _("diff"), + N_(" - view diff between HEAD and index")); + printf("\n"); + color_fprintf(stdout, help_color, "%s%s", _("add untracked"), + N_(" - add contents of untracked files to the staged set of changes")); + printf("\n"); +} diff --git a/add-interactive.h b/add-interactive.h index 1f4747553c..46e17c5c71 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -5,4 +5,6 @@ int add_i_config(const char *var, const char *value, void *cbdata); void add_i_print_modified(void); -#endif \ No newline at end of file +void add_i_show_help(void); + +#endif diff --git a/builtin/add--helper.c b/builtin/add--helper.c index 43545d9af5..a3b3a68b68 100644 --- a/builtin/add--helper.c +++ b/builtin/add--helper.c @@ -10,7 +10,8 @@ static const char * const builtin_add_helper_usage[] = { enum cmd_mode { DEFAULT = 0, - STATUS + STATUS, + HELP }; int cmd_add__helper(int argc, const char **argv, const char *prefix) @@ -20,6 +21,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_CMDMODE(0, "status", &mode, N_("print status information with diffstat"), STATUS), + OPT_CMDMODE(0, "show-help", &mode, + N_("show help"), HELP), OPT_END() }; @@ -30,6 +33,8 @@ int cmd_add__helper(int argc, const char **argv, const char *prefix) if (mode == STATUS) add_i_print_modified(); + else if (mode == HELP) + add_i_show_help(); else usage_with_options(builtin_add_helper_usage, options);