From: Philip Oakley <philipoakley@iee.org>
Date: Sun, 03 Mar 2013 23:44:18 GMT
Subject: Re: [PATCH v2 3/5] Help.c add --guide option
Message-ID: <667B2BF898CC4491BE02C1142472A833@PhilipOakley>
In-Reply-To: <7vk3poyryc.fsf@alter.siamese.dyndns.org>

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Sunday, March 03, 2013 11:38 PM
> Philip Oakley <philipoakley@iee.org> writes:
>
>> Logic, but no actions, included.
>
> I am not sure what you mean.  Is that to break "bisect"?
>
> Ahh, you meant "command line is parsed but we do not actually show
> guides yet, which is done by later patches in this series".  OK.
True. I can add to the commit message.

>
>> The --all commands option, if given, will display first.
>> The --guide option's list will then be displayed.
>>
>> The common commands list is only displayed if neither option,
>> nor a command or guide name, is given.
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.org>
>> ---
>>  builtin/help.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/help.c b/builtin/help.c
>> index d10cbed..6089d72 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -36,10 +36,12 @@ enum help_format {
>>  static const char *html_path;
>>
>>  static int show_all = 0;
>> +static int show_guides = 0;
>>  static unsigned int colopts;
>>  static enum help_format help_format = HELP_FORMAT_NONE;
>>  static struct option builtin_help_options[] = {
>>  OPT_COUNTUP('a', "all", &show_all, N_("print all available 
>> commands")),
>> + OPT_COUNTUP('g', "guides", &show_guides, N_("print list of useful 
>> guides")),
>>  OPT_SET_INT('m', "man", &help_format, N_("show man page"), 
>> HELP_FORMAT_MAN),
>>  OPT_SET_INT('w', "web", &help_format, N_("show manual in web 
>> browser"),
>>  HELP_FORMAT_WEB),
>> @@ -49,7 +51,7 @@ static struct option builtin_help_options[] = {
>>  };
>>
>>  static const char * const builtin_help_usage[] = {
>> - N_("git help [--all] [--man|--web|--info] [command]"),
>> + N_("git help [--all] [--guides] [--man|--web|--info] [command]"),
>>  NULL
>>  };
>>
>> @@ -429,9 +431,11 @@ int cmd_help(int argc, const char **argv, const 
>> char *prefix)
>>  printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>>  list_commands(colopts, &main_cmds, &other_cmds);
>>  printf("%s\n", _(git_more_info_string));
>> + if (!show_guides) return 0;
>> + }
>> + if (show_guides) {
>>  return 0;
>>  }
>> -
>
> Ugly.

I was trying to minimise the apparent changes, but like you say...

>
> if (show_all) {
>        ... do not touch anything here ...
> ... but remove "return 0;" ...
> }
>
> if (show_guides) {
>        ... show guides but do not "return 0" ...
> }
>
> if (show_all || show_guides) {
> ... we were asked to do either/or --all/--guides ...
>                ... and have done what we were asked to do ...
> return 0;
> }

Neater.

>
> This is a tangent, but before all of the above, cmd_help() should
> verify that it got no arguments (when show_all/show_guides is in
> effect) or it got one argument (otherwise), I think.
>
>>  if (!argv[0]) {
>>  printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
>>  list_common_cmds_help();

Seems reasonable. Should that be a separate preparatory patch (likely), 
or joined in with the rest?

Philip