threads / patch / 36035

patchrev-parse --parseopt: option argument name hints

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

## tl;dr

23 messages between Mar 3, 2014 and Mar 24, 2014. Diffs are folded; open one to read it.

replies: 22people: 3as markdown or json

Ilya Bobyr· Mar 3, 2014, 10:32 UTC · lore

Built-in commands can specify names for option arguments, that are shown when usage text is generated for the 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>
---
 Documentation/git-rev-parse.txt |   11 +++++++++--
 builtin/rev-parse.c             |   17 ++++++++++++++++-
 t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
 3 files changed, 45 insertions(+), 3 deletions(-)
Show changes to 3 files +45 −3

Documentation/git-rev-parse.txt, builtin/rev-parse.c, t/t1502-rev-parse-parseopt.sh

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..4cb6e02 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.
+(could be more than one) 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>*<argh>? 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.
 
+`<argh>`::
+	`<argh>`, if specified, is used as a name of the argument, if the
+	option takes an argument. `<argh>` is terminated by the first
+	whitespace. Angle braces are added automatically.  Underscore symbols
+	are replaced with spaces.
+
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
 
@@ -333,6 +339,7 @@ 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 an argument named <arg>
 
   An option group Header
 C?        option C with an optional argument"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..83a769e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		usage[unb++] = strbuf_detach(&sb, NULL);
 	}
 
-	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
+	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
 	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
 		const char *s;
+		const char *argh;
 		struct option *o;
 
 		if (!sb.len)
@@ -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 */
+		argh = s;
+		while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
+			--s;
+		if (s != sb.buf && s != argh) {
+			char *a;
+			o->argh = a = xmemdupz(s, argh - s);
+			while (a = strchr(a, '_'))
+				*a = ' ';
+		}
+		if (s == sb.buf)
+			s = argh;
+
 		while (s > sb.buf && strchr("*=?!", s[-1])) {
 			switch (*--s) {
 			case '=':
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..bf0db05 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
     -C[...]               option C with an optional argument
     -d, --data[=...]      short and long option with an optional argument
 
+Argument hints
+    -b <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with spaces>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <a very long argument hint>
+                          a very long argument hint
+
 Extras
     --extra1              line above used to cause a segfault but no longer does
 
@@ -39,6 +50,15 @@ b,baz     a short and long option
 C?        option C with an optional argument
 d,data?   short and long option with an optional argument
 
+ Argument hints
+b=arg     short option required argument
+bar2=arg  long option required argument
+e,fuz=with_spaces  short and long option required argument
+s?some    short option optional argument
+long?data long option optional argument
+g,fluf?path     short and long option optional argument
+longest=a_very_long_argument_hint  a very long argument hint
+
 Extras
 extra1    line above used to cause a segfault but no longer does
 EOF
-- 
1.7.9
Junio C Hamano· Mar 4, 2014, 19:22 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
Show 26 quoted lines
> Built-in commands can specify names for option arguments, that are shown
> when usage text is generated for the 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>
> ---
>  Documentation/git-rev-parse.txt |   11 +++++++++--
>  builtin/rev-parse.c             |   17 ++++++++++++++++-
>  t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 0d2cdcd..4cb6e02 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.
> +(could be more than one) are used for the usage.

Good spotting. I think the original author meant to say there should be at least one line to serve as the usage string, so updating it to "should be one or more" may be more accurate, but "could be more than one" would also work.

Show 19 quoted lines
>  The lines after the separator describe the options.
>  
>  Each line of options has this format:
>  
>  ------------
> -<opt_spec><flags>* SP+ help LF
> +<opt_spec><flags>*<argh>? 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.
>  
> +`<argh>`::
> +	`<argh>`, if specified, is used as a name of the argument, if the
> +	option takes an argument. `<argh>` is terminated by the first
> +	whitespace. Angle braces are added automatically.  Underscore symbols
> +	are replaced with spaces.

I had a hard time understanding this "Angle brackets are added automatically" one (obviously nobody wants extra angle brackets added around option arguments given by the user), until I looked at the addition of the test to realize that this description is only about how it appears in the help output. The description needs to be clarified to avoid confusion.

Show 5 quoted lines
> @@ -333,6 +339,7 @@ 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 an argument named <arg>

It probably is better not to have " named <arg>" at the end here, as that gives an apparent-but-false contradiction with the "Angle brackets are added *automatically*" and confuse readers. At least, it confused _this_ reader.

After the "eval" in the existing example to parse the "$@" argument list in this part of the documentation, it may be a good idea to say something like:

	The above command, when "$@" is "--help", produces the
	following help output:
	... sample output here ...

to show the actual output. That way, we can illustrate how input "baz?arg description of baz" is turned into "--baz[=<arg>]" output clearly (yes, I am suggesting to use '?' in the new example, not '=' whose usage is already shown in the existing example).

Show 13 quoted lines
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index aaeb611..83a769e 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		usage[unb++] = strbuf_detach(&sb, NULL);
>  	}
>  
> -	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
> +	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
>  	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
>  		const char *s;
> +		const char *argh;
Let's spell that variable name out, e.g. arg_hint or something.
Show 35 quoted lines
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index 83b1300..bf0db05 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -18,6 +18,17 @@ An option group Header
>      -C[...]               option C with an optional argument
>      -d, --data[=...]      short and long option with an optional argument
>  
> +Argument hints
> +    -b <arg>              short option required argument
> +    --bar2 <arg>          long option required argument
> +    -e, --fuz <with spaces>
> +                          short and long option required argument
> +    -s[<some>]            short option optional argument
> +    --long[=<data>]       long option optional argument
> +    -g, --fluf[=<path>]   short and long option optional argument
> +    --longest <a very long argument hint>
> +                          a very long argument hint
> +
>  Extras
>      --extra1              line above used to cause a segfault but no longer does
>  
> @@ -39,6 +50,15 @@ b,baz     a short and long option
>  C?        option C with an optional argument
>  d,data?   short and long option with an optional argument
>  
> + Argument hints
> +b=arg     short option required argument
> +bar2=arg  long option required argument
> +e,fuz=with_spaces  short and long option required argument
> +s?some    short option optional argument
> +long?data long option optional argument
> +g,fluf?path     short and long option optional argument
> +longest=a_very_long_argument_hint  a very long argument hint
> +
Nice.
Thanks.
Ilya Bobyr· Mar 10, 2014, 05:47 UTC · re: Junio C Hamano · lore

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

On 3/4/2014 11:22 AM, Junio C Hamano wrote:
Show 32 quoted lines
> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>> Built-in commands can specify names for option arguments, that are shown
>> when usage text is generated for the 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>
>> ---
>>   Documentation/git-rev-parse.txt |   11 +++++++++--
>>   builtin/rev-parse.c             |   17 ++++++++++++++++-
>>   t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
>>   3 files changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
>> index 0d2cdcd..4cb6e02 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.
>> +(could be more than one) are used for the usage.
> Good spotting.  I think the original author meant to say there
> should be at least one line to serve as the usage string, so
> updating it to "should be one or more" may be more accurate, but
> "could be more than one" would also work.
Changed to "should be one or more".
Show 25 quoted lines
>>   The lines after the separator describe the options.
>>   
>>   Each line of options has this format:
>>   
>>   ------------
>> -<opt_spec><flags>* SP+ help LF
>> +<opt_spec><flags>*<argh>? 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.
>>   
>> +`<argh>`::
>> +	`<argh>`, if specified, is used as a name of the argument, if the
>> +	option takes an argument. `<argh>` is terminated by the first
>> +	whitespace. Angle braces are added automatically.  Underscore symbols
>> +	are replaced with spaces.
> I had a hard time understanding this "Angle brackets are added
> automatically" one (obviously nobody wants extra angle brackets
> added around option arguments given by the user), until I looked at
> the addition of the test to realize that this description is only
> about how it appears in the help output.  The description needs to
> be clarified to avoid confusion.

