ilya.bobyr@xxxxxxxxx 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html