Re: [PATCH 1/3] rev-parse: support OPT_NUMBER_CALLBACK in --parseopt

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> If the option spec is
>
> -NUM Help string
>
> then rev-parse will accept and parse -([0-9]+) and return "-NUM $1"

Even though the hardcoded "NUM" token initially gave me a knee-jerk
"Yuck" reaction, that literal option name is very unlikely to be
desired by scripts/commands for their real option names, and being
in all uppercase it is very clear that it is magic convention
between the parsing mechanism and the script it uses.

It however felt funny to me without a matching (possibly hidden)
mechanism to allow parse-options machinery to consume such an output
as its input.  In a script that uses this mechanism to parse out the
numeric option "-NUM 3" out of "git script -3" and uses that "three"
to drive an underlying command (e.g. "git grep -3"), wouldn't it be
more natural if that underlying command can be told to accept the
same notation (i.e. "git grep -NUM 3")?  For that to be consistent
with the rest of the system, "-NUM" would not be a good token; being
it multi-character, it must be "--NUM" or something with double-dash
prefix.

I kind of like the basic idea, the capability it tries to give
scripted Porcelain implementations.  But my impression is that
"rebase -i -4", which this mechanism was invented for, is not
progressing, so perhaps we should wait until the real user of this
feature appears.

Thanks.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/rev-parse.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 45901df..b37676f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -331,6 +331,8 @@ static int parseopt_dump(const struct option *o, const char *arg, int unset)
>  	struct strbuf *parsed = o->value;
>  	if (unset)
>  		strbuf_addf(parsed, " --no-%s", o->long_name);
> +	else if (o->type == OPTION_NUMBER)
> +		strbuf_addf(parsed, " -NUM");
>  	else if (o->short_name && (o->long_name == NULL || !stuck_long))
>  		strbuf_addf(parsed, " -%c", o->short_name);
>  	else
> @@ -338,7 +340,7 @@ static int parseopt_dump(const struct option *o, const char *arg, int unset)
>  	if (arg) {
>  		if (!stuck_long)
>  			strbuf_addch(parsed, ' ');
> -		else if (o->long_name)
> +		else if (o->long_name || o->type == OPTION_NUMBER)
>  			strbuf_addch(parsed, '=');
>  		sq_quote_buf(parsed, arg);
>  	}
> @@ -439,7 +441,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  
>  		if (s - sb.buf == 1) /* short option only */
>  			o->short_name = *sb.buf;
> -		else if (sb.buf[1] != ',') /* long option only */
> +		else if (s - sb.buf == 4 && !strncmp(sb.buf, "-NUM", 4)) {
> +			o->type = OPTION_NUMBER;
> +			o->flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG;
> +		} else if (sb.buf[1] != ',') /* long option only */
>  			o->long_name = xmemdupz(sb.buf, s - sb.buf);
>  		else {
>  			o->short_name = *sb.buf;