From: Phillip Wood Date: Fri, 18 Jan 2019 14:25:04 GMT Subject: Re: [PATCH v2 5/7] add-interactive.c: implement show-help command Message-ID: In-Reply-To: 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 >>> >>> 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 >>> --- >>> 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); >>>