Re: [PATCH RFC] git remote: Separate usage strings for subcommands

From: Tim Henigan <tim.henigan@gmail.com>
Date: Nov 17, 2009, 01:39 UTC
Message-ID: <32c343770911161739g203a32d4p2b9f8ea6b39d900@mail.gmail.com>
In-Reply-To: <7vocn2klp8.fsf@alter.siamese.dyndns.org>

On Mon, Nov 16, 2009 at 8:02 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> ...
>> +static const char * const builtin_remote_add_usage[] = {
>> REMOTE_ADD_USAGE, NULL };
>> ...
>
> I am not sure about the value of reusing option string like this, and for
> all other subcommands the same comment applies.  For example, in the case
> of "remote add -h", you would use
>
> "git remote add [-t <branch>] [-m <master>] [-f] [--mirror] <name> <url>"
> from REMOTE_ADD_USAGE, but ...

Do you object to the new #defines for the strings?  I added them since we now
really need to construct the usage string for 2 separate uses:
  (1) When 'git remote -h' is invoked, we need to return the usage
string showing
       all subcommands.
  (2) When 'git remote <subcommand> -h' is invoked, we need to return the
      usage for just that command.

It doesn't make sense to me to maintain separate strings for the two use cases.

Show 25 quoted lines

>> @@ -70,7 +87,6 @@ static int add(int argc, const char **argv)
>>       int i;
>>
>>       struct option options[] = {
>> -             OPT_GROUP("add specific options"),
>>               OPT_BOOLEAN('f', "fetch", &fetch, "fetch the remote branches"),
>>               OPT_CALLBACK('t', "track", &track, "branch",
>>                       "branch(es) to track", opt_parse_track),
>> @@ -79,11 +95,11 @@ static int add(int argc, const char **argv)
>>               OPT_END()
>>       };
>>
>> -     argc = parse_options(argc, argv, NULL, options, builtin_remote_usage,
>> +     argc = parse_options(argc, argv, NULL, options, builtin_remote_add_usage,
>>                            0);
>
> ... the options list is used to reproduce the information in a major part
> of that string already.  So I would prefer builtin_remote_add_usage[] to
> be something like:
>
>    "git remote add [<options>...] <name> <url>"
>
> It is in line with a discussion we had earlier:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/129906/focus=130646

I planned to make this change in a separate (or maybe just a later version) patch, but first I wanted to be sure my other changes were sound. I will send a new version that includes this change.

Should this change be made to the man page as well?

Show 8 quoted lines

>> @@ -1334,7 +1348,6 @@ static int show_all(void)
>>  int cmd_remote(int argc, const char **argv, const char *prefix)
>>  {
>>  	struct option options[] = {
>> -		OPT__VERBOSE(&verbose),
>>  		OPT_END()
>>  	};
>>  	int result;

I did find one problem with the above portion of the patch. With this change '-v' was no longer recognized as an option. I will remove this change in the next version. Keeping it means that '-v' will still be reported in the 'git remote -h' usage string, but I don't see an easy way to remove the string without removing the feature.