I've reworded some of the sentences. I think it is better now. Let me know what you think.

Show 9 quoted lines
>> @@ -333,6 +339,7 @@ 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 an argument named <arg>
> It probably is better not to have " named <arg>" at the end here, as
> that gives an apparent-but-false contradiction with the "Angle
> brackets are added *automatically*" and confuse readers.  At least,
> it confused _this_ reader.

I am not sure I understand what is confusing here. But I removed the " named <arg>" part. If there would be an example, I think, it is easy to understand how it works.

Show 13 quoted lines
> After the "eval" in the existing example to parse the "$@" argument
> list in this part of the documentation, it may be a good idea to say
> something like:
>
> 	The above command, when "$@" is "--help", produces the
> 	following help output:
>
> 	... sample output here ...
>
> to show the actual output.  That way, we can illustrate how input
> "baz?arg description of baz" is turned into "--baz[=<arg>]" output
> clearly (yes, I am suggesting to use '?' in the new example, not '='
> whose usage is already shown in the existing example).

Documentation on the whole argument parsing is quite short, so, I though, adding an example just to show how usage is generated would look like I am trying to make this feature look important than it is :)

I've added another section that shows usage text generated for the example specification.

Show 14 quoted lines
>> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
>> index aaeb611..83a769e 100644
>> --- a/builtin/rev-parse.c
>> +++ b/builtin/rev-parse.c
>> @@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>>   		usage[unb++] = strbuf_detach(&sb, NULL);
>>   	}
>>   
>> -	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
>> +	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
>>   	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
>>   		const char *s;
>> +		const char *argh;
> Let's spell that variable name out, e.g. arg_hint or something.

I was looking at the surrounding code for some style guidance, but most local variables have short names like "s", "o", "onb", "osz", "sb". There are some that are longer. So I was quite unsure here. At the same time the target structure that holds the option description calls this string "argh". Also, this is not really an "arg_hint" but the end of it. Argument name is actually between s and argh, if there is some. Considering all that, "argh" seemed like an OK name.

I've renamed it to "end". It is used to remember possible end of the argument name in just one paragraph of code. Comments above the paragraph clarifies what is been extracted. Should there be another "parameter" in the option specification, the same variable could be used while parsing that one as well.

Let me know if you what that to be "arg_hint", or "arg_hint_end", or anything else.

Show 36 quoted lines
>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
>> index 83b1300..bf0db05 100755
>> --- a/t/t1502-rev-parse-parseopt.sh
>> +++ b/t/t1502-rev-parse-parseopt.sh
>> @@ -18,6 +18,17 @@ An option group Header
>>       -C[...]               option C with an optional argument
>>       -d, --data[=...]      short and long option with an optional argument
>>   
>> +Argument hints
>> +    -b <arg>              short option required argument
>> +    --bar2 <arg>          long option required argument
>> +    -e, --fuz <with spaces>
>> +                          short and long option required argument
>> +    -s[<some>]            short option optional argument
>> +    --long[=<data>]       long option optional argument
>> +    -g, --fluf[=<path>]   short and long option optional argument
>> +    --longest <a very long argument hint>
>> +                          a very long argument hint
>> +
>>   Extras
>>       --extra1              line above used to cause a segfault but no longer does
>>   
>> @@ -39,6 +50,15 @@ b,baz     a short and long option
>>   C?        option C with an optional argument
>>   d,data?   short and long option with an optional argument
>>   
>> + Argument hints
>> +b=arg     short option required argument
>> +bar2=arg  long option required argument
>> +e,fuz=with_spaces  short and long option required argument
>> +s?some    short option optional argument
>> +long?data long option optional argument
>> +g,fluf?path     short and long option optional argument
>> +longest=a_very_long_argument_hint  a very long argument hint
>> +
> Nice.
Thanks :)
P.S. Patch comes in the next message.
Ilya Bobyr· Mar 10, 2014, 05:55 UTC · re: Ilya Bobyr · lore

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

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>
---
 Documentation/git-rev-parse.txt |   11 +++++++++--
 builtin/rev-parse.c             |   17 ++++++++++++++++-
 t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
 3 files changed, 45 insertions(+), 3 deletions(-)
Show changes to 3 files +45 −3

Documentation/git-rev-parse.txt, builtin/rev-parse.c, t/t1502-rev-parse-parseopt.sh

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..4cb6e02 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.
+(could be more than one) 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>*<argh>? 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.
 
+`<argh>`::
+	`<argh>`, if specified, is used as a name of the argument, if the
+	option takes an argument. `<argh>` is terminated by the first
+	whitespace. Angle braces are added automatically.  Underscore symbols
+	are replaced with spaces.
+
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
 
@@ -333,6 +339,7 @@ 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 an argument named <arg>
 
   An option group Header
 C?        option C with an optional argument"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 45901df..7a58404 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		usage[unb++] = strbuf_detach(&sb, NULL);
 	}
 
