Re: [PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:45
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'.
quoted hunk
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>"?
quoted hunk
`<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.