{"thread":{"id":"46822","subject":"[PATCH 1/3] t0040,t1502: Demonstrate parse_options bugs","startedAt":"2017-09-25T04:08:19Z","lastAt":"2017-09-25T05:53:35Z","messageCount":5,"participants":["Brandon Casey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"328786","messageId":"1506312485-8370-1-git-send-email-drafnel@gmail.com","threadId":"46822","inReplyTo":null,"subject":"[PATCH 1/3] t0040,t1502: Demonstrate parse_options bugs","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-25T04:08:03Z","receivedAt":"2017-09-25T04:08:19Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When the option spec contains no switches or only hidden switches,\nparse_options will emit an extra blank line at the end of help output so\nthat the help text will end in two blank lines instead of one.\n\nWhen parse_options produces internal help output after an error has\noccurred it will emit blank lines within the usage string to stdout\ninstead of stderr.\n\nUpdate t/helper/test-parse-options.c to have a description body in the\nusage string to exercise this second bug and mark tests as failing in\nt0040.\n\nAdd tests to t1502 to demonstrate both of these problems.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n\nNotes:\n    FYI: this is built on top of bc/rev-parse-parseopt-fix (697bc88) merged\n    into next.\n\n t/helper/test-parse-options.c |   2 +\n t/t0040-parse-options.sh      |   8 ++--\n t/t1502-rev-parse-parseopt.sh | 100 ++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 107 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 75fe883..630c76d 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -99,6 +99,8 @@ int cmd_main(int argc, const char **argv)\n \tconst char *prefix = \"prefix/\";\n \tconst char *usage[] = {\n \t\t\"test-parse-options <options>\",\n+\t\t\"\",\n+\t\t\"A helper function for the parse-options API.\",\n \t\tNULL\n \t};\n \tstruct string_list expect = STRING_LIST_INIT_NODUP;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 74d2cd7..a36434b 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -10,6 +10,8 @@ test_description='our own option parser'\n cat >expect <<\\EOF\n usage: test-parse-options <options>\n \n+    A helper function for the parse-options API.\n+\n     --yes                 get a boolean\n     -D, --no-doubt        begins with 'no-'\n     -B, --no-fear         be brave\n@@ -90,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'\n test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'\n test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'\n \n-test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n-test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n+test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n+test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n \n test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n@@ -286,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '\n \n >expect\n \n-test_expect_success 'OPT_CALLBACK() and callback errors work' '\n+test_expect_failure 'OPT_CALLBACK() and callback errors work' '\n \ttest_must_fail test-parse-options --no-length >output 2>output.err &&\n \ttest_i18ncmp expect output &&\n \ttest_i18ncmp expect.err output.err\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 6e1b45f..1bfa80f 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -38,6 +38,25 @@ test_expect_success 'setup optionspec' '\n EOF\n '\n \n+test_expect_success 'setup optionspec-no-switches' '\n+\tsed -e \"s/^|//\" >optionspec_no_switches <<\\EOF\n+|some-command [options] <args>...\n+|\n+|some-command does foo and bar!\n+|--\n+EOF\n+'\n+\n+test_expect_success 'setup optionspec-only-hidden-switches' '\n+\tsed -e \"s/^|//\" >optionspec_only_hidden_switches <<\\EOF\n+|some-command [options] <args>...\n+|\n+|some-command does foo and bar!\n+|--\n+|hidden1* A hidden switch\n+EOF\n+'\n+\n test_expect_success 'test --parseopt help output' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n@@ -79,6 +98,87 @@ END_EXPECT\n \ttest_i18ncmp expect output\n '\n \n+test_expect_failure 'test --parseopt help output no switches' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n+|cat <<\\EOF\n+|usage: some-command [options] <args>...\n+|\n+|    some-command does foo and bar!\n+|\n+|EOF\n+END_EXPECT\n+\ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&\n+\ttest_i18ncmp expect output\n+'\n+\n+test_expect_failure 'test --parseopt help output hidden switches' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n+|cat <<\\EOF\n+|usage: some-command [options] <args>...\n+|\n+|    some-command does foo and bar!\n+|\n+|EOF\n+END_EXPECT\n+\ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&\n+\ttest_i18ncmp expect output\n+'\n+\n+test_expect_success 'test --parseopt help-all output hidden switches' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n+|cat <<\\EOF\n+|usage: some-command [options] <args>...\n+|\n+|    some-command does foo and bar!\n+|\n+|    --hidden1             A hidden switch\n+|\n+|EOF\n+END_EXPECT\n+\ttest_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&\n+\ttest_i18ncmp expect output\n+'\n+\n+test_expect_failure 'test --parseopt invalid switch help output' '\n+\tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n+|error: unknown option `does-not-exist'\\''\n+|usage: some-command [options] <args>...\n+|\n+|    some-command does foo and bar!\n+|\n+|    -h, --help            show the help\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>\n+|                          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>\n+|                          a very long argument hint\n+|    --pair <key=value>    with an equals sign in the hint\n+|    --aswitch             help te=t contains? fl*g characters!`\n+|    --bswitch <hint>      hint has trailing tab character\n+|    --cswitch             switch has trailing tab character\n+|    --short-hint <a>      with a one symbol hint\n+|\n+|Extras\n+|    --extra1              line above used to cause a segfault but no longer does\n+|\n+END_EXPECT\n+\ttest_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&\n+\ttest_i18ncmp expect output\n+'\n+\n test_expect_success 'setup expect.1' \"\n \tcat > expect <<EOF\n set -- --foo --bar 'ham' -b --aswitch -- 'arg'\n-- \n2.2.0.rc3\n\n"},{"id":"328787","messageId":"1506312485-8370-3-git-send-email-drafnel@gmail.com","threadId":"46822","inReplyTo":"1506312485-8370-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 3/3] parse-options: only insert newline in help text if needed","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-25T04:08:05Z","receivedAt":"2017-09-25T04:08:21Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Currently, when parse_options() produces a help message it always emits\na blank line after the usage text to separate it from the options text.\nIf the option spec does not define any switches, or only defines hidden\nswitches that will not be displayed, then the help text will end up with\ntwo trailing blank lines instead of one.  Let's defer emitting the blank\nline between the usage text and the options text until it is clear that\nthe options section will not be empty.\n\nFixes t1502.5, t1502.6.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n parse-options.c               | 10 ++++++++--\n t/t1502-rev-parse-parseopt.sh |  4 ++--\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 6a03a52..fca7159 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -581,6 +581,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\t\t       const struct option *opts, int full, int err)\n {\n \tFILE *outfile = err ? stderr : stdout;\n+\tint need_newline;\n \n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n@@ -603,8 +604,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tusagestr++;\n \t}\n \n-\tif (opts->type != OPTION_GROUP)\n-\t\tfputc('\\n', outfile);\n+\tneed_newline = 1;\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tsize_t pos;\n@@ -612,6 +612,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \n \t\tif (opts->type == OPTION_GROUP) {\n \t\t\tfputc('\\n', outfile);\n+\t\t\tneed_newline = 0;\n \t\t\tif (*opts->help)\n \t\t\t\tfprintf(outfile, \"%s\\n\", _(opts->help));\n \t\t\tcontinue;\n@@ -619,6 +620,11 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (!full && (opts->flags & PARSE_OPT_HIDDEN))\n \t\t\tcontinue;\n \n+\t\tif (need_newline) {\n+\t\t\tfputc('\\n', outfile);\n+\t\t\tneed_newline = 0;\n+\t\t}\n+\n \t\tpos = fprintf(outfile, \"    \");\n \t\tif (opts->short_name) {\n \t\t\tif (opts->flags & PARSE_OPT_NODASH)\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex ce7dda1..a859abe 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -98,7 +98,7 @@ END_EXPECT\n \ttest_i18ncmp expect output\n '\n \n-test_expect_failure 'test --parseopt help output no switches' '\n+test_expect_success 'test --parseopt help output no switches' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] <args>...\n@@ -111,7 +111,7 @@ END_EXPECT\n \ttest_i18ncmp expect output\n '\n \n-test_expect_failure 'test --parseopt help output hidden switches' '\n+test_expect_success 'test --parseopt help output hidden switches' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |cat <<\\EOF\n |usage: some-command [options] <args>...\n-- \n2.2.0.rc3\n\n"},{"id":"328788","messageId":"1506312485-8370-2-git-send-email-drafnel@gmail.com","threadId":"46822","inReplyTo":"1506312485-8370-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 2/3] parse-options: write blank line to correct output stream","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2017-09-25T04:08:04Z","receivedAt":"2017-09-25T04:08:22Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When commit 54e6dc7 added translation support to parse-options, an\nfprintf was mistakenly replaced by a call to putchar().  Let's use fputc\ninstead.\n\nFixes t0040.11, t0040.12, t0040.33, and t1502.8.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n parse-options.c               | 2 +-\n t/t0040-parse-options.sh      | 6 +++---\n t/t1502-rev-parse-parseopt.sh | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0dd9fc6..6a03a52 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -599,7 +599,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (**usagestr)\n \t\t\tfprintf_ln(outfile, _(\"    %s\"), _(*usagestr));\n \t\telse\n-\t\t\tputchar('\\n');\n+\t\t\tfputc('\\n', outfile);\n \t\tusagestr++;\n \t}\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex a36434b..0c2fc81 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -92,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'\n test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'\n test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'\n \n-test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n-test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n+test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'\n+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'\n \n test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'\n \n@@ -288,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '\n \n >expect\n \n-test_expect_failure 'OPT_CALLBACK() and callback errors work' '\n+test_expect_success 'OPT_CALLBACK() and callback errors work' '\n \ttest_must_fail test-parse-options --no-length >output 2>output.err &&\n \ttest_i18ncmp expect output &&\n \ttest_i18ncmp expect.err output.err\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 1bfa80f..ce7dda1 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -139,7 +139,7 @@ END_EXPECT\n \ttest_i18ncmp expect output\n '\n \n-test_expect_failure 'test --parseopt invalid switch help output' '\n+test_expect_success 'test --parseopt invalid switch help output' '\n \tsed -e \"s/^|//\" >expect <<\\END_EXPECT &&\n |error: unknown option `does-not-exist'\\''\n |usage: some-command [options] <args>...\n-- \n2.2.0.rc3\n\n"},{"id":"328791","messageId":"xmqqing7e3qs.fsf@gitster.mtv.corp.google.com","threadId":"46822","inReplyTo":"1506312485-8370-3-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 3/3] parse-options: only insert newline in help text if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T05:39:23Z","receivedAt":"2017-09-25T05:39:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey <drafnel@gmail.com> writes:\n\n> Currently, when parse_options() produces a help message it always emits\n> a blank line after the usage text to separate it from the options text.\n> If the option spec does not define any switches, or only defines hidden\n> switches that will not be displayed, then the help text will end up with\n> two trailing blank lines instead of one.  Let's defer emitting the blank\n> line between the usage text and the options text until it is clear that\n> the options section will not be empty.\n\nThis somehow looks familiar.  I think (together with the fix in 2/3)\nthis makes it definitely better.  \n\nI also wonder if we want the final blank line, but that is sort-of a\ndifferent issue.\n\nThanks.\n"},{"id":"328792","messageId":"xmqqefqve33b.fsf@gitster.mtv.corp.google.com","threadId":"46822","inReplyTo":"xmqqing7e3qs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] parse-options: only insert newline in help text if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-25T05:53:28Z","receivedAt":"2017-09-25T05:53:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Brandon Casey <drafnel@gmail.com> writes:\n>\n>> Currently, when parse_options() produces a help message it always emits\n>> a blank line after the usage text to separate it from the options text.\n>> If the option spec does not define any switches, or only defines hidden\n>> switches that will not be displayed, then the help text will end up with\n>> two trailing blank lines instead of one.  Let's defer emitting the blank\n>> line between the usage text and the options text until it is clear that\n>> the options section will not be empty.\n>\n> This somehow looks familiar.  I think (together with the fix in 2/3)\n> this makes it definitely better.  \n>\n> I also wonder if we want the final blank line, but that is sort-of a\n> different issue.\n>\n> Thanks.\n\nOh, no wonder that this looked familiar.  It solves the same issue\nas 48b8d3cf (\"usage_with_options: omit double new line on empty\noption list\", 2017-08-25) and of course it conflicts with it.\n\nI find the solution presented with this patch is more direct and\nstraightforward, leaving less chance to future breakage.  Besides\nit comes with tests ;-), so perhaps I should drop the other one.\n\n\n"}]}