-	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
+	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
 	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
 		const char *s;
+		const char *argh;
 		struct option *o;
 
 		if (!sb.len)
@@ -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 */
+		argh = s;
+		while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
+			--s;
+		if (s != sb.buf && s != argh) {
+			char *a;
+			o->argh = a = xmemdupz(s, argh - s);
+			while (a = strchr(a, '_'))
+				*a = ' ';
+		}
+		if (s == sb.buf)
+			s = argh;
+
 		while (s > sb.buf && strchr("*=?!", s[-1])) {
 			switch (*--s) {
 			case '=':
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..bf0db05 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
     -C[...]               option C with an optional argument
     -d, --data[=...]      short and long option with an optional argument
 
+Argument hints
+    -b <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with spaces>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <a very long argument hint>
+                          a very long argument hint
+
 Extras
     --extra1              line above used to cause a segfault but no longer does
 
@@ -39,6 +50,15 @@ b,baz     a short and long option
 C?        option C with an optional argument
 d,data?   short and long option with an optional argument
 
+ Argument hints
+b=arg     short option required argument
+bar2=arg  long option required argument
+e,fuz=with_spaces  short and long option required argument
+s?some    short option optional argument
+long?data long option optional argument
+g,fluf?path     short and long option optional argument
+longest=a_very_long_argument_hint  a very long argument hint
+
 Extras
 extra1    line above used to cause a segfault but no longer does
 EOF
-- 
1.7.9
Junio C Hamano· Mar 10, 2014, 19:55 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
Show 13 quoted lines
> On 3/4/2014 11:22 AM, Junio C Hamano wrote:
>> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>>> @@ -333,6 +339,7 @@ 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 an argument named <arg>
>> It probably is better not to have " named <arg>" at the end here, as
>> that gives an apparent-but-false contradiction with the "Angle
>> brackets are added *automatically*" and confuse readers.  At least,
>> it confused _this_ reader.
>
> I am not sure I understand what is confusing here.  But I removed the
> " named <arg>" part.

After reading "Angle brackets are automatically given", seeing that the argument description has manually spelled "<arg>" gave me "Huh?".

Without " named <arg>" there is no such confusion.
> If there would be an example, I think, it is easy to understand how it
> works.

Of course. That is why I suggested to do without " named <arg>" part---I didn't mean to suggest not to add the example. I also think that you can demonstrate something other than '=' (whose usage is already shown with "bar=" above) here as well, but I think we can go either way.

Show 18 quoted lines
>> After the "eval" in the existing example to parse the "$@" argument
>> list in this part of the documentation, it may be a good idea to say
>> something like:
>>
>> 	The above command, when "$@" is "--help", produces the
>> 	following help output:
>>
>> 	... sample output here ...
>>
>> to show the actual output.  That way, we can illustrate how input
>> "baz?arg description of baz" is turned into "--baz[=<arg>]" output
>> clearly (yes, I am suggesting to use '?' in the new example, not '='
>> whose usage is already shown in the existing example).
>
> Documentation on the whole argument parsing is quite short, so, I
> though, adding an example just to show how usage is generated would
> look like I am trying to make this feature look important than it is
> :)

You already are by saying the "Angle brackets are automatic", aren't you?

> At the same time the target structure that holds the option
> description calls this string "argh".

OK, that is fine, then (I'd prefer a field name not to sound like arrrgh, but that is an entirely different topic).

> I've renamed it to "end".  It is used to remember possible end of the
> argument name in just one paragraph of code.
Sounds good.
Junio C Hamano· Mar 11, 2014, 19:10 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
>> Documentation on the whole argument parsing is quite short, so, I
>> though, adding an example just to show how usage is generated would
>> look like I am trying to make this feature look important than it is
>> :)
>
> You already are by saying the "Angle brackets are automatic", aren't
> you?

That is, among the things --parseopt mode does, the above stresses what happens _only_ when it emits help text for items that use this feature.

Ilya Bobyr· Mar 12, 2014, 07:26 UTC · re: Junio C Hamano · lore

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

On 3/11/2014 12:10 PM, Junio C Hamano wrote:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Documentation on the whole argument parsing is quite short, so, I
>>> though, adding an example just to show how usage is generated would
>>> look like I am trying to make this feature look important than it is
>>> :)
>> You already are by saying the "Angle brackets are automatic", aren't
>> you?
> That is, among the things --parseopt mode does, the above stresses
> what happens _only_ when it emits help text for items that use this
> feature.

`argh' is used only while help text is generated. So, there seems to be no way around it :) I was talking not about the automatic addition of angle brackets, but about the documentation on `argh' in general. The section where I've added a paragraph, is not specific to the help output, but describes --parseopt. I though that an example just to describe `argh' while useful would look a bit disproportional, compared to the amount of text on --parseopt.

But now that I've added a "Usage text" section to looks quite in place.

I just realized that the second patch I sent did not contain the changes. Sorry about - I will resend it.

I was also wondering about the possible next step(s). If you like the patch will you just take it from the maillist and it would appear in the next "What's cooking in git.git"? Or the process is different?

Junio C Hamano· Mar 12, 2014, 16:59 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
Show 5 quoted lines
> I though that an example just to describe `argh' while useful would
> look a bit disproportional, compared to the amount of text on
> --parseopt.
>
> But now that I've added a "Usage text" section to looks quite in place.
Good thinking.
> I was also wondering about the possible next step(s).  If you like
> the patch will you just take it from the maillist and it would
> appear in the next "What's cooking in git.git"?  Or the process is
> different?
It goes more like this:
 - A topic that is in a good enough shape to be discussed and moved
   forward is given its own topic branch and then merged to 'pu', so
   that we do not forget.  The topic enters "What's cooking" at this
   stage.
 - Discussion on the topic continues on the list, and the topic can
   be replaced or built upon while it is still on 'pu' to polish it
   further.
   . We may see a grave issue with the change and may discard it
     from 'pu'.  
   . We may see a period of inaction after issues are pointed out
     and/or improvements are suggested, which would cause the topic
     marked as stalled; this may cause it to be eventually discarded
     as "abandoned" if nobody cares deeply enough.
 - After a while, when it seems that we, collectively as the Git
   development circle, agree that we would eventually want that
   change in a released version in some future (not necessarily in
   the upcoming release), the topic is merged to 'next', which is
   the branch Git developers are expected to run in their daily
   lives.
    . We may see some updates that builds on the patches merged to
      'next' so far to fix late issues discovered.
    . We may see a grave issue with the change and may have to
      revert & discard it from 'next'.
 - After a while, when the topic proves to be solid, it is merged to
   'master', in preparation for the upcoming release.
