Hi Slavica
I think the basics of this patch are fine, I've got a few comments below
On 20/12/2018 12:09, Slavica Djukic via GitGitGadget wrote:
> From: Slavica Djukic <slawica92@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> add-interactive.c | 19 +++++++++++++++++++
> add-interactive.h | 2 ++
> builtin/add--helper.c | 7 ++++++-
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index c55d934186..ff5bfbac49 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -8,6 +8,16 @@
>
> #define HEADER_INDENT " "
>
> +/* TRANSLATORS: please do not translate the command names
> + 'status', 'update', 'revert', etc. */
> +static const char help_info[] =
> + N_("status - show paths with changes\n"
> + "update - add working tree state to the staged set of changes\n"
> + "revert - revert staged set of changes back to the HEAD version\n"
> + "patch - pick hunks and update selectively\n"
> + "diff - view diff between HEAD and index\n"
> + "add untracked - add contents of untracked files to the staged set of changes");
If we were starting from scratch I'd suggest only translating the help
text, not the command names as this would avoid any possible problems
with the command names and indentation. As we already have this
translated it is easier to leave it as is unless the translators think
it would be useful to change it. I'd be inclined to put this definition
next to the function that uses it (or possibly in that function) so it
is easy to see what text will be printed when reading the function.
> +
> enum collection_phase {
> WORKTREE,
> INDEX
> @@ -244,3 +254,12 @@ void add_i_print_modified(void)
> free(files);
> hashmap_free(&s.file_map, 1);
> }
> +
> +void show_help(void)
> +{
> + const char *help_color = get_color(COLOR_HELP);
> + const char *modified_fmt = _("%s");
This does not need to be translated, also I'm not sure we need a
separate variable (unless something funny is happening - why has it got
'modified' in its name?)
> + printf("\n");
> + color_fprintf(stdout, help_color, modified_fmt, _(help_info));
> + printf("\n");
> +}
> diff --git a/add-interactive.h b/add-interactive.h
> index 1f4747553c..a74c65b7e1 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);
>
> +void show_help(void);
As this is a global function I think it needs a more specific name to
avoid clashing with a function that shows the help for a different
command. Maybe add_i_show_help() to match add_i_print_modified()?
> +
> #endif
> \ No newline at end of file
C files should end with a new line (it is actually undefined behavior if
they don't!)
Best Wishes
Phillip
> diff --git a/builtin/add--helper.c b/builtin/add--helper.c
> index 43545d9af5..e288412d56 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)
> + show_help();
> else
> usage_with_options(builtin_add_helper_usage,
> options);
>
