Re: [PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 13, 2015, 21:55 UTC
Message-ID: <xmqqlhejyb74.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com>

ilya.bobyr@gmail.com writes:

> Junio, thank you very much for all the comments.  I hope I have included
> all of the suggestions you made.  Please, let me know if I have missed
> anything or if there is something else you think should be improved.

There were a few that still remained, which I locally amended. Please check what is queued on 'pu'.

Show 12 quoted lines

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index c483100..2ea169d 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -311,8 +311,8 @@ Each line of options has this format:
>  `<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>`.
> +	is necessary. May not contain any of the `<flags>` characters.
> +	`h,help`, `dry-run` and `f` are all three correct `<opt-spec>`.

"are examples of correct <opt-spec>"?

Show 12 quoted lines

>  
>  `<flags>`::
>  	`<flags>` are of `*`, `=`, `?` or `!`.
> @@ -331,8 +331,9 @@ Each line of options has this format:
>  `<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.
> +	terminated by the first whitespace.  It may contain any of the
> +	`<flags>` characters after the first character. It is customary to
> +	use a dash to separate words in a multi-word argument hint.

I think no change in this hunk is necessary for two reasons:

 - You already said in <opt-spec> that any letters used for flags
   cannot be used there, implying that the way rules are described
   in the document around here is that anything is allowed unless
   explicitly prohibited, which makes "It may contain..."
   unnecessary.

 - It may be worth saying "It may not contains any whitespace", but
   that is already implied with the existing "is terminated by the
   first whitespace".

Show 10 quoted lines

> +		if (s - sb.buf == 1) /* short option only */
> +			o->short_name = *sb.buf;
> +		else if (sb.buf[1] != ',') /* long option only */
> +			o->long_name = xmemdupz(sb.buf, s - sb.buf);
> +		else {
> +			o->short_name = *sb.buf;
> +			o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
> +		}
> +
> +		/* type */

s/type/flags/?

Show 11 quoted lines

> +		while (s < help) {
> +			switch (*s++) {
>  			case '=':
>  				o->flags &= ~PARSE_OPT_NOARG;
> -				break;
> +				continue;
>  			case '?':
>  				o->flags &= ~PARSE_OPT_NOARG;
>  				o->flags |= PARSE_OPT_OPTARG;
> -				break;
> +				continue;

The updated code was a lot more pleasant read compared to the original (and the v1 patch).

Thanks.