Ilya Bobyr· Mar 19, 2014, 09:02 UTC · re: Junio C Hamano · lore

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

On 3/12/2014 9:59 AM, Junio C Hamano wrote:
Show 14 quoted lines
> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>> I though that an example just to describe `argh' while useful would
>> look a bit disproportional, compared to the amount of text on
>> --parseopt.
>>
>> But now that I've added a "Usage text" section to looks quite in place.
> Good thinking.
>
>> I was also wondering about the possible next step(s).  If you like
>> the patch will you just take it from the maillist and it would
>> appear in the next "What's cooking in git.git"?  Or the process is
>> different?
> It goes more like this:
Thank you for all the details.
>   - A topic that is in a good enough shape to be discussed and moved
>     forward is given its own topic branch and then merged to 'pu', so
>     that we do not forget.  The topic enters "What's cooking" at this
>     stage.

I can not find this particular patch in the latest "What's cooking" email. Is there something I can do? It does not seems like there is a lot of interest, so I am not sure there will be a lot of discussion. It is a minor fix and considering the number of the emails on the list, I do not unexpected this kind of stuff to be very popular. But it seems like a valid improvement to me. Maybe I am missing something?

Same questions about this one:
     [PATCH] gitk: replace SHA1 entry field on keyboard paste
     http://www.mail-archive.com/git@vger.kernel.org/msg45040.html

I think they are more or less similar, except that the second one is just trivial.

Show 28 quoted lines
>   - Discussion on the topic continues on the list, and the topic can
>     be replaced or built upon while it is still on 'pu' to polish it
>     further.
>
>     . We may see a grave issue with the change and may discard it
>       from 'pu'.
>
>     . We may see a period of inaction after issues are pointed out
>       and/or improvements are suggested, which would cause the topic
>       marked as stalled; this may cause it to be eventually discarded
>       as "abandoned" if nobody cares deeply enough.
>
>   - After a while, when it seems that we, collectively as the Git
>     development circle, agree that we would eventually want that
>     change in a released version in some future (not necessarily in
>     the upcoming release), the topic is merged to 'next', which is
>     the branch Git developers are expected to run in their daily
>     lives.
>
>      . We may see some updates that builds on the patches merged to
>        'next' so far to fix late issues discovered.
>
>      . We may see a grave issue with the change and may have to
>        revert & discard it from 'next'.
>
>   - After a while, when the topic proves to be solid, it is merged to
>     'master', in preparation for the upcoming release.
>
Junio C Hamano· Mar 19, 2014, 18:46 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
> I can not find this particular patch in the latest "What's cooking" email.
> Is there something I can do?

IIRC, I think I was waiting for the version with a new "Usage text" section to the documentation you alluded to in this exchange ($gmane/243924):

    Ilya Bobyr <ilya.bobyr@gmail.com> writes:
    > On 3/11/2014 12:10 PM, Junio C Hamano wrote:
    >>
    >>>> Documentation on the whole argument parsing is quite short, so,...
    > ...
    > I though that an example just to describe `argh' while useful would
    > look a bit disproportional, compared to the amount of text on
    > --parseopt.
    >
    > But now that I've added a "Usage text" section to looks quite in place.
    >
    > I just realized that the second patch I sent did not contain the
    > changes.  Sorry about - I will resend it.
Show 6 quoted lines
> It does not seems like there is a lot of interest, so I am not sure
> there will be a lot of discussion.
> It is a minor fix and considering the number of the emails on the
> list, I do not unexpected this kind of stuff to be very popular.
> But it seems like a valid improvement to me.
> Maybe I am missing something?

You did the right thing by sending a reminder message with a pointer to help others locate the original (like the one I am responding to), as nobody can keep up with a busy list traffic.

Show 7 quoted lines
> Same questions about this one:
>
>     [PATCH] gitk: replace SHA1 entry field on keyboard paste
>     http://www.mail-archive.com/git@vger.kernel.org/msg45040.html
>
> I think they are more or less similar, except that the second one is
> just trivial.

I do not remember if I forwarded the patch to the area maintainer Paul Mackerras <paulus@samba.org>, but if I didn't please do so yourself. The changes to gitk and git-gui come to me via their own project repositories.

Ilya Bobyr· Mar 20, 2014, 08:38 UTC · re: Junio C Hamano · lore

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

