{"thread":{"id":"48099","subject":"[GSoC][PATCH v6] parse-options: do not show usage upon invalid option value","startedAt":"2018-03-20T17:50:58Z","lastAt":"2018-03-22T18:34:44Z","messageCount":4,"participants":["Paul-Sebastian Ungureanu","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"342344","messageId":"20180320175005.18759-1-ungureanupaulsebastian@gmail.com","threadId":"48099","inReplyTo":null,"subject":"[GSoC][PATCH v6] parse-options: do not show usage upon invalid option value","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-03-20T17:50:05Z","receivedAt":"2018-03-20T17:50:58Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"Usually, the usage should be shown only if the user does not know what\noptions are available. If the user specifies an invalid value, the user\nis already aware of the available options. In this case, there is no\npoint in displaying the usage anymore.\n\nThis patch applies to \"git tag --contains\", \"git branch --contains\",\n\"git branch --points-at\", \"git for-each-ref --contains\" and many more.\n\nSigned-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>\n---\n builtin/blame.c               |   1 +\n builtin/shortlog.c            |   1 +\n builtin/update-index.c        |   1 +\n parse-options.c               |  20 ++++---\n parse-options.h               |   1 +\n t/t0040-parse-options.sh      |   2 +-\n t/t0041-usage.sh              | 107 ++++++++++++++++++++++++++++++++++\n t/t3404-rebase-interactive.sh |   6 +-\n 8 files changed, 125 insertions(+), 14 deletions(-)\n create mode 100755 t/t0041-usage.sh\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 9dcb367b9..e8c6a4d6a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -729,6 +729,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n \t\tcase PARSE_OPT_HELP:\n+\t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\n \t\tcase PARSE_OPT_DONE:\n \t\t\tif (ctx.argv[0])\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex e29875b84..be4df6a03 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -283,6 +283,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\n \t\tcase PARSE_OPT_HELP:\n+\t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\n \t\tcase PARSE_OPT_DONE:\n \t\t\tgoto parse_done;\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 58d1c2d28..34adf55a7 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\tswitch (parseopt_state) {\n \t\tcase PARSE_OPT_HELP:\n+\t\tcase PARSE_OPT_ERROR:\n \t\t\texit(129);\n \t\tcase PARSE_OPT_NON_OPTION:\n \t\tcase PARSE_OPT_DONE:\ndiff --git a/parse-options.c b/parse-options.c\nindex d02eb8b01..47c09a82b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -317,14 +317,16 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,\n \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n-\tif (ambiguous_option)\n-\t\treturn error(\"Ambiguous option: %s \"\n+\tif (ambiguous_option) {\n+\t\terror(\"Ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\",\n \t\t\targ,\n \t\t\t(ambiguous_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tambiguous_option->long_name,\n \t\t\t(abbrev_flags & OPT_UNSET) ?  \"no-\" : \"\",\n \t\t\tabbrev_option->long_name);\n+\t\treturn -3;\n+\t}\n \tif (abbrev_option)\n \t\treturn get_value(p, abbrev_option, all_opts, abbrev_flags);\n \treturn -2;\n@@ -434,7 +436,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t       const char * const usagestr[])\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n-\tint err = 0;\n \n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n@@ -459,7 +460,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tctx->opt = arg + 1;\n \t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\tcase -1:\n-\t\t\t\tgoto show_usage_error;\n+\t\t\t\treturn PARSE_OPT_ERROR;\n \t\t\tcase -2:\n \t\t\t\tif (ctx->opt)\n \t\t\t\t\tcheck_typos(arg + 1, options);\n@@ -472,7 +473,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\twhile (ctx->opt) {\n \t\t\t\tswitch (parse_short_opt(ctx, options)) {\n \t\t\t\tcase -1:\n-\t\t\t\t\tgoto show_usage_error;\n+\t\t\t\t\treturn PARSE_OPT_ERROR;\n \t\t\t\tcase -2:\n \t\t\t\t\tif (internal_help && *ctx->opt == 'h')\n \t\t\t\t\t\tgoto show_usage;\n@@ -504,9 +505,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n \t\tcase -1:\n-\t\t\tgoto show_usage_error;\n+\t\t\treturn PARSE_OPT_ERROR;\n \t\tcase -2:\n \t\t\tgoto unknown;\n+\t\tcase -3:\n+\t\t\tgoto show_usage;\n \t\t}\n \t\tcontinue;\n unknown:\n@@ -517,10 +520,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t}\n \treturn PARSE_OPT_DONE;\n \n- show_usage_error:\n-\terr = 1;\n  show_usage:\n-\treturn usage_with_options_internal(ctx, usagestr, options, 0, err);\n+\treturn usage_with_options_internal(ctx, usagestr, options, 0, 0);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n@@ -539,6 +540,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n+\tcase PARSE_OPT_ERROR:\n \t\texit(129);\n \tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\ndiff --git a/parse-options.h b/parse-options.h\nindex af711227a..c77bb3b4f 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -188,6 +188,7 @@ enum {\n \tPARSE_OPT_HELP = -1,\n \tPARSE_OPT_DONE,\n \tPARSE_OPT_NON_OPTION,\n+\tPARSE_OPT_ERROR,\n \tPARSE_OPT_UNKNOWN\n };\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 0c2fc81d7..04d474c84 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -291,7 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() 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\n+\ttest_must_be_empty output.err\n '\n \n cat >expect <<\\EOF\ndiff --git a/t/t0041-usage.sh b/t/t0041-usage.sh\nnew file mode 100755\nindex 000000000..ac96bc3b9\n--- /dev/null\n+++ b/t/t0041-usage.sh\n@@ -0,0 +1,107 @@\n+#!/bin/sh\n+\n+test_description='Test commands behavior when given invalid argument value'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\ttest_commit \"v1.0\"\n+'\n+\n+test_expect_success 'tag --contains <existent_tag>' '\n+\tgit tag --contains \"v1.0\" 1>actual 2>actual.err &&\n+\tgrep \"v1.0\" actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'tag --contains <inexistent_tag>' '\n+\ttest_must_fail git tag --contains \"notag\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'tag --no-contains <existent_tag>' '\n+\tgit tag --no-contains \"v1.0\" 1>actual 2>actual.err  &&\n+\ttest_line_count = 0 actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'tag --no-contains <inexistent_tag>' '\n+\ttest_must_fail git tag --no-contains \"notag\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'tag usage error' '\n+\ttest_must_fail git tag --noopt 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'branch --contains <existent_commit>' '\n+\tgit branch --contains \"master\" 1>actual 2>actual.err &&\n+\ttest_i18ngrep \"master\" actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'branch --contains <inexistent_commit>' '\n+\ttest_must_fail git branch --no-contains \"nocommit\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'branch --no-contains <existent_commit>' '\n+\tgit branch --no-contains \"master\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'branch --no-contains <inexistent_commit>' '\n+\ttest_must_fail git branch --no-contains \"nocommit\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'branch usage error' '\n+\ttest_must_fail git branch --noopt 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'for-each-ref --contains <existent_object>' '\n+\tgit for-each-ref --contains \"master\" 1>actual 2>actual.err &&\n+\ttest_line_count = 2 actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'for-each-ref --contains <inexistent_object>' '\n+\ttest_must_fail git for-each-ref --no-contains \"noobject\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'for-each-ref --no-contains <existent_object>' '\n+\tgit for-each-ref --no-contains \"master\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_line_count = 0 actual.err\n+'\n+\n+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '\n+\ttest_must_fail git for-each-ref --no-contains \"noobject\" 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"error\" actual.err &&\n+\ttest_must_fail test_i18ngrep \"usage\" actual.err\n+'\n+\n+test_expect_success 'for-each-ref usage error' '\n+\ttest_must_fail git for-each-ref --noopt 1>actual 2>actual.err &&\n+\ttest_line_count = 0 actual &&\n+\ttest_i18ngrep \"usage\" actual.err\n+'\n+\n+test_done\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ef2887bd8..cac8b2bd8 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -919,10 +919,8 @@ test_expect_success 'rebase --exec works without -i ' '\n test_expect_success 'rebase -i --exec without <CMD>' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n-\ttest_must_fail git rebase -i --exec 2>tmp &&\n-\tsed -e \"1d\" tmp >actual &&\n-\ttest_must_fail git rebase -h >expected &&\n-\ttest_cmp expected actual &&\n+\ttest_must_fail git rebase -i --exec 2>actual &&\n+\ttest_i18ngrep \"requires a value\" actual &&\n \tgit checkout master\n '\n \n-- \n2.16.2.346.g16307f54f.dirty\n\n"},{"id":"342351","messageId":"CAPig+cT8OemuHYbkfX_zpQw47zjupDf65FKC-uC=GP+FLR62og@mail.gmail.com","threadId":"48099","inReplyTo":"20180320175005.18759-1-ungureanupaulsebastian@gmail.com","subject":"Re: [GSoC][PATCH v6] parse-options: do not show usage upon invalid option value","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-20T18:36:54Z","receivedAt":"2018-03-20T18:37:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 20, 2018 at 1:50 PM, Paul-Sebastian Ungureanu\n<ungureanupaulsebastian@gmail.com> wrote:\n> Usually, the usage should be shown only if the user does not know what\n> options are available. If the user specifies an invalid value, the user\n> is already aware of the available options. In this case, there is no\n> point in displaying the usage anymore.\n>\n> This patch applies to \"git tag --contains\", \"git branch --contains\",\n> \"git branch --points-at\", \"git for-each-ref --contains\" and many more.\n>\n> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>\n> ---\n\nAs this is a very active mailing list, with reviewers poring over many\n(sometimes dozens of) patches each day, it is easy for a reviewer to\nforget exact details of each individual submission and review comment.\nAs a patch contributor, you can help ease reviewers' burden by adding\ncommentary to your submission here, below the \"---\" line following\nyour sign-off. In particular, reviewers find it very helpful when you:\n\n* describe in some detail what changed since the previous version of\nthe patch, showing that you understood and took into consideration\neach of the review comments from the previous iteration\n\n* a link, like this[1], pointing at the previous round (and perhaps\nfurther back) so reviewers can refresh their memories about issues\nraised previously; this also helps people wanting to review this round\nwho were not involved in earlier rounds\n\n[1]: https://public-inbox.org/git/20180319185436.14309-1-ungureanupaulsebastian@gmail.com/\n"},{"id":"342432","messageId":"xmqqk1u5p9um.fsf@gitster-ct.c.googlers.com","threadId":"48099","inReplyTo":"20180320175005.18759-1-ungureanupaulsebastian@gmail.com","subject":"Re: [GSoC][PATCH v6] parse-options: do not show usage upon invalid option value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-21T18:22:25Z","receivedAt":"2018-03-21T18:22:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:\n\n> diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh\n> new file mode 100755\n> index 000000000..ac96bc3b9\n> --- /dev/null\n> +++ b/t/t0041-usage.sh\n> @@ -0,0 +1,107 @@\n> +#!/bin/sh\n> +\n> +test_description='Test commands behavior when given invalid argument value'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup ' '\n> +\ttest_commit \"v1.0\"\n> +'\n> +\n> +test_expect_success 'tag --contains <existent_tag>' '\n> +\tgit tag --contains \"v1.0\" 1>actual 2>actual.err &&\n\nIt is not wrong per-se, but >redirection without a number redirects\nfd#1, so \"1>actual\" is an unusual way to spell \">actual\".\n\n> +\tgrep \"v1.0\" actual &&\n> +\ttest_line_count = 0 actual.err\n> +'\n> +\n> +test_expect_success 'tag --contains <inexistent_tag>' '\n> +\ttest_must_fail git tag --contains \"notag\" 1>actual 2>actual.err &&\n> +\ttest_line_count = 0 actual &&\n> +\ttest_i18ngrep \"error\" actual.err &&\n> +\ttest_must_fail test_i18ngrep \"usage\" actual.err\n\ntest_must_fail mustn't be used like that for two reasons\n\n - It is to be used for \"git\" stuff, because it knows failing with\n   segfault and other failure modes are _wrong_ and should not be\n   happy even if the command \"fail\"ed.  We however do not expect\n   commands that are not git (e.g. test_i18ngrep) to require special\n   casing of the failure modes.\n\n - test_i18ngrep pretends to always \"have found match\" when running\n   under GETTEXT_POISON build, so it will pretend that usage exists\n   in actual.error.  test_must_fail will then say \"oops, the string\n   'usage' shouldn't appear in the output but it did\", which is not\n   what you want.\n\nPerhaps\n\n\ttest_i18ngrep ! \"usage\" actual.err\n\nis what you want to say here instead.\n\nIn any case, the tests got a lot cleaner compared to the previous\nround, and I feel that this patch is \"getting there\" ;-)\n\nThanks for working on it.\n"},{"id":"342535","messageId":"1521743676.8404.4.camel@gmail.com","threadId":"48099","inReplyTo":"CAPig+cT8OemuHYbkfX_zpQw47zjupDf65FKC-uC=GP+FLR62og@mail.gmail.com","subject":"Re: [GSoC][PATCH v6] parse-options: do not show usage upon invalid option value","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-03-22T18:34:36Z","receivedAt":"2018-03-22T18:34:44Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"Hi,\n\nThank you a lot for your advice! I will keep in mind your words next\ntime I will send a patch. \n\nBest regards,\nPaul Ungurenanu\n"}]}