Re: [PATCH v3] rev-parse --parseopt: option argument name hints

[Ilya Bobyr <ilya.bobyr@gmail.com>]

On 3/20/2014 11:38 AM, Junio C Hamano wrote:
> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>> Built-in commands can specify names for option arguments when usage text
>> is generated for a command.  sh based commands should be able to do the
>> same.
>>
>> Option argument name hint is any text that comes after [*=?!] after the
>> argument name up to the first whitespace.  Underscores are replaced with
>> whitespace.  It is unlikely that an underscore would be useful in the
>> hint text.
>>
>> Signed-off-by: Ilya Bobyr <ilya.bobyr@gmail.com>
>> ---
>>   Changed according to the last comments.  Added "Usage text" paragraph in the
>>   documentation and updated variable names.
>>
>>   Documentation/git-rev-parse.txt |   34 ++++++++++++++++++++++++++++++++--
>>   builtin/rev-parse.c             |   17 ++++++++++++++++-
>>   t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
>>   3 files changed, 68 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
>> index 0d2cdcd..b8aabc9 100644
>> --- a/Documentation/git-rev-parse.txt
>> +++ b/Documentation/git-rev-parse.txt
>> @@ -284,13 +284,13 @@ Input Format
>>   
>>   'git rev-parse --parseopt' input format is fully text based. It has two parts,
>>   separated by a line that contains only `--`. The lines before the separator
>> -(should be more than one) are used for the usage.
>> +(should be one or more) are used for the usage.
>>   The lines after the separator describe the options.
>>   
>>   Each line of options has this format:
>>   
>>   ------------
>> -<opt_spec><flags>* SP+ help LF
>> +<opt_spec><flags>*<arg_hint>? SP+ help LF
>>   ------------
>>   
>>   `<opt_spec>`::
>> @@ -313,6 +313,12 @@ Each line of options has this format:
>>   
>>   	* Use `!` to not make the corresponding negated long option available.
>>   
>> +`<arg_hint>`::
>> +	`<arg_hing>`, if specified, is used as a name of the argument in the
>> +	help output, for options that take arguments. `<arg_hint>` is
>> +	terminated by the first whitespace. When output the name is shown in
>> +	angle braces.  Underscore symbols are replaced with spaces.
> The last part is troubling (and sounds not very sane).  Do we do
> such a munging anywhere else, or is it just here?  If the latter I'd
> prefer not to see such a hack.

The following commands have spaces in argument names in the "-h" output 
for one or two arguments:
   * clone
   * commit
   * merge

A number of commands use dashes to separate words in arguments names.

"git notes" is the only command that uses an underscore in one argument 
name.

At the moment space is used to separate option specification from the 
help line.  As argument name hint is part of the option specification it 
ends at the first space.

It seems a bit unfair if sh based commands would not be able to use 
spaces while the build-in ones can.
As underscores are not used in the UI (at least that was my impression 
so far), I thought that to be a good option.

Do you think a different kind of escaping should be used? Backslashes?
Or no spaces?

>> @@ -333,6 +339,8 @@ h,help    show the help
>>   
>>   foo       some nifty option --foo
>>   bar=      some cool option --bar with an argument
>> +baz=arg   another cool option --baz with a named argument
>> +qux?path  qux may take a path argument but has meaning by itself
>>   
>>     An option group Header
>>   C?        option C with an optional argument"
>> @@ -340,6 +348,28 @@ C?        option C with an optional argument"
>>   eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
>>   ------------
>>   
>> +
>> +Usage text
>> +~~~~~~~~~~
>> +
>> +When "$@" is "-h" or "--help" the above example would produce the following
>> +usage text:
> Sounds like a good idea to add this; all the above arguments inside
> double quotes should be typeset `as-typed`, though.

Thanks, I will fix that.

>> @@ -419,6 +420,20 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>>   		o->value = &parsed;
>>   		o->flags = PARSE_OPT_NOARG;
>>   		o->callback = &parseopt_dump;
>> +
>> +		/* Possible argument name hint */
>> +		end = s;
>> +		while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
>> +			--s;
>> +		if (s != sb.buf && s != end) {
>> +			char *a;
>> +			o->argh = a = xmemdupz(s, end - s);
>> +			while (a = strchr(a, '_'))
>> +				*a = ' ';
> ... and without the "underscore" munging, we do not have to allocate
> a new piece of memory, either.

We would have to do it any way to have the string zero terminated.
The list of arguments that holds the lines been parsed is "const char *".

But I do not think this is an argument to be considered when designing 
the user interface :)

Never the less if there is a way not to allocate extra memory that I am 
missing - let me know, I would remove the allocation.