On 3/19/2014 11:46 AM, Junio C Hamano wrote:
Show 22 quoted lines
> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>> I can not find this particular patch in the latest "What's cooking" email.
>> Is there something I can do?
> IIRC, I think I was waiting for the version with a new "Usage text"
> section to the documentation you alluded to in this exchange
> ($gmane/243924):
>
>      Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>
>      > On 3/11/2014 12:10 PM, Junio C Hamano wrote:
>      >>
>      >>>> Documentation on the whole argument parsing is quite short, so,...
>      > ...
>      > I though that an example just to describe `argh' while useful would
>      > look a bit disproportional, compared to the amount of text on
>      > --parseopt.
>      >
>      > But now that I've added a "Usage text" section to looks quite in place.
>      >
>      > I just realized that the second patch I sent did not contain the
>      > changes.  Sorry about - I will resend it.

Oh %) I did sent it in the next minute. And did receive a copy myself. But it seems it never showed up in the list. I am still a bit new to the tools, maybe I did something wrong. Will try again :)

Show 9 quoted lines
>> It does not seems like there is a lot of interest, so I am not sure
>> there will be a lot of discussion.
>> It is a minor fix and considering the number of the emails on the
>> list, I do not unexpected this kind of stuff to be very popular.
>> But it seems like a valid improvement to me.
>> Maybe I am missing something?
> You did the right thing by sending a reminder message with a pointer
> to help others locate the original (like the one I am responding
> to), as nobody can keep up with a busy list traffic.
Thanks :)
Show 11 quoted lines
>> Same questions about this one:
>>
>>      [PATCH] gitk: replace SHA1 entry field on keyboard paste
>>      http://www.mail-archive.com/git@vger.kernel.org/msg45040.html
>>
>> I think they are more or less similar, except that the second one is
>> just trivial.
> I do not remember if I forwarded the patch to the area maintainer
> Paul Mackerras <paulus@samba.org>, but if I didn't please do so
> yourself.  The changes to gitk and git-gui come to me via their own
> project repositories.

You did and I even replied with additional details, that I should have included as a cover letter. I can see those messages in the web archive. It seems that Paul Mackerras gitk repository is here: git://ozlabs.org/~paulus/gitk.git At least that is what is online. I do not see the change in there. I will remind him about it.

Ilya Bobyr· Mar 20, 2014, 08:44 UTC · re: Ilya Bobyr · lore

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

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(-)
Show changes to 3 files +68 −3

Documentation/git-rev-parse.txt, builtin/rev-parse.c, t/t1502-rev-parse-parseopt.sh

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 remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
 
@@ -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:
+
+------------
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    -h, --help            show the help
+    --foo                 some nifty option --foo
+    --bar ...             some cool option --bar with an argument
+    --bar <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
+------------
+
 SQ-QUOTE
 --------
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 45901df..a4c9fdf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		usage[unb++] = strbuf_detach(&sb, NULL);
 	}
 
-	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
+	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
 	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
 		const char *s;
+		const char *end;
 		struct option *o;
 
 		if (!sb.len)
@@ -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 = ' ';
+		}
+		if (s == sb.buf)
+			s = end;
+
 		while (s > sb.buf && strchr("*=?!", s[-1])) {
 			switch (*--s) {
 			case '=':
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..bf0db05 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
     -C[...]               option C with an optional argument
     -d, --data[=...]      short and long option with an optional argument
 
+Argument hints
+    -b <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with spaces>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <a very long argument hint>
+                          a very long argument hint
+
 Extras
     --extra1              line above used to cause a segfault but no longer does
 
@@ -39,6 +50,15 @@ b,baz     a short and long option
 C?        option C with an optional argument
 d,data?   short and long option with an optional argument
 
+ Argument hints
+b=arg     short option required argument
+bar2=arg  long option required argument
+e,fuz=with_spaces  short and long option required argument
+s?some    short option optional argument
+long?data long option optional argument
+g,fluf?path     short and long option optional argument
+longest=a_very_long_argument_hint  a very long argument hint
+
 Extras
 extra1    line above used to cause a segfault but no longer does
 EOF
-- 
1.7.9
Junio C Hamano· Mar 20, 2014, 18:38 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
Show 48 quoted lines
> 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.

Show 19 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.

Show 14 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.

Ilya Bobyr· Mar 20, 2014, 23:19 UTC · re: Junio C Hamano · lore

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

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.

Ilya Bobyr· Mar 21, 2014, 07:55 UTC · re: Ilya Bobyr · lore

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

On 3/20/2014 4:19 PM, Ilya Bobyr wrote:
Show 26 quoted lines
> On 3/20/2014 11:38 AM, Junio C Hamano wrote:
>> Ilya Bobyr <ilya.bobyr@gmail.com> writes:
>>
>>> [...]
>>>     ------------
>>> -<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
s/clone/checkout/
Show 19 quoted lines
>   * 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?

"git merge" also uses equals sign in one of the argument names. That would not be possible for sh based commands either.

As a lot of commands are using dashes instead of spaces, so not supporting spaces is probably fine.

Another option I can think of is to use (or just allow) angle brackets around argument names. That would look similar to the actual output. "git shortlog" has some punctuation in an argument name, which braces would make a bit easier to read. This is how an option description would look like then:

OPTION_SPEC="\ ... S,gpg-sign?<key id> GPG sign commit from "commit" w?<w[,i1[,i2]]> "shortlog" option with a complicated argument name ... "

If there is interest in this, I could code it up and post for discussion.
> [...]
Junio C Hamano· Mar 21, 2014, 17:04 UTC · re: Ilya Bobyr · lore

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

Ilya Bobyr <ilya.bobyr@gmail.com> writes:
Show 15 quoted lines
>>> +	`<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.

That was not what I asked. I was asking if there is a precedent to use "you cannot have underscores in hint; they will be turned into spaces" quoting convention. I do not think of any (we either do a backslash-quote, c-quote inside dq-pair, or %20, depending on the context).

Personally, because these "hints" are not even hints (they are more like placeholders for value that makes it easier to refer to in the description of an option [*1*]), I wouldn't shed tears if scripted Porcelains cannot use a space in the argh. In fact, it probably makes the result harder to read and format more funnily if you had a space in the argh string, be it in a subcommand implemented in C or in a scripted Porcelain.

"An optional argh is terminated by a whitespace" is perfectly fine, and by doing so we do not have to worry about having to introduce a new quoting convention like you did, which is a big plus.

[Footnote]
*1* Perhaps like this:
	--gpg-sign[=<key-id>]
        	Sign (with the key specified with <key-id>)
Ilya Bobyr· Mar 22, 2014, 09:47 UTC · re: Junio C Hamano · lore

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

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.

Signed-off-by: Ilya Bobyr <ilya.bobyr@gmail.com>
---
 Fixed "arg_hing" typo, decoration for `-h` and `--help` and removed
 underscore replacement for the hint text.
 Documentation/git-rev-parse.txt |   34 ++++++++++++++++++++++++++++++++--
 builtin/rev-parse.c             |   13 ++++++++++++-
 t/t1502-rev-parse-parseopt.sh   |   20 ++++++++++++++++++++
 3 files changed, 64 insertions(+), 3 deletions(-)
Show changes to 3 files +64 −3

