Re: [PATCH v3] rev-parse --parseopt: option argument name hints
- From
Ilya Bobyr <ilya.bobyr@gmail.com>
- Date
- Mar 20, 2014, 23:19 UTC
- Message-ID
- <532B7774.30308@gmail.com>
- In-Reply-To
- <xmqqpplgyaud.fsf@gitster.dls.corp.google.com>
On 3/20/2014 11:38 AM, Junio C Hamano wrote:
Show 53 quoted lines
> 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?
Show 21 quoted lines
>> @@ -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.
Show 16 quoted lines
>> @@ -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.