Re: [PATCH v2 5/7] add-interactive.c: implement show-help command

[Phillip Wood <phillip.wood@talktalk.net>]

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);
>>>