Documentation/git-rev-parse.txt, builtin/rev-parse.c, t/t1502-rev-parse-parseopt.sh

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..be85023 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_hint>`, 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 you need to use space in the
+	argument hint use dash instead.
+
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
 
@@ -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:
+
+------------
+usage: some-command [options] <args>...
+
+    some-command does foo and bar!
+
+    -h, --help            show the help
+    --foo                 some nifty option --foo
+    --bar ...             some cool option --bar with an argument
+    --bar <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
+------------
+
 SQ-QUOTE
 --------
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 45901df..1a6122d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		usage[unb++] = strbuf_detach(&sb, NULL);
 	}
 
-	/* parse: (<short>|<short>,<long>|<long>)[=?]? SP+ <help> */
+	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
 	while (strbuf_getline(&sb, stdin, '\n') != EOF) {
 		const char *s;
+		const char *end;
 		struct option *o;
 
 		if (!sb.len)
@@ -419,6 +420,16 @@ 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)
+			o->argh = xmemdupz(s, end - s);
+		if (s == sb.buf)
+			s = end;
+
 		while (s > sb.buf && strchr("*=?!", s[-1])) {
 			switch (*--s) {
 			case '=':
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..e3c6f02 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
     -C[...]               option C with an optional argument
     -d, --data[=...]      short and long option with an optional argument
 
+Argument hints
+    -b <arg>              short option required argument
+    --bar2 <arg>          long option required argument
+    -e, --fuz <with-space>
+                          short and long option required argument
+    -s[<some>]            short option optional argument
+    --long[=<data>]       long option optional argument
+    -g, --fluf[=<path>]   short and long option optional argument
+    --longest <very-long-argument-hint>
+                          a very long argument hint
+
 Extras
     --extra1              line above used to cause a segfault but no longer does
 
@@ -39,6 +50,15 @@ b,baz     a short and long option
 C?        option C with an optional argument
 d,data?   short and long option with an optional argument
 
+ Argument hints
+b=arg     short option required argument
+bar2=arg  long option required argument
+e,fuz=with-space  short and long option required argument
+s?some    short option optional argument
+long?data long option optional argument
+g,fluf?path     short and long option optional argument
+longest=very-long-argument-hint  a very long argument hint
+
 Extras
 extra1    line above used to cause a segfault but no longer does
 EOF
-- 
1.7.9
Junio C Hamano· Mar 24, 2014, 17:52 UTC · re: Ilya Bobyr · lore

[PATCH 0/3] Parse-options: spell multi-word placeholders with dashes

This is a follow-up to Ilya's 4th round of letting scripted porcelains to give argv-help to their users with their command line option parser based on "rev-parse --parseopt".

While reviewing the patch, we found that a few options to the built-in commands were described with an argv-help (the placeholder for an option parameter, e.g. "key-id" in "--gpg-sign <key-id>") that has multiple words to decribe a single entity, spelling these multiple words separated in spaces. It is more customary to spell a multi-word parameter with dashes, and the first patch in series is about making it so.

During the course of the development of the first patch, I needed a mechanical way to catch existing offenders; the last patch teaches the parse-options API implementation to find argv-help strings that contain SP or underscore.

There is one glitch, though. "update-index --cacheinfo" option takes THREE parameters: mode, sha1, and path. Because a command line option that takes multiple options is very unusual, the second patch introduces a new syntax to pass these three items as a single parameter to "--cacheinfo" option, which brings our command line argument convention more uniform and consistent. We however cannot deprecate or remove the traditional syntax, so it is still kept as an alternative "backward compatibility" syntax.

Junio C Hamano (3):
  parse-options: multi-word argh should use dash to separate words
  update-index: teach --cacheinfo a new syntax "mode,sha1,path"
  parse-options: make sure argh string does not have SP or _
 Documentation/git-cherry-pick.txt  |  6 +++---
 Documentation/git-commit.txt       |  2 +-
 Documentation/git-merge.txt        |  2 +-
 Documentation/git-notes.txt        |  2 +-
 Documentation/git-rev-parse.txt    | 16 ++++++++--------
 Documentation/git-revert.txt       |  6 +++---
 Documentation/git-update-index.txt |  8 ++++++--
 builtin/checkout.c                 |  2 +-
 builtin/commit.c                   |  2 +-
 builtin/merge.c                    |  2 +-
 builtin/notes.c                    |  2 +-
 builtin/revert.c                   |  2 +-
 builtin/tag.c                      |  2 +-
 builtin/update-index.c             | 34 +++++++++++++++++++++++++++++++---
 parse-options.c                    |  3 +++
 parse-options.h                    |  2 +-
 t/t2107-update-index-basic.sh      | 13 +++++++++++++
 17 files changed, 77 insertions(+), 29 deletions(-)
-- 
1.9.1-471-gcccbd8b
Junio C Hamano· Mar 24, 2014, 17:52 UTC · re: Junio C Hamano · lore

[PATCH 1/3] parse-options: multi-word argh should use dash to separate words

"When you need to use space, use dash" is a strange way to say that you must not use a space. Because it is more common for the command line descriptions to use dashed-multi-words, you do not even want to use spaces in these places. Rephrase the documentation to avoid this strangeness.

Fix a few existing multi-word argument help strings, i.e.
 - GPG key-ids given to -S/--gpg-sign are "key-id";
 - Refs used for storing notes are "notes-ref"; and
 - Expiry timestamps given to --expire are "expiry-date".
and update the corresponding documentation pages.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-cherry-pick.txt |  6 +++---
 Documentation/git-commit.txt      |  2 +-
 Documentation/git-merge.txt       |  2 +-
 Documentation/git-notes.txt       |  2 +-
 Documentation/git-rev-parse.txt   | 16 ++++++++--------
 Documentation/git-revert.txt      |  6 +++---
 builtin/checkout.c                |  2 +-
 builtin/commit.c                  |  2 +-
 builtin/merge.c                   |  2 +-
 builtin/notes.c                   |  2 +-
 builtin/revert.c                  |  2 +-
 builtin/tag.c                     |  2 +-
 parse-options.h                   |  2 +-
 13 files changed, 24 insertions(+), 24 deletions(-)
Show changes to 13 files +24 −22

Documentation/git-cherry-pick.txt, Documentation/git-commit.txt, Documentation/git-merge.txt, Documentation/git-notes.txt, Documentation/git-rev-parse.txt, Documentation/git-revert.txt, builtin/checkout.c, builtin/commit.c, builtin/merge.c, builtin/notes.c, builtin/revert.c, builtin/tag.c, parse-options.h

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index f1e6b2f..1c03c79 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
-		  [-S[<keyid>]] <commit>...
+		  [-S[<key-id>]] <commit>...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
 'git cherry-pick' --abort
@@ -101,8 +101,8 @@ effect to your index in a row.
 --signoff::
 	Add Signed-off-by line at the end of the commit message.
 
--S[<keyid>]::
---gpg-sign[=<keyid>]::
+-S[<key-id>]::
+--gpg-sign[=<key-id>]::
 	GPG-sign commits.
 
 --ff::
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 7c42e9c..d58758f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	   [-F <file> | -m <msg>] [--reset-author] [--allow-empty]
 	   [--allow-empty-message] [--no-verify] [-e] [--author=<author>]
 	   [--date=<date>] [--cleanup=<mode>] [--[no-]status]
-	   [-i | -o] [-S[<keyid>]] [--] [<file>...]
+	   [-i | -o] [-S[<key-id>]] [--] [<file>...]
 
 DESCRIPTION
 -----------
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4395459..a3c1fa3 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git merge' [-n] [--stat] [--no-commit] [--squash] [--[no-]edit]
-	[-s <strategy>] [-X <strategy-option>] [-S[<keyid>]]
+	[-s <strategy>] [-X <strategy-option>] [-S[<key-id>]]
 	[--[no-]rerere-autoupdate] [-m <msg>] [<commit>...]
 'git merge' <msg> HEAD <commit>...
 'git merge' --abort
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 84bb0fe..310f0a5 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git notes' append [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
 'git notes' edit [<object>]
 'git notes' show [<object>]
-'git notes' merge [-v | -q] [-s <strategy> ] <notes_ref>
+'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
 'git notes' merge --commit [-v | -q]
 'git notes' merge --abort [-v | -q]
 'git notes' remove [--ignore-missing] [--stdin] [<object>...]
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index e05e6b3..c452f33 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -290,14 +290,14 @@ The lines after the separator describe the options.
 Each line of options has this format:
 
 ------------
-<opt_spec><flags>*<arg_hint>? SP+ help LF
+<opt-spec><flags>*<arg-hint>? SP+ help LF
 ------------
 
-`<opt_spec>`::
+`<opt-spec>`::
 	its format is the short option character, then the long option name
 	separated by a comma. Both parts are not required, though at least one
 	is necessary. `h,help`, `dry-run` and `f` are all three correct
-	`<opt_spec>`.
+	`<opt-spec>`.
 
 `<flags>`::
 	`<flags>` are of `*`, `=`, `?` or `!`.
@@ -313,11 +313,11 @@ Each line of options has this format:
 
 	* Use `!` to not make the corresponding negated long option available.
 
-`<arg_hint>`::
-	`<arg_hint>`, 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 you need to use space in the
-	argument hint use dash instead.
+`<arg-hint>`::
+	`<arg-hint>`, 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.  It is customary to use a
+	dash to separate words in a multi-word argument hint.
 
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 9eb83f0..cceb5f2 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,7 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 --------
 [verse]
-'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>...
+'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<key-id>]] <commit>...
 'git revert' --continue
 'git revert' --quit
 'git revert' --abort
@@ -80,8 +80,8 @@ more details.
 This is useful when reverting more than one commits'
 effect to your index in a row.
 
--S[<keyid>]::
---gpg-sign[=<keyid>]::
+-S[<key-id>]::
+--gpg-sign[=<key-id>]::
 	GPG-sign commits.
 
 -s::
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ada51fa..a0e72d2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1095,7 +1095,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "detach", &opts.force_detach, N_("detach the HEAD at named commit")),
 		OPT_SET_INT('t', "track",  &opts.track, N_("set upstream info for new branch"),
 			BRANCH_TRACK_EXPLICIT),
-		OPT_STRING(0, "orphan", &opts.new_orphan_branch, N_("new branch"), N_("new unparented branch")),
+		OPT_STRING(0, "orphan", &opts.new_orphan_branch, N_("new-branch"), N_("new unparented branch")),
 		OPT_SET_INT('2', "ours", &opts.writeout_stage, N_("checkout our version for unmerged files"),
 			    2),
 		OPT_SET_INT('3', "theirs", &opts.writeout_stage, N_("checkout their version for unmerged files"),
diff --git a/builtin/commit.c b/builtin/commit.c
index 3783bca..96bf762 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1472,7 +1472,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("default"), N_("how to strip spaces and #comments from message")),
 		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
-		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key id"),
+		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		/* end commit message options */
 
diff --git a/builtin/merge.c b/builtin/merge.c
index f0cf120..2a144e1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -220,7 +220,7 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),
 	OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1),
-	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key id"),
+	{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
 	  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 	OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")),
 	OPT_END()
diff --git a/builtin/notes.c b/builtin/notes.c
index bb89930..39c8573 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -939,7 +939,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 	int result;
 	const char *override_notes_ref = NULL;
 	struct option options[] = {
-		OPT_STRING(0, "ref", &override_notes_ref, N_("notes_ref"),
+		OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"),
 			   N_("use notes from <notes_ref>")),
 		OPT_END()
 	};
diff --git a/builtin/revert.c b/builtin/revert.c
index 065d88d..f9ed5bd 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -89,7 +89,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_STRING(0, "strategy", &opts->strategy, N_("strategy"), N_("merge strategy")),
 		OPT_CALLBACK('X', "strategy-option", &opts, N_("option"),
 			N_("option for merge strategy"), option_parse_x),
-		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key id"),
+		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
 		OPT_END(),
 		OPT_END(),
diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..6c7c6bd 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -513,7 +513,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('s', "sign", &opt.sign, N_("annotated and GPG-signed tag")),
 		OPT_STRING(0, "cleanup", &cleanup_arg, N_("mode"),
 			N_("how to strip spaces and #comments from message")),
-		OPT_STRING('u', "local-user", &keyid, N_("key id"),
+		OPT_STRING('u', "local-user", &keyid, N_("key-id"),
 					N_("use another key to sign the tag")),
 		OPT__FORCE(&force, N_("replace the tag if exists")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
diff --git a/parse-options.h b/parse-options.h
index d670cb9..8fa02dc 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -143,7 +143,7 @@ struct option {
 	{ OPTION_CALLBACK, (s), (l), (v), N_("time"),(h), 0,	\
 	  parse_opt_approxidate_cb }
 #define OPT_EXPIRY_DATE(s, l, v, h) \
-	{ OPTION_CALLBACK, (s), (l), (v), N_("expiry date"),(h), 0,	\
+	{ OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0,	\
 	  parse_opt_expiry_date_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
-- 
1.9.1-471-gcccbd8b
Junio C Hamano· Mar 24, 2014, 17:52 UTC · re: Junio C Hamano · lore

[PATCH 2/3] update-index: teach --cacheinfo a new syntax "mode,sha1,path"

The "--cacheinfo" option is unusual in that it takes three option parameters. An option with an optional parameter is bad enough. An option with multiple parameters is simply insane.

Introduce a new syntax that takes these three things concatenated together with a comma, which makes the command line syntax more uniform across subcommands, while retaining the traditional syntax for backward compatiblity.

If we were designing the "update-index" subcommand from scratch today, it may probably have made sense to make this option (and possibly others) a command mode option that does not take any option parameter (hence no need for arg-help). But we do not live in such an ideal world, and as far as I can tell, the command still supports (and must support) mixed command modes in a single invocation, e.g.

    $ git update-index path1 --add path2 \
        --cacheinfo 100644 $(git hash-object --stdin -w <path3) path3 \
	path4

must make sure path1 is already in the index and update all of these four paths. So this is probably as far as we can go to fix this issue without risking to break people's existing scripts.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-update-index.txt |  8 ++++++--
 builtin/update-index.c             | 34 +++++++++++++++++++++++++++++++---
 t/t2107-update-index-basic.sh      | 13 +++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)
Show changes to 3 files +50 −5

Documentation/git-update-index.txt, builtin/update-index.c, t/t2107-update-index-basic.sh

diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
index e0a8702..d6de4a0 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git update-index'
 	     [--add] [--remove | --force-remove] [--replace]
 	     [--refresh] [-q] [--unmerged] [--ignore-missing]
-	     [(--cacheinfo <mode> <object> <file>)...]
+	     [(--cacheinfo <mode>,<object>,<file>)...]
 	     [--chmod=(+|-)x]
 	     [--[no-]assume-unchanged]
 	     [--[no-]skip-worktree]
@@ -68,8 +68,12 @@ OPTIONS
 --ignore-missing::
 	Ignores missing files during a --refresh
 
+--cacheinfo <mode>,<object>,<path>::
 --cacheinfo <mode> <object> <path>::
-	Directly insert the specified info into the index.
+	Directly insert the specified info into the index.  For
+	backward compatibility, you can also give these three
+	arguments as three separate parameters, but new users are
+	encouraged to use a single-parameter form.
 
 --index-info::
         Read index information from stdin.
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d12ad95..ba54e19 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -629,14 +629,42 @@ static int resolve_undo_clear_callback(const struct option *opt,
 	return 0;
 }
 
+static int parse_new_style_cacheinfo(const char *arg,
+				     unsigned int *mode,
+				     unsigned char sha1[],
+				     const char **path)
+{
+	unsigned long ul;
+	char *endp;
+
+	errno = 0;
+	ul = strtoul(arg, &endp, 8);
+	if (errno || endp == arg || *endp != ',' || (unsigned int) ul != ul)
+		return -1; /* not a new-style cacheinfo */
+	*mode = ul;
+	endp++;
+	if (get_sha1_hex(endp, sha1) || endp[40] != ',')
+		return -1;
+	*path = endp + 41;
+	return 0;
+}
+
 static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
 				const struct option *opt, int unset)
 {
 	unsigned char sha1[20];
 	unsigned int mode;
+	const char *path;
 
+	if (!parse_new_style_cacheinfo(ctx->argv[1], &mode, sha1, &path)) {
+		if (add_cacheinfo(mode, sha1, path, 0))
+			die("git update-index: --cacheinfo cannot add %s", path);
+		ctx->argv++;
+		ctx->argc--;
+		return 0;
+	}
 	if (ctx->argc <= 3)
-		return error("option 'cacheinfo' expects three arguments");
+		return error("option 'cacheinfo' expects <mode>,<sha1>,<path>");
 	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
 	    get_sha1_hex(*++ctx->argv, sha1) ||
 	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
@@ -740,9 +768,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			really_refresh_callback},
 		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
-			N_("<mode> <object> <path>"),
+			N_("<mode>,<object>,<path>"),
 			N_("add the specified entry to the index"),
-			PARSE_OPT_NOARG |	/* disallow --cacheinfo=<mode> form */
+			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			(parse_opt_cb *) cacheinfo_callback},
 		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index a6405d3..fe2fb17 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -48,4 +48,17 @@ test_expect_success '--cacheinfo does not accept gitlink null sha1' '
 	test_cmp expect actual
 '
 
+test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '
+	echo content >file &&
+	git hash-object -w --stdin <file >expect &&
+
+	git update-index --add --cacheinfo 100644 "$(cat expect)" file &&
+	git rev-parse :file >actual &&
+	test_cmp expect actual &&
+
+	git update-index --add --cacheinfo "100644,$(cat expect),elif" &&
+	git rev-parse :elif >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.9.1-471-gcccbd8b
Junio C Hamano· Mar 24, 2014, 17:52 UTC · re: Junio C Hamano · lore

[PATCH 3/3] parse-options: make sure argh string does not have SP or _

We encourage to spell an argument hint that consists of multiple words as a single-token separated with dashes. In order to help catching violations added by new callers of parse-options, make sure argh does not contain SP or _ when the code validates the option definitions.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to parse-options.c +3 −0
diff --git a/parse-options.c b/parse-options.c
index a5fa0b8..c81d3a0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -375,6 +375,9 @@ static void parse_options_check(const struct option *opts)
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
+		if (opts->argh &&
+		    strcspn(opts->argh, " _") != strlen(opts->argh))
+			err |= optbug(opts, "multi-word argh should use dash to separate words");
 	}
 	if (err)
 		exit(128);
-- 
1.9.1-471-gcccbd8b
Eric Sunshine· Mar 20, 2014, 20:18 UTC · re: Ilya Bobyr · lore

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

On Thu, Mar 20, 2014 at 4:44 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
Show 13 quoted lines
> 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.

As this is a high-traffic list, it can be difficult for reviewers to remember all the comments regarding the previous version. It can help a lot if you include a reference to the previous attempt, like this [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243216/focus=243945
One more comment below...
Show 10 quoted lines
> 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
> @@ -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
arg_hing?
Show 6 quoted lines
> +       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 remainder of the line, after stripping the spaces, is used
>  as the help associated to the option.
Ilya Bobyr· Mar 21, 2014, 03:38 UTC · re: Eric Sunshine · lore

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

On 3/20/2014 1:18 PM, Eric Sunshine wrote:
Show 18 quoted lines
> On Thu, Mar 20, 2014 at 4:44 AM, Ilya Bobyr <ilya.bobyr@gmail.com> wrote:
>> 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.
> As this is a high-traffic list, it can be difficult for reviewers to
> remember all the comments regarding the previous version. It can help
> a lot if you include a reference to the previous attempt, like this
> [1].
Got it, thanks :)
Show 5 quoted lines
>> [...]
>>
>> +`<arg_hint>`::
>> +       `<arg_hing>`, if specified, is used as a name of the argument in the
> arg_hing?
Will fix it in the next patch.

← back to recent threads