git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: ilya.bobyr@gmail.com
Cc: git@vger.kernel.org, Pierre Habouzit <madcoder@debian.org>,
	Philip Oakley <philipoakley@iee.org>
Subject: Re: [PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints
Date: Mon, 13 Jul 2015 14:55:27 -0700	[thread overview]
Message-ID: <xmqqlhejyb74.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com> (ilya bobyr's message of "Mon, 13 Jul 2015 03:12:35 -0700")

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'.

> 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>"?

>  
>  `<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".

> +		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/?

> +		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.

  parent reply	other threads:[~2015-07-13 21:55 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-12  9:39 [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints ilya.bobyr
2015-07-12 17:28 ` Junio C Hamano
2015-07-12 18:22 ` Philip Oakley
2015-07-13 10:12 ` [PATCHv2] " ilya.bobyr
2015-07-13 11:19   ` Philip Oakley
2015-07-13 21:55   ` Junio C Hamano [this message]
2015-07-14  8:17   ` [PATCHv3] " Ilya Bobyr

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlhejyb74.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=ilya.bobyr@gmail.com \
    --cc=madcoder@debian.org \
    --cc=philipoakley@iee.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).