{"thread":{"id":"39835","subject":"[PATCH] rev-parse --parseopt: allow [*=?!] in argument hints","startedAt":"2015-07-12T09:39:50Z","lastAt":"2015-07-14T08:17:44Z","messageCount":7,"participants":["ilya.bobyr@gmail.com","Junio C Hamano","Philip Oakley","Ilya Bobyr"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"266056","messageId":"1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com","threadId":"39835","inReplyTo":null,"subject":"[PATCH] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"","fromEmail":"ilya.bobyr@gmail.com","sentAt":"2015-07-12T09:39:50Z","receivedAt":"2015-07-12T09:39:50Z","isPatch":true,"sender":{"key":"ilya.bobyr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/694419?v=4"},"body":"From: Ilya Bobyr \n\nIt is not very likely that any of the \"*=?!\" Characters would be useful\nin the argument short or long names. On the other hand, there are\nalready argument hints that contain the \"=\" sign. It used to be\nimpossible to include any of the \"*=?!\" signs in the arguments hints\nbefore.\n\nAdded test case with equals sign in the argument hint and updated the\ntest to perform all the operations in test_expect_success matching the\nt\\README requirements and allowing commands like\n\n ./t1502-rev-parse-parseopt.sh --run=1-2\n\nto stop at the test case 2 without any further modification of the test\nstate area.\n\nSigned-off-by: Ilya Bobyr \n---\n builtin/rev-parse.c | 36 ++++++++--------\n t/t1502-rev-parse-parseopt.sh | 97 ++++++++++++++++++++++++++-----------------\n 2 files changed, 77 insertions(+), 56 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b623239..205ea67 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -423,17 +423,25 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\to->flags = PARSE_OPT_NOARG;\n \t\to->callback = &parseopt_dump;\n \n-\t\t/* Possible argument name hint */\n+\t\t/* parse names, type and the hint */\n \t\tend = s;\n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n-\t\t\t--s;\n-\t\tif (s != sb.buf && s != end)\n-\t\t\to->argh = xmemdupz(s, end - s);\n-\t\tif (s == sb.buf)\n-\t\t\ts = end;\n+\t\ts = sb.buf;\n+\n+\t\t/* name(s) */\n+\t\twhile (s < end && strchr(\"*=?!\", *s) == NULL)\n+\t\t\t++s;\n+\n+\t\tif (s - sb.buf == 1) /* short option only */\n+\t\t\to->short_name = *sb.buf;\n+\t\telse if (sb.buf[1] != ',') /* long option only */\n+\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n+\t\telse {\n+\t\t\to->short_name = *sb.buf;\n+\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n+\t\t}\n \n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n-\t\t\tswitch (*--s) {\n+\t\twhile (s < end && strchr(\"*=?!\", *s)) {\n+\t\t\tswitch (*s++) {\n \t\t\tcase '=':\n \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n \t\t\t\tbreak;\n@@ -450,14 +458,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \t\t}\n \n-\t\tif (s - sb.buf == 1) /* short option only */\n-\t\t\to->short_name = *sb.buf;\n-\t\telse if (sb.buf[1] != ',') /* long option only */\n-\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n-\t\telse {\n-\t\t\to->short_name = *sb.buf;\n-\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n-\t\t}\n+\t\tif (s < end)\n+\t\t\to->argh = xmemdupz(s, end - s);\n \t}\n \tstrbuf_release(&sb);\n \ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex ebe7c3b..d5e5720 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -3,7 +3,39 @@\n test_description='test git rev-parse --parseopt'\n . ./test-lib.sh\n \n-sed -e 's/^|//' >expect <<\\END_EXPECT\n+test_expect_success 'setup optionspec' '\n+\tsed -e \"s/^|//\" >optionspec <<\\EOF\n+|some-command [options] ...\n+|\n+|some-command does foo and bar!\n+|--\n+|h,help show the help\n+|\n+|foo some nifty option --foo\n+|bar= some cool option --bar with an argument\n+|b,baz a short and long option\n+|\n+| An option group Header\n+|C? option C with an optional argument\n+|d,data? short and long option with an optional argument\n+|\n+| Argument hints\n+|B=arg short option required argument\n+|bar2=arg long option required argument\n+|e,fuz=with-space short and long option required argument\n+|s?some short option optional argument\n+|long?data long option optional argument\n+|g,fluf?path short and long option optional argument\n+|longest=very-long-argument-hint a very long argument hint\n+|pair=key=value with an equals sign in the hint\n+|\n+|Extras\n+|extra1 line above used to cause a segfault but no longer does\n+EOF\n+'\n+\n+test_expect_success 'test --parseopt help output' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n@@ -28,49 +60,22 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n | -g, --fluf[=] short and long option optional argument\n | --longest \n | a very long argument hint\n+| --pair with an equals sign in the hint\n |\n |Extras\n | --extra1 line above used to cause a segfault but no longer does\n |\n |EOF\n END_EXPECT\n-\n-sed -e 's/^|//' >optionspec <<\\EOF\n-|some-command [options] ...\n-|\n-|some-command does foo and bar!\n-|--\n-|h,help show the help\n-|\n-|foo some nifty option --foo\n-|bar= some cool option --bar with an argument\n-|b,baz a short and long option\n-|\n-| An option group Header\n-|C? option C with an optional argument\n-|d,data? short and long option with an optional argument\n-|\n-| Argument hints\n-|B=arg short option required argument\n-|bar2=arg long option required argument\n-|e,fuz=with-space short and long option required argument\n-|s?some short option optional argument\n-|long?data long option optional argument\n-|g,fluf?path short and long option optional argument\n-|longest=very-long-argument-hint a very long argument hint\n-|\n-|Extras\n-|extra1 line above used to cause a segfault but no longer does\n-EOF\n-\n-test_expect_success 'test --parseopt help output' '\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&\n \ttest_i18ncmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -82,9 +87,11 @@ test_expect_success 'test --parseopt with mixed options and arguments' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -96,54 +103,66 @@ test_expect_success 'test --parseopt --stop-at-non-option' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n \ttest_cmp expect output\n '\n \n-cat >expect <expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n-- \n2.4.5\n"},{"id":"266065","messageId":"xmqqzj31i8ts.fsf@gitster.dls.corp.google.com","threadId":"39835","inReplyTo":"1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com","subject":"Re: [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-12T17:28:47Z","receivedAt":"2015-07-12T17:28:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ilya.bobyr@gmail.com writes:\n\n> From: Ilya Bobyr \n>\n> It is not very likely that any of the \"*=?!\" Characters would be useful\n> in the argument short or long names. On the other hand, there are\n> already argument hints that contain the \"=\" sign. It used to be\n> impossible to include any of the \"*=?!\" signs in the arguments hints\n> before.\n\nAfter reading this three times (without looking at the code), it is\nunclear to me what the change wants to achieve. A few points that\nconfuse me:\n\n - \"It is not very likely...\"; so what does this change do to such\n an unlikely case? Does it just forbid? Or does it have escape\n hatches?\n\n - \"... there are already ...\"; so an unlikely case already exists?\n\n - \"It used to be impossible...\"; hmmmm, it earlier said there are\n already cases there---how they have been working?\n\nPerhaps it would clarify the paragraph if you said upfront that a\nparseopt option specification is (i.e. short and long\nnames) optionally followed by (i.e. one or more of these\n\"*=?!\" characters) and the string to remind the readers\nand reviewers, and phrase what you wrote to make the differences\nbetween them stand out? \n\n A line in the input to \"rev-parse --parseopt\" describes an\n option by listing a short and/or long name, optional flags\n [*=?!], argument hint, and then whitespace and help string.\n\n The code first finds the help string and scans backwards to find\n the flags, which would imply that [*=?!] is allowed in the\n option names but not in argument hint string.\n\n That is backwards; you do not want these special characters in\n option names, but you may want to be able to include them\n (especially '=', as in 'key=value') in the argument hint string.\n\n Change the parsing to go from the beginning to find the first\n occurrence of [*=?!] to find the flags and use the remainder as\n argument hint.\n\nor something, perhaps.\n\n> Added test case with equals sign in the argument hint and updated the\n\n\"Add a test case with ... and update the test ...\". Write your log\nmessage as if you are giving somebody an order with your commit to\ndo such and such.\n\n> test to perform all the operations in test_expect_success matching the\n> t\\README requirements and allowing commands like\n\nt/README.\n\n>\n> ./t1502-rev-parse-parseopt.sh --run=1-2\n>\n> to stop at the test case 2 without any further modification of the test\n> state area.\n>\n> Signed-off-by: Ilya Bobyr \n> ---\n> builtin/rev-parse.c | 36 ++++++++--------\n> t/t1502-rev-parse-parseopt.sh | 97 ++++++++++++++++++++++++++-----------------\n> 2 files changed, 77 insertions(+), 56 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b623239..205ea67 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -423,17 +423,25 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n> \t\to->flags = PARSE_OPT_NOARG;\n> \t\to->callback = &parseopt_dump;\n> \n> -\t\t/* Possible argument name hint */\n> +\t\t/* parse names, type and the hint */\n> \t\tend = s;\n> -\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n> -\t\t\t--s;\n\nI must have overlooked this one long time ago when a patch added\nthis; it is a horrible way to parse a thing from the tail. Good to\nsee the code go ;-)\n\n> -\t\tif (s != sb.buf && s != end)\n> -\t\t\to->argh = xmemdupz(s, end - s);\n> -\t\tif (s == sb.buf)\n> -\t\t\ts = end;\n> +\t\ts = sb.buf;\n> +\n> +\t\t/* name(s) */\n> +\t\twhile (s < end && strchr(\"*=?!\", *s) == NULL)\n> +\t\t\t++s;\n\nIn C, we usually pre-decrement and post-increment, unless the value\nis used.\n\nMore importantly, can't we write this more concisely by using\nstrcspn(3)?\n\n\tconst char *flags_chars = \"*=?!\";\n size_t leading = strcspn(s, flags_chars);\n\n\tif (s + leading < end)\n \t... /* s + leading is the beginning of flags */\n\telse\n \t... /* there was no flags before end */\n\n> +\n> +\t\tif (s - sb.buf == 1) /* short option only */\n> +\t\t\to->short_name = *sb.buf;\n> +\t\telse if (sb.buf[1] != ',') /* long option only */\n> +\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n> +\t\telse {\n> +\t\t\to->short_name = *sb.buf;\n> +\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> +\t\t}\n> \n> -\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n> -\t\t\tswitch (*--s) {\n> +\t\twhile (s < end && strchr(\"*=?!\", *s)) {\n> +\t\t\tswitch (*s++) {\n> \t\t\tcase '=':\n\nNo need for the strchr() dance, I think, because you will do\ndifferent things depending on *s inside the loop anyway. Just\n\n\t\twhile (s < end) {\n \tswitch (*s++) {\n case '=':\n \tdo the \"equal\" thing;\n continue;\n\t\t\tcase '*':\n \tdo the \"asterisk\" thing;\n continue;\n ...\n\t\t\t}\n break;\n\t\t}\n\nor something.\n\nYes, I agree that the original is coded very incompetently, but\nthere is no reason to inherit that to your fixed version ;-).\n\nThanks.\n"},{"id":"266067","messageId":"E1A07B6921124F5B8B6537C21556BD86@PhilipOakley","threadId":"39835","inReplyTo":"1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com","subject":"Re: [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-07-12T18:22:34Z","receivedAt":"2015-07-12T18:22:34Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: Sent: Sunday, July 12, 2015 10:39 AM\n> From: Ilya Bobyr \n>\n> It is not very likely that any of the \"*=?!\" Characters would be \n> useful\n> in the argument short or long names. On the other hand, there are\n> already argument hints that contain the \"=\" sign. It used to be\n> impossible to include any of the \"*=?!\" signs in the arguments hints\n> before.\n\nI found this difficult to parse, and had to check the man pages to \nunderstand the proposal, which were nearly as bad (for me).\n\nPerhaps:\n\"In the 'git rev-parse' parseopt mode, the the short and long options \n cannot contain any of the terminating flag characters \"*=?!\". \nThis restriction does not apply to the argument name hint \nwhich is space terminated.\n\nAllow flag characters in the , after the arg-hint's initial \nnon flag character.\"\n\nWould that be a correct understanding?\n--\nPhilip\n\n>\n> Added test case with equals sign in the argument hint and updated the\n> test to perform all the operations in test_expect_success matching the\n> t\\README requirements and allowing commands like\n>\n> ./t1502-rev-parse-parseopt.sh --run=1-2\n>\n> to stop at the test case 2 without any further modification of the \n> test\n> state area.\n>\n> Signed-off-by: Ilya Bobyr \n> ---\n> builtin/rev-parse.c | 36 ++++++++--------\n> t/t1502-rev-parse-parseopt.sh | 97 \n> ++++++++++++++++++++++++++-----------------\n> 2 files changed, 77 insertions(+), 56 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b623239..205ea67 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -423,17 +423,25 @@ static int cmd_parseopt(int argc, const char \n> **argv, const char *prefix)\n> o->flags = PARSE_OPT_NOARG;\n> o->callback = &parseopt_dump;\n>\n> - /* Possible argument name hint */\n> + /* parse names, type and the hint */\n> end = s;\n> - while (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n> - --s;\n> - if (s != sb.buf && s != end)\n> - o->argh = xmemdupz(s, end - s);\n> - if (s == sb.buf)\n> - s = end;\n> + s = sb.buf;\n> +\n> + /* name(s) */\n> + while (s < end && strchr(\"*=?!\", *s) == NULL)\n> + ++s;\n> +\n> + if (s - sb.buf == 1) /* short option only */\n> + o->short_name = *sb.buf;\n> + else if (sb.buf[1] != ',') /* long option only */\n> + o->long_name = xmemdupz(sb.buf, s - sb.buf);\n> + else {\n> + o->short_name = *sb.buf;\n> + o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> + }\n>\n> - while (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n> - switch (*--s) {\n> + while (s < end && strchr(\"*=?!\", *s)) {\n> + switch (*s++) {\n> case '=':\n> o->flags &= ~PARSE_OPT_NOARG;\n> break;\n> @@ -450,14 +458,8 @@ static int cmd_parseopt(int argc, const char \n> **argv, const char *prefix)\n> }\n> }\n>\n> - if (s - sb.buf == 1) /* short option only */\n> - o->short_name = *sb.buf;\n> - else if (sb.buf[1] != ',') /* long option only */\n> - o->long_name = xmemdupz(sb.buf, s - sb.buf);\n> - else {\n> - o->short_name = *sb.buf;\n> - o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> - }\n> + if (s < end)\n> + o->argh = xmemdupz(s, end - s);\n> }\n> strbuf_release(&sb);\n>\n> diff --git a/t/t1502-rev-parse-parseopt.sh \n> b/t/t1502-rev-parse-parseopt.sh\n> index ebe7c3b..d5e5720 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -3,7 +3,39 @@\n> test_description='test git rev-parse --parseopt'\n> . ./test-lib.sh\n>\n> -sed -e 's/^|//' >expect <<\\END_EXPECT\n> +test_expect_success 'setup optionspec' '\n> + sed -e \"s/^|//\" >optionspec <<\\EOF\n> +|some-command [options] ...\n> +|\n> +|some-command does foo and bar!\n> +|--\n> +|h,help show the help\n> +|\n> +|foo some nifty option --foo\n> +|bar= some cool option --bar with an argument\n> +|b,baz a short and long option\n> +|\n> +| An option group Header\n> +|C? option C with an optional argument\n> +|d,data? short and long option with an optional argument\n> +|\n> +| Argument hints\n> +|B=arg short option required argument\n> +|bar2=arg long option required argument\n> +|e,fuz=with-space short and long option required argument\n> +|s?some short option optional argument\n> +|long?data long option optional argument\n> +|g,fluf?path short and long option optional argument\n> +|longest=very-long-argument-hint a very long argument hint\n> +|pair=key=value with an equals sign in the hint\n> +|\n> +|Extras\n> +|extra1 line above used to cause a segfault but no longer does\n> +EOF\n> +'\n> +\n> +test_expect_success 'test --parseopt help output' '\n> + sed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n> |cat <<\\EOF\n> |usage: some-command [options] ...\n> |\n> @@ -28,49 +60,22 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n> | -g, --fluf[=] short and long option optional argument\n> | --longest \n> | a very long argument hint\n> +| --pair with an equals sign in the hint\n> |\n> |Extras\n> | --extra1 line above used to cause a segfault but no \n> longer does\n> |\n> |EOF\n> END_EXPECT\n> -\n> -sed -e 's/^|//' >optionspec <<\\EOF\n> -|some-command [options] ...\n> -|\n> -|some-command does foo and bar!\n> -|--\n> -|h,help show the help\n> -|\n> -|foo some nifty option --foo\n> -|bar= some cool option --bar with an argument\n> -|b,baz a short and long option\n> -|\n> -| An option group Header\n> -|C? option C with an optional argument\n> -|d,data? short and long option with an optional argument\n> -|\n> -| Argument hints\n> -|B=arg short option required argument\n> -|bar2=arg long option required argument\n> -|e,fuz=with-space short and long option required argument\n> -|s?some short option optional argument\n> -|long?data long option optional argument\n> -|g,fluf?path short and long option optional argument\n> -|longest=very-long-argument-hint a very long argument hint\n> -|\n> -|Extras\n> -|extra1 line above used to cause a segfault but no longer does\n> -EOF\n> -\n> -test_expect_success 'test --parseopt help output' '\n> test_expect_code 129 git rev-parse --parseopt -- -h > output < \n> optionspec &&\n> test_i18ncmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.1' \"\n> + cat > expect < set -- --foo --bar 'ham' -b -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt' '\n> git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > \n> output &&\n> @@ -82,9 +87,11 @@ test_expect_success 'test --parseopt with mixed \n> options and arguments' '\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.2' \"\n> + cat > expect < set -- --foo -- 'arg' '--bar=ham'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt with --' '\n> git rev-parse --parseopt -- --foo -- arg --bar=ham < optionspec > \n> output &&\n> @@ -96,54 +103,66 @@ test_expect_success \n> 'test --parseopt --stop-at-non-option' '\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.3' \"\n> + cat > expect < set -- --foo -- '--' 'arg' '--bar=ham'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --keep-dashdash' '\n> git rev-parse --parseopt --keep-dashdash -- --foo -- arg --bar=ham < \n> optionspec > output &&\n> test_cmp expect output\n> '\n>\n> -cat >expect < +test_expect_success 'setup expect.4' \"\n> + cat >expect < set -- --foo -- '--' 'arg' '--spam=ham'\n> EOF\n> +\"\n>\n> test_expect_success \n> 'test --parseopt --keep-dashdash --stop-at-non-option with --' '\n> git \n> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo -- \n> arg --spam=ham output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.5' \"\n> + cat > expect < set -- --foo -- 'arg' '--spam=ham'\n> EOF\n> +\"\n>\n> test_expect_success \n> 'test --parseopt --keep-dashdash --stop-at-non-option without --' '\n> git \n> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo \n> arg --spam=ham output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.6' \"\n> + cat > expect < set -- --foo --bar='z' --baz -C'Z' --data='A' -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long' '\n> git rev-parse --parseopt --stuck-long -- --foo --bar=z -b arg -CZ -dA \n> output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.7' \"\n> + cat > expect < set -- --data='' -C --baz -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long and empty optional \n> argument' '\n> git rev-parse --parseopt --stuck-long -- --data= arg -C -b \n> output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.8' \"\n> + cat > expect < set -- --data --baz -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long and long option with \n> unset optional argument' '\n> git rev-parse --parseopt --stuck-long -- --data arg -b >output &&\n> -- \n> 2.4.5\n>\n"},{"id":"266076","messageId":"1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com","threadId":"39835","inReplyTo":"1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com","subject":"[PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"","fromEmail":"ilya.bobyr@gmail.com","sentAt":"2015-07-13T10:12:35Z","receivedAt":"2015-07-13T10:12:35Z","isPatch":false,"sender":{"key":"ilya.bobyr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/694419?v=4"},"body":"From: Ilya Bobyr \n\nA line in the input to \"rev-parse --parseopt\" describes an option by\nlisting a short and/or long name, optional flags [*=?!], argument hint, and\nthen whitespace and help string.\n\nWe did not allow any of the [*=?!] characters in the argument hints. The\nfollowing \n\n pair=key=value equals sign in the hint\n\nused to generate a help line like this:\n\n --pair=key equals sign in the hint\n\nand used to expect \"pair=key\" as the argument name.\n\nThat is not very helpful as we generally do not want any of the [*=?!]\ncharacters in the argument names. But we do want to use at least the\nequals sign in the argument hints.\n\nNow long argument names stop at the very first [*=?!] character.\n\nAdded test case with equals sign in the argument hint and updated the\ntest to perform all the operations in test_expect_success matching the\nt\\README requirements and allowing commands like\n\n ./t1502-rev-parse-parseopt.sh --run=1-2\n\nto stop at the test case 2 without any further modification of the test\nstate area.\n\nSigned-off-by: Ilya Bobyr \n---\nJunio, thank you very much for all the comments. I hope I have included\nall of the suggestions you made. Please, let me know if I have missed\nanything or if there is something else you think should be improved.\n\nI assumed that the commit description would be read by someone making\nchanges in the same area of code. So, I thought that an explanation\nsimilar to the one in the very first paragraph would be redundant.\n\nI have also made a slight addition to the man page to clarify the \nparsing, based on the Philip Oakley comment. Not sure if it is at the\nlevel Philip wants it to be. Please, let me know if you think it is still\nnot good enough.\n\n Documentation/git-rev-parse.txt | 9 ++--\n builtin/rev-parse.c | 57 +++++++++++++-----------\n t/t1502-rev-parse-parseopt.sh | 99 +++++++++++++++++++++++++----------------\n 3 files changed, 95 insertions(+), 70 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex c483100..2ea169d 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -311,8 +311,8 @@ Each line of options has this format:\n ``::\n \tits format is the short option character, then the long option name\n \tseparated by a comma. Both parts are not required, though at least one\n-\tis necessary. `h,help`, `dry-run` and `f` are all three correct\n-\t``.\n+\tis necessary. May not contain any of the `` characters.\n+\t`h,help`, `dry-run` and `f` are all three correct ``.\n \n ``::\n \t`` are of `*`, `=`, `?` or `!`.\n@@ -331,8 +331,9 @@ Each line of options has this format:\n ``::\n \t``, if specified, is used as a name of the argument in the\n \thelp output, for options that take arguments. `` is\n-\tterminated by the first whitespace. It is customary to use a\n-\tdash to separate words in a multi-word argument hint.\n+\tterminated by the first whitespace. It may contain any of the\n+\t`` characters after the first character. It is customary to\n+\tuse a dash to separate words in a multi-word argument hint.\n \n The remainder of the line, after stripping the spaces, is used\n as the help associated to the option.\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b623239..15acea4 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -371,6 +371,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tN_(\"output in stuck long form\")),\n \t\tOPT_END(),\n \t};\n+\tstatic const char * const flag_chars = \"*=?!\";\n \n \tstruct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT;\n \tconst char **usage = NULL;\n@@ -400,7 +401,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t/* parse: (|,|)[*=?!]*? SP+ */\n \twhile (strbuf_getline(&sb, stdin, '\\n') != EOF) {\n \t\tconst char *s;\n-\t\tconst char *end;\n+\t\tconst char *help;\n \t\tstruct option *o;\n \n \t\tif (!sb.len)\n@@ -410,54 +411,56 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tmemset(opts + onb, 0, sizeof(opts[onb]));\n \n \t\to = &opts[onb++];\n-\t\ts = strchr(sb.buf, ' ');\n-\t\tif (!s || *sb.buf == ' ') {\n+\t\thelp = strchr(sb.buf, ' ');\n+\t\tif (!help || *sb.buf == ' ') {\n \t\t\to->type = OPTION_GROUP;\n \t\t\to->help = xstrdup(skipspaces(sb.buf));\n \t\t\tcontinue;\n \t\t}\n \n \t\to->type = OPTION_CALLBACK;\n-\t\to->help = xstrdup(skipspaces(s));\n+\t\to->help = xstrdup(skipspaces(help));\n \t\to->value = &parsed;\n \t\to->flags = PARSE_OPT_NOARG;\n \t\to->callback = &parseopt_dump;\n \n-\t\t/* Possible argument name hint */\n-\t\tend = s;\n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n-\t\t\t--s;\n-\t\tif (s != sb.buf && s != end)\n-\t\t\to->argh = xmemdupz(s, end - s);\n-\t\tif (s == sb.buf)\n-\t\t\ts = end;\n-\n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n-\t\t\tswitch (*--s) {\n+\t\t/* name(s) */\n+\t\ts = strpbrk(sb.buf, flag_chars);\n+\t\tif (s == NULL)\n+\t\t\ts = help;\n+\n+\t\tif (s - sb.buf == 1) /* short option only */\n+\t\t\to->short_name = *sb.buf;\n+\t\telse if (sb.buf[1] != ',') /* long option only */\n+\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n+\t\telse {\n+\t\t\to->short_name = *sb.buf;\n+\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n+\t\t}\n+\n+\t\t/* type */\n+\t\twhile (s < help) {\n+\t\t\tswitch (*s++) {\n \t\t\tcase '=':\n \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '?':\n \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n \t\t\t\to->flags |= PARSE_OPT_OPTARG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '!':\n \t\t\t\to->flags |= PARSE_OPT_NONEG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '*':\n \t\t\t\to->flags |= PARSE_OPT_HIDDEN;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\t}\n+\t\t\ts--;\n+\t\t\tbreak;\n \t\t}\n \n-\t\tif (s - sb.buf == 1) /* short option only */\n-\t\t\to->short_name = *sb.buf;\n-\t\telse if (sb.buf[1] != ',') /* long option only */\n-\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n-\t\telse {\n-\t\t\to->short_name = *sb.buf;\n-\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n-\t\t}\n+\t\tif (s < help)\n+\t\t\to->argh = xmemdupz(s, help - s);\n \t}\n \tstrbuf_release(&sb);\n \ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex ebe7c3b..63392a8 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -3,7 +3,40 @@\n test_description='test git rev-parse --parseopt'\n . ./test-lib.sh\n \n-sed -e 's/^|//' >expect <<\\END_EXPECT\n+test_expect_success 'setup optionspec' '\n+\tsed -e \"s/^|//\" >optionspec <<\\EOF\n+|some-command [options] ...\n+|\n+|some-command does foo and bar!\n+|--\n+|h,help show the help\n+|\n+|foo some nifty option --foo\n+|bar= some cool option --bar with an argument\n+|b,baz a short and long option\n+|\n+| An option group Header\n+|C? option C with an optional argument\n+|d,data? short and long option with an optional argument\n+|\n+| Argument hints\n+|B=arg short option required argument\n+|bar2=arg long option required argument\n+|e,fuz=with-space short and long option required argument\n+|s?some short option optional argument\n+|long?data long option optional argument\n+|g,fluf?path short and long option optional argument\n+|longest=very-long-argument-hint a very long argument hint\n+|pair=key=value with an equals sign in the hint\n+|short-hint=a with a one simbol hint\n+|\n+|Extras\n+|extra1 line above used to cause a segfault but no longer does\n+EOF\n+'\n+\n+test_expect_success 'test --parseopt help output' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n@@ -28,49 +61,23 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n | -g, --fluf[=] short and long option optional argument\n | --longest \n | a very long argument hint\n+| --pair with an equals sign in the hint\n+| --short-hint with a one simbol hint\n |\n |Extras\n | --extra1 line above used to cause a segfault but no longer does\n |\n |EOF\n END_EXPECT\n-\n-sed -e 's/^|//' >optionspec <<\\EOF\n-|some-command [options] ...\n-|\n-|some-command does foo and bar!\n-|--\n-|h,help show the help\n-|\n-|foo some nifty option --foo\n-|bar= some cool option --bar with an argument\n-|b,baz a short and long option\n-|\n-| An option group Header\n-|C? option C with an optional argument\n-|d,data? short and long option with an optional argument\n-|\n-| Argument hints\n-|B=arg short option required argument\n-|bar2=arg long option required argument\n-|e,fuz=with-space short and long option required argument\n-|s?some short option optional argument\n-|long?data long option optional argument\n-|g,fluf?path short and long option optional argument\n-|longest=very-long-argument-hint a very long argument hint\n-|\n-|Extras\n-|extra1 line above used to cause a segfault but no longer does\n-EOF\n-\n-test_expect_success 'test --parseopt help output' '\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&\n \ttest_i18ncmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -82,9 +89,11 @@ test_expect_success 'test --parseopt with mixed options and arguments' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -96,54 +105,66 @@ test_expect_success 'test --parseopt --stop-at-non-option' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n \ttest_cmp expect output\n '\n \n-cat >expect <expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n-- \n2.4.5\n"},{"id":"266079","messageId":"FD4C89DDA11A4B20B62703A2D7B4BD10@PhilipOakley","threadId":"39835","inReplyTo":"1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com","subject":"Re: [PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-07-13T11:19:19Z","receivedAt":"2015-07-13T11:19:19Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \n> From: Ilya Bobyr \n>\n> A line in the input to \"rev-parse --parseopt\" describes an option by\n> listing a short and/or long name, optional flags [*=?!], argument \n> hint, and\n> then whitespace and help string.\n>\n> We did not allow any of the [*=?!] characters in the argument hints. \n> The\n> following \n>\n> pair=key=value equals sign in the hint\n>\n> used to generate a help line like this:\n>\n> --pair=key equals sign in the hint\n>\n> and used to expect \"pair=key\" as the argument name.\n>\n> That is not very helpful as we generally do not want any of the [*=?!]\n> characters in the argument names. But we do want to use at least the\n> equals sign in the argument hints.\n>\n> Now long argument names stop at the very first [*=?!] character.\n>\n> Added test case with equals sign in the argument hint and updated the\n> test to perform all the operations in test_expect_success matching the\n> t\\README requirements and allowing commands like\n>\n> ./t1502-rev-parse-parseopt.sh --run=1-2\n>\n> to stop at the test case 2 without any further modification of the \n> test\n> state area.\n>\n> Signed-off-by: Ilya Bobyr \n> ---\n> Junio, thank you very much for all the comments. I hope I have \n> included\n> all of the suggestions you made. Please, let me know if I have missed\n> anything or if there is something else you think should be improved.\n>\n> I assumed that the commit description would be read by someone making\n> changes in the same area of code. So, I thought that an explanation\n> similar to the one in the very first paragraph would be redundant.\n>\n> I have also made a slight addition to the man page to clarify the \n> \n> parsing, based on the Philip Oakley comment. Not sure if it is at the\n> level Philip wants it to be. Please, let me know if you think it is \n> still\n> not good enough.\n\nThe doc patch looks good. I've made one minor suggestion for clarity.\n\nI hadn't noticed the reverse order parsing that Junio pointed out. The \nnew wording makes it clear that the flag chars can only occur in the \narg-name-hint, and after the initial non-flag character.\nPhilip\n\n>\n> Documentation/git-rev-parse.txt | 9 ++--\n> builtin/rev-parse.c | 57 +++++++++++++-----------\n> t/t1502-rev-parse-parseopt.sh | 99 \n> +++++++++++++++++++++++++----------------\n> 3 files changed, 95 insertions(+), 70 deletions(-)\n>\n> diff --git a/Documentation/git-rev-parse.txt \n> b/Documentation/git-rev-parse.txt\n> index c483100..2ea169d 100644\n> --- a/Documentation/git-rev-parse.txt\n> +++ b/Documentation/git-rev-parse.txt\n> @@ -311,8 +311,8 @@ Each line of options has this format:\n> ``::\n> its format is the short option character, then the long option name\n> separated by a comma. Both parts are not required, though at least \n> one\n> - is necessary. `h,help`, `dry-run` and `f` are all three correct\n> - ``.\n> + is necessary. May not contain any of the `` characters.\n> + `h,help`, `dry-run` and `f` are all three correct ``.\n>\n> ``::\n> `` are of `*`, `=`, `?` or `!`.\n> @@ -331,8 +331,9 @@ Each line of options has this format:\n> ``::\n> ``, if specified, is used as a name of the argument in the\n> help output, for options that take arguments. `` is\n> - terminated by the first whitespace. It is customary to use a\n> - dash to separate words in a multi-word argument hint.\n> + terminated by the first whitespace. It may contain any of the\n> + `` characters after the first character. It is customary to\n\ns/the/its/ to clarify it's the first character of the hint, not of the \nflag chars ;-)\nor perhaps s/first character/first hint character/, dunno.\n\n> + use a dash to separate words in a multi-word argument hint.\n>\n> The remainder of the line, after stripping the spaces, is used\n> as the help associated to the option.\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b623239..15acea4 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -371,6 +371,7 @@ static int cmd_parseopt(int argc, const char \n> **argv, const char *prefix)\n> N_(\"output in stuck long form\")),\n> OPT_END(),\n> };\n> + static const char * const flag_chars = \"*=?!\";\n>\n> struct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT;\n> const char **usage = NULL;\n> @@ -400,7 +401,7 @@ static int cmd_parseopt(int argc, const char \n> **argv, const char *prefix)\n> /* parse: (|,|)[*=?!]*? SP+ \n> */\n> while (strbuf_getline(&sb, stdin, '\\n') != EOF) {\n> const char *s;\n> - const char *end;\n> + const char *help;\n> struct option *o;\n>\n> if (!sb.len)\n> @@ -410,54 +411,56 @@ static int cmd_parseopt(int argc, const char \n> **argv, const char *prefix)\n> memset(opts + onb, 0, sizeof(opts[onb]));\n>\n> o = &opts[onb++];\n> - s = strchr(sb.buf, ' ');\n> - if (!s || *sb.buf == ' ') {\n> + help = strchr(sb.buf, ' ');\n> + if (!help || *sb.buf == ' ') {\n> o->type = OPTION_GROUP;\n> o->help = xstrdup(skipspaces(sb.buf));\n> continue;\n> }\n>\n> o->type = OPTION_CALLBACK;\n> - o->help = xstrdup(skipspaces(s));\n> + o->help = xstrdup(skipspaces(help));\n> o->value = &parsed;\n> o->flags = PARSE_OPT_NOARG;\n> o->callback = &parseopt_dump;\n>\n> - /* Possible argument name hint */\n> - end = s;\n> - while (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n> - --s;\n> - if (s != sb.buf && s != end)\n> - o->argh = xmemdupz(s, end - s);\n> - if (s == sb.buf)\n> - s = end;\n> -\n> - while (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n> - switch (*--s) {\n> + /* name(s) */\n> + s = strpbrk(sb.buf, flag_chars);\n> + if (s == NULL)\n> + s = help;\n> +\n> + if (s - sb.buf == 1) /* short option only */\n> + o->short_name = *sb.buf;\n> + else if (sb.buf[1] != ',') /* long option only */\n> + o->long_name = xmemdupz(sb.buf, s - sb.buf);\n> + else {\n> + o->short_name = *sb.buf;\n> + o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> + }\n> +\n> + /* type */\n> + while (s < help) {\n> + switch (*s++) {\n> case '=':\n> o->flags &= ~PARSE_OPT_NOARG;\n> - break;\n> + continue;\n> case '?':\n> o->flags &= ~PARSE_OPT_NOARG;\n> o->flags |= PARSE_OPT_OPTARG;\n> - break;\n> + continue;\n> case '!':\n> o->flags |= PARSE_OPT_NONEG;\n> - break;\n> + continue;\n> case '*':\n> o->flags |= PARSE_OPT_HIDDEN;\n> - break;\n> + continue;\n> }\n> + s--;\n> + break;\n> }\n>\n> - if (s - sb.buf == 1) /* short option only */\n> - o->short_name = *sb.buf;\n> - else if (sb.buf[1] != ',') /* long option only */\n> - o->long_name = xmemdupz(sb.buf, s - sb.buf);\n> - else {\n> - o->short_name = *sb.buf;\n> - o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> - }\n> + if (s < help)\n> + o->argh = xmemdupz(s, help - s);\n> }\n> strbuf_release(&sb);\n>\n> diff --git a/t/t1502-rev-parse-parseopt.sh \n> b/t/t1502-rev-parse-parseopt.sh\n> index ebe7c3b..63392a8 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -3,7 +3,40 @@\n> test_description='test git rev-parse --parseopt'\n> . ./test-lib.sh\n>\n> -sed -e 's/^|//' >expect <<\\END_EXPECT\n> +test_expect_success 'setup optionspec' '\n> + sed -e \"s/^|//\" >optionspec <<\\EOF\n> +|some-command [options] ...\n> +|\n> +|some-command does foo and bar!\n> +|--\n> +|h,help show the help\n> +|\n> +|foo some nifty option --foo\n> +|bar= some cool option --bar with an argument\n> +|b,baz a short and long option\n> +|\n> +| An option group Header\n> +|C? option C with an optional argument\n> +|d,data? short and long option with an optional argument\n> +|\n> +| Argument hints\n> +|B=arg short option required argument\n> +|bar2=arg long option required argument\n> +|e,fuz=with-space short and long option required argument\n> +|s?some short option optional argument\n> +|long?data long option optional argument\n> +|g,fluf?path short and long option optional argument\n> +|longest=very-long-argument-hint a very long argument hint\n> +|pair=key=value with an equals sign in the hint\n> +|short-hint=a with a one simbol hint\n> +|\n> +|Extras\n> +|extra1 line above used to cause a segfault but no longer does\n> +EOF\n> +'\n> +\n> +test_expect_success 'test --parseopt help output' '\n> + sed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n> |cat <<\\EOF\n> |usage: some-command [options] ...\n> |\n> @@ -28,49 +61,23 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n> | -g, --fluf[=] short and long option optional argument\n> | --longest \n> | a very long argument hint\n> +| --pair with an equals sign in the hint\n> +| --short-hint with a one simbol hint\n> |\n> |Extras\n> | --extra1 line above used to cause a segfault but no \n> longer does\n> |\n> |EOF\n> END_EXPECT\n> -\n> -sed -e 's/^|//' >optionspec <<\\EOF\n> -|some-command [options] ...\n> -|\n> -|some-command does foo and bar!\n> -|--\n> -|h,help show the help\n> -|\n> -|foo some nifty option --foo\n> -|bar= some cool option --bar with an argument\n> -|b,baz a short and long option\n> -|\n> -| An option group Header\n> -|C? option C with an optional argument\n> -|d,data? short and long option with an optional argument\n> -|\n> -| Argument hints\n> -|B=arg short option required argument\n> -|bar2=arg long option required argument\n> -|e,fuz=with-space short and long option required argument\n> -|s?some short option optional argument\n> -|long?data long option optional argument\n> -|g,fluf?path short and long option optional argument\n> -|longest=very-long-argument-hint a very long argument hint\n> -|\n> -|Extras\n> -|extra1 line above used to cause a segfault but no longer does\n> -EOF\n> -\n> -test_expect_success 'test --parseopt help output' '\n> test_expect_code 129 git rev-parse --parseopt -- -h > output < \n> optionspec &&\n> test_i18ncmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.1' \"\n> + cat > expect < set -- --foo --bar 'ham' -b -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt' '\n> git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > \n> output &&\n> @@ -82,9 +89,11 @@ test_expect_success 'test --parseopt with mixed \n> options and arguments' '\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.2' \"\n> + cat > expect < set -- --foo -- 'arg' '--bar=ham'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt with --' '\n> git rev-parse --parseopt -- --foo -- arg --bar=ham < optionspec > \n> output &&\n> @@ -96,54 +105,66 @@ test_expect_success \n> 'test --parseopt --stop-at-non-option' '\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.3' \"\n> + cat > expect < set -- --foo -- '--' 'arg' '--bar=ham'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --keep-dashdash' '\n> git rev-parse --parseopt --keep-dashdash -- --foo -- arg --bar=ham < \n> optionspec > output &&\n> test_cmp expect output\n> '\n>\n> -cat >expect < +test_expect_success 'setup expect.4' \"\n> + cat >expect < set -- --foo -- '--' 'arg' '--spam=ham'\n> EOF\n> +\"\n>\n> test_expect_success \n> 'test --parseopt --keep-dashdash --stop-at-non-option with --' '\n> git \n> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo -- \n> arg --spam=ham output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.5' \"\n> + cat > expect < set -- --foo -- 'arg' '--spam=ham'\n> EOF\n> +\"\n>\n> test_expect_success \n> 'test --parseopt --keep-dashdash --stop-at-non-option without --' '\n> git \n> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo \n> arg --spam=ham output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.6' \"\n> + cat > expect < set -- --foo --bar='z' --baz -C'Z' --data='A' -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long' '\n> git rev-parse --parseopt --stuck-long -- --foo --bar=z -b arg -CZ -dA \n> output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.7' \"\n> + cat > expect < set -- --data='' -C --baz -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long and empty optional \n> argument' '\n> git rev-parse --parseopt --stuck-long -- --data= arg -C -b \n> output &&\n> test_cmp expect output\n> '\n>\n> -cat > expect < +test_expect_success 'setup expect.8' \"\n> + cat > expect < set -- --data --baz -- 'arg'\n> EOF\n> +\"\n>\n> test_expect_success 'test --parseopt --stuck-long and long option with \n> unset optional argument' '\n> git rev-parse --parseopt --stuck-long -- --data arg -b >output &&\n> -- \n> 2.4.5\n>\n> \n"},{"id":"266098","messageId":"xmqqlhejyb74.fsf@gitster.dls.corp.google.com","threadId":"39835","inReplyTo":"1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com","subject":"Re: [PATCHv2] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-13T21:55:27Z","receivedAt":"2015-07-13T21:55:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ilya.bobyr@gmail.com writes:\n\n> Junio, thank you very much for all the comments. I hope I have included\n> all of the suggestions you made. Please, let me know if I have missed\n> anything or if there is something else you think should be improved.\n\nThere were a few that still remained, which I locally amended.\nPlease check what is queued on 'pu'.\n\n> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\n> index c483100..2ea169d 100644\n> --- a/Documentation/git-rev-parse.txt\n> +++ b/Documentation/git-rev-parse.txt\n> @@ -311,8 +311,8 @@ Each line of options has this format:\n> ``::\n> \tits format is the short option character, then the long option name\n> \tseparated by a comma. Both parts are not required, though at least one\n> -\tis necessary. `h,help`, `dry-run` and `f` are all three correct\n> -\t``.\n> +\tis necessary. May not contain any of the `` characters.\n> +\t`h,help`, `dry-run` and `f` are all three correct ``.\n\n\"are examples of correct \"?\n\n> \n> ``::\n> \t`` are of `*`, `=`, `?` or `!`.\n> @@ -331,8 +331,9 @@ Each line of options has this format:\n> ``::\n> \t``, if specified, is used as a name of the argument in the\n> \thelp output, for options that take arguments. `` is\n> -\tterminated by the first whitespace. It is customary to use a\n> -\tdash to separate words in a multi-word argument hint.\n> +\tterminated by the first whitespace. It may contain any of the\n> +\t`` characters after the first character. It is customary to\n> +\tuse a dash to separate words in a multi-word argument hint.\n\nI think no change in this hunk is necessary for two reasons:\n\n - You already said in that any letters used for flags\n cannot be used there, implying that the way rules are described\n in the document around here is that anything is allowed unless\n explicitly prohibited, which makes \"It may contain...\"\n unnecessary.\n\n - It may be worth saying \"It may not contains any whitespace\", but\n that is already implied with the existing \"is terminated by the\n first whitespace\".\n\n> +\t\tif (s - sb.buf == 1) /* short option only */\n> +\t\t\to->short_name = *sb.buf;\n> +\t\telse if (sb.buf[1] != ',') /* long option only */\n> +\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n> +\t\telse {\n> +\t\t\to->short_name = *sb.buf;\n> +\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> +\t\t}\n> +\n> +\t\t/* type */\n\ns/type/flags/?\n\n> +\t\twhile (s < help) {\n> +\t\t\tswitch (*s++) {\n> \t\t\tcase '=':\n> \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n> -\t\t\t\tbreak;\n> +\t\t\t\tcontinue;\n> \t\t\tcase '?':\n> \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n> \t\t\t\to->flags |= PARSE_OPT_OPTARG;\n> -\t\t\t\tbreak;\n> +\t\t\t\tcontinue;\n\nThe updated code was a lot more pleasant read compared to the\noriginal (and the v1 patch).\n\nThanks.\n"},{"id":"266106","messageId":"1436861864-192-1-git-send-email-ilya.bobyr@gmail.com","threadId":"39835","inReplyTo":"1436782355-3576-1-git-send-email-ilya.bobyr@gmail.com","subject":"[PATCHv3] rev-parse --parseopt: allow [*=?!] in argument hints","fromName":"Ilya Bobyr","fromEmail":"ilya.bobyr@gmail.com","sentAt":"2015-07-14T08:17:44Z","receivedAt":"2015-07-14T08:17:44Z","isPatch":false,"sender":{"key":"ilya.bobyr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/694419?v=4"},"body":"A line in the input to \"rev-parse --parseopt\" describes an option by\nlisting a short and/or long name, optional flags [*=?!], argument hint,\nand then whitespace and help string.\n\nWe did not allow any of the [*=?!] characters in the argument hints.\nThe following input\n\n pair=key=value equals sign in the hint\n\nused to generate a help line like this:\n\n --pair=key equals sign in the hint\n\nand used to expect \"pair=key\" as the argument name.\n\nThat is not very helpful as we generally do not want any of the [*=?!]\ncharacters in the argument names. But we do want to use at least the\nequals sign in the argument hints.\n\nUpdate the parser to make long argument names stop at the first [*=?!]\ncharacter.\n\nAdd test case with equals sign in the argument hint and updated the test\nto perform all the operations in test_expect_success matching the\nt/README requirements and allowing commands like\n\n ./t1502-rev-parse-parseopt.sh --run=1-2\n\nto stop at the test case 2 without any further modification of the test\nstate area.\n\nSigned-off-by: Ilya Bobyr \n---\n\nOn 7/13/2015 2:55 PM, Junio C Hamano wrote:\n> ilya.bobyr@gmail.com writes:\n>\n>> Junio, thank you very much for all the comments. I hope I have included\n>> all of the suggestions you made. Please, let me know if I have missed\n>> anything or if there is something else you think should be improved.\n>\n> There were a few that still remained, which I locally amended.\n> Please check what is queued on 'pu'.\n\nSorry about that %)\nI have included add the changes from 'pu' into this patch as well as your\ncomments.\n\n>> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\n>> index c483100..2ea169d 100644\n>> --- a/Documentation/git-rev-parse.txt\n>> +++ b/Documentation/git-rev-parse.txt\n>> @@ -311,8 +311,8 @@ Each line of options has this format:\n>> ``::\n>> \tits format is the short option character, then the long option name\n>> \tseparated by a comma. Both parts are not required, though at least one\n>> -\tis necessary. `h,help`, `dry-run` and `f` are all three correct\n>> -\t``.\n>> +\tis necessary. May not contain any of the `` characters.\n>> +\t`h,help`, `dry-run` and `f` are all three correct ``.\n>\n> \"are examples of correct \"?\n\nFixed.\n\n>> \n>> ``::\n>> \t`` are of `*`, `=`, `?` or `!`.\n>> @@ -331,8 +331,9 @@ Each line of options has this format:\n>> ``::\n>> \t``, if specified, is used as a name of the argument in the\n>> \thelp output, for options that take arguments. `` is\n>> -\tterminated by the first whitespace. It is customary to use a\n>> -\tdash to separate words in a multi-word argument hint.\n>> +\tterminated by the first whitespace. It may contain any of the\n>> +\t`` characters after the first character. It is customary to\n>> +\tuse a dash to separate words in a multi-word argument hint.\n>\n> I think no change in this hunk is necessary for two reasons:\n>\n> - You already said in that any letters used for flags\n> cannot be used there, implying that the way rules are described\n> in the document around here is that anything is allowed unless\n> explicitly prohibited, which makes \"It may contain...\"\n> unnecessary.\n>\n> - It may be worth saying \"It may not contains any whitespace\", but\n> that is already implied with the existing \"is terminated by the\n> first whitespace\".\n\nI thought that if someone is reading only the part it makes it\neasier to understan what can they use. If one reads description of all of\nthe pieces it is indeed redundant.\n\nI have removed this hunk from the patch.\n\n>> +\t\tif (s - sb.buf == 1) /* short option only */\n>> +\t\t\to->short_name = *sb.buf;\n>> +\t\telse if (sb.buf[1] != ',') /* long option only */\n>> +\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n>> +\t\telse {\n>> +\t\t\to->short_name = *sb.buf;\n>> +\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n>> +\t\t}\n>> +\n>> +\t\t/* type */\n>\n> s/type/flags/?\n\nFixed.\n\n>> +\t\twhile (s < help) {\n>> +\t\t\tswitch (*s++) {\n>> \t\t\tcase '=':\n>> \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n>> -\t\t\t\tbreak;\n>> +\t\t\t\tcontinue;\n>> \t\t\tcase '?':\n>> \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n>> \t\t\t\to->flags |= PARSE_OPT_OPTARG;\n>> -\t\t\t\tbreak;\n>> +\t\t\t\tcontinue;\n>\n> The updated code was a lot more pleasant read compared to the\n> original (and the v1 patch).\n\nAgreed :)\n\n\nOn 7/13/2015 4:19 AM, Philip Oakley wrote:\n> From: \n>> From: Ilya Bobyr \n>>\n>> [...]\n>>\n>> I have also made a slight addition to the man page to clarify the \n>> parsing, based on the Philip Oakley comment. Not sure if it is at the\n>> level Philip wants it to be. Please, let me know if you think it is still\n>> not good enough. \n>\n> The doc patch looks good. I've made one minor suggestion for clarity.\n>\n> I hadn't noticed the reverse order parsing that Junio pointed out. The\n> new wording makes it clear that the flag chars can only occur in the\n> arg-name-hint, and after the initial non-flag character. \n\nI thought that the fact that it used to parse in reverse is kind of a low\nlevel implementation detail. And an example that I have inserted instead\nmakes it easier to understand what the change is about. After all, the\nfact that it used to parse in reverse, I think, is of no real value after\nthe change is in place.\n\n\n Documentation/git-rev-parse.txt | 4 +-\n builtin/rev-parse.c | 57 +++++++++++++-----------\n t/t1502-rev-parse-parseopt.sh | 99 +++++++++++++++++++++++++----------------\n 3 files changed, 92 insertions(+), 68 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex c483100..b6c6326 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -311,8 +311,8 @@ Each line of options has this format:\n ``::\n \tits format is the short option character, then the long option name\n \tseparated by a comma. Both parts are not required, though at least one\n-\tis necessary. `h,help`, `dry-run` and `f` are all three correct\n-\t``.\n+\tis necessary. May not contain any of the `` characters.\n+\t`h,help`, `dry-run` and `f` are examples of correct ``.\n \n ``::\n \t`` are of `*`, `=`, `?` or `!`.\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b623239..02d747d 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -371,6 +371,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tN_(\"output in stuck long form\")),\n \t\tOPT_END(),\n \t};\n+\tstatic const char * const flag_chars = \"*=?!\";\n \n \tstruct strbuf sb = STRBUF_INIT, parsed = STRBUF_INIT;\n \tconst char **usage = NULL;\n@@ -400,7 +401,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t/* parse: (|,|)[*=?!]*? SP+ */\n \twhile (strbuf_getline(&sb, stdin, '\\n') != EOF) {\n \t\tconst char *s;\n-\t\tconst char *end;\n+\t\tconst char *help;\n \t\tstruct option *o;\n \n \t\tif (!sb.len)\n@@ -410,54 +411,56 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tmemset(opts + onb, 0, sizeof(opts[onb]));\n \n \t\to = &opts[onb++];\n-\t\ts = strchr(sb.buf, ' ');\n-\t\tif (!s || *sb.buf == ' ') {\n+\t\thelp = strchr(sb.buf, ' ');\n+\t\tif (!help || *sb.buf == ' ') {\n \t\t\to->type = OPTION_GROUP;\n \t\t\to->help = xstrdup(skipspaces(sb.buf));\n \t\t\tcontinue;\n \t\t}\n \n \t\to->type = OPTION_CALLBACK;\n-\t\to->help = xstrdup(skipspaces(s));\n+\t\to->help = xstrdup(skipspaces(help));\n \t\to->value = &parsed;\n \t\to->flags = PARSE_OPT_NOARG;\n \t\to->callback = &parseopt_dump;\n \n-\t\t/* Possible argument name hint */\n-\t\tend = s;\n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1]) == NULL)\n-\t\t\t--s;\n-\t\tif (s != sb.buf && s != end)\n-\t\t\to->argh = xmemdupz(s, end - s);\n-\t\tif (s == sb.buf)\n-\t\t\ts = end;\n-\n-\t\twhile (s > sb.buf && strchr(\"*=?!\", s[-1])) {\n-\t\t\tswitch (*--s) {\n+\t\t/* name(s) */\n+\t\ts = strpbrk(sb.buf, flag_chars);\n+\t\tif (s == NULL)\n+\t\t\ts = help;\n+\n+\t\tif (s - sb.buf == 1) /* short option only */\n+\t\t\to->short_name = *sb.buf;\n+\t\telse if (sb.buf[1] != ',') /* long option only */\n+\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n+\t\telse {\n+\t\t\to->short_name = *sb.buf;\n+\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n+\t\t}\n+\n+\t\t/* flags */\n+\t\twhile (s < help) {\n+\t\t\tswitch (*s++) {\n \t\t\tcase '=':\n \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '?':\n \t\t\t\to->flags &= ~PARSE_OPT_NOARG;\n \t\t\t\to->flags |= PARSE_OPT_OPTARG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '!':\n \t\t\t\to->flags |= PARSE_OPT_NONEG;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\tcase '*':\n \t\t\t\to->flags |= PARSE_OPT_HIDDEN;\n-\t\t\t\tbreak;\n+\t\t\t\tcontinue;\n \t\t\t}\n+\t\t\ts--;\n+\t\t\tbreak;\n \t\t}\n \n-\t\tif (s - sb.buf == 1) /* short option only */\n-\t\t\to->short_name = *sb.buf;\n-\t\telse if (sb.buf[1] != ',') /* long option only */\n-\t\t\to->long_name = xmemdupz(sb.buf, s - sb.buf);\n-\t\telse {\n-\t\t\to->short_name = *sb.buf;\n-\t\t\to->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n-\t\t}\n+\t\tif (s < help)\n+\t\t\to->argh = xmemdupz(s, help - s);\n \t}\n \tstrbuf_release(&sb);\n \ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex ebe7c3b..63392a8 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -3,7 +3,40 @@\n test_description='test git rev-parse --parseopt'\n . ./test-lib.sh\n \n-sed -e 's/^|//' >expect <<\\END_EXPECT\n+test_expect_success 'setup optionspec' '\n+\tsed -e \"s/^|//\" >optionspec <<\\EOF\n+|some-command [options] ...\n+|\n+|some-command does foo and bar!\n+|--\n+|h,help show the help\n+|\n+|foo some nifty option --foo\n+|bar= some cool option --bar with an argument\n+|b,baz a short and long option\n+|\n+| An option group Header\n+|C? option C with an optional argument\n+|d,data? short and long option with an optional argument\n+|\n+| Argument hints\n+|B=arg short option required argument\n+|bar2=arg long option required argument\n+|e,fuz=with-space short and long option required argument\n+|s?some short option optional argument\n+|long?data long option optional argument\n+|g,fluf?path short and long option optional argument\n+|longest=very-long-argument-hint a very long argument hint\n+|pair=key=value with an equals sign in the hint\n+|short-hint=a with a one simbol hint\n+|\n+|Extras\n+|extra1 line above used to cause a segfault but no longer does\n+EOF\n+'\n+\n+test_expect_success 'test --parseopt help output' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n@@ -28,49 +61,23 @@ sed -e 's/^|//' >expect <<\\END_EXPECT\n | -g, --fluf[=] short and long option optional argument\n | --longest \n | a very long argument hint\n+| --pair with an equals sign in the hint\n+| --short-hint with a one simbol hint\n |\n |Extras\n | --extra1 line above used to cause a segfault but no longer does\n |\n |EOF\n END_EXPECT\n-\n-sed -e 's/^|//' >optionspec <<\\EOF\n-|some-command [options] ...\n-|\n-|some-command does foo and bar!\n-|--\n-|h,help show the help\n-|\n-|foo some nifty option --foo\n-|bar= some cool option --bar with an argument\n-|b,baz a short and long option\n-|\n-| An option group Header\n-|C? option C with an optional argument\n-|d,data? short and long option with an optional argument\n-|\n-| Argument hints\n-|B=arg short option required argument\n-|bar2=arg long option required argument\n-|e,fuz=with-space short and long option required argument\n-|s?some short option optional argument\n-|long?data long option optional argument\n-|g,fluf?path short and long option optional argument\n-|longest=very-long-argument-hint a very long argument hint\n-|\n-|Extras\n-|extra1 line above used to cause a segfault but no longer does\n-EOF\n-\n-test_expect_success 'test --parseopt help output' '\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&\n \ttest_i18ncmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -82,9 +89,11 @@ test_expect_success 'test --parseopt with mixed options and arguments' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n@@ -96,54 +105,66 @@ test_expect_success 'test --parseopt --stop-at-non-option' '\n \ttest_cmp expect output\n '\n \n-cat > expect < expect < output &&\n \ttest_cmp expect output\n '\n \n-cat >expect <expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n \ttest_cmp expect output\n '\n \n-cat > expect < expect <output &&\n-- \n2.4.5\n"}]}