{"thread":{"id":"47971","subject":"[GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid","startedAt":"2018-03-03T21:10:04Z","lastAt":"2018-03-06T19:45:46Z","messageCount":3,"participants":["Paul-Sebastian Ungureanu","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"340960","messageId":"20180303210938.32474-1-ungureanupaulsebastian@gmail.com","threadId":"47971","inReplyTo":null,"subject":"[GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-03-03T21:09:38Z","receivedAt":"2018-03-03T21:10:04Z","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/update-index.c        |  2 +\n parse-options.c               | 13 +++--\n parse-options.h               |  1 +\n t/t0040-parse-options.sh      |  9 ++--\n t/t3404-rebase-interactive.sh |  2 +-\n t/t3502-cherry-pick-merge.sh  |  8 +--\n t/tcontains.sh                | 92 +++++++++++++++++++++++++++++++++++\n 7 files changed, 111 insertions(+), 16 deletions(-)\n create mode 100755 t/tcontains.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 58d1c2d28..eeee1c170 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1060,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tswitch (parseopt_state) {\n \t\tcase PARSE_OPT_HELP:\n \t\t\texit(129);\n+\t\tcase PARSE_OPT_ERROR:\n+\t\t\texit(1);\n \t\tcase PARSE_OPT_NON_OPTION:\n \t\tcase PARSE_OPT_DONE:\n \t\t{\ndiff --git a/parse-options.c b/parse-options.c\nindex d02eb8b01..eee401662 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -434,7 +434,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 +458,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 +471,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,7 +503,7 @@ 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\t}\n@@ -517,10 +516,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@@ -543,6 +540,8 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n+\tcase PARSE_OPT_ERROR:\n+\t\texit(1);\n \tdefault: /* PARSE_OPT_UNKNOWN */\n \t\tif (ctx.argv[0][1] == '-') {\n \t\t\terror(\"unknown option `%s'\", ctx.argv[0] + 2);\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..8af12e8a1 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -162,9 +162,9 @@ test_expect_success 'long options' '\n '\n \n test_expect_success 'missing required value' '\n-\ttest_expect_code 129 test-parse-options -s &&\n-\ttest_expect_code 129 test-parse-options --string &&\n-\ttest_expect_code 129 test-parse-options --file\n+\ttest_expect_code 1 test-parse-options -s &&\n+\ttest_expect_code 1 test-parse-options --string &&\n+\ttest_expect_code 1 test-parse-options --file\n '\n \n cat >expect <<\\EOF\n@@ -214,7 +214,7 @@ test_expect_success 'unambiguously abbreviated option with \"=\"' '\n '\n \n test_expect_success 'ambiguously abbreviated option' '\n-\ttest_expect_code 129 test-parse-options --strin 123\n+\ttest_expect_code 1 test-parse-options --strin 123\n '\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n@@ -291,6 +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+\t>expect.err &&\n \ttest_i18ncmp expect.err output.err\n '\n \ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex ef2887bd8..e6a0766f8 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -921,7 +921,7 @@ test_expect_success 'rebase -i --exec without <CMD>' '\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+\t>expected &&\n \ttest_cmp expected actual &&\n \tgit checkout master\n '\ndiff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\nindex b1602718f..157cbcdb2 100755\n--- a/t/t3502-cherry-pick-merge.sh\n+++ b/t/t3502-cherry-pick-merge.sh\n@@ -34,10 +34,10 @@ test_expect_success setup '\n test_expect_success 'cherry-pick -m complains of bogus numbers' '\n \t# expect 129 here to distinguish between cases where\n \t# there was nothing to cherry-pick\n-\ttest_expect_code 129 git cherry-pick -m &&\n-\ttest_expect_code 129 git cherry-pick -m foo b &&\n-\ttest_expect_code 129 git cherry-pick -m -1 b &&\n-\ttest_expect_code 129 git cherry-pick -m 0 b\n+\ttest_expect_code 1 git cherry-pick -m &&\n+\ttest_expect_code 1 git cherry-pick -m foo b &&\n+\ttest_expect_code 1 git cherry-pick -m -1 b &&\n+\ttest_expect_code 1 git cherry-pick -m 0 b\n '\n \n test_expect_success 'cherry-pick a non-merge with -m should fail' '\ndiff --git a/t/tcontains.sh b/t/tcontains.sh\nnew file mode 100755\nindex 000000000..4856111ff\n--- /dev/null\n+++ b/t/tcontains.sh\n@@ -0,0 +1,92 @@\n+#!/bin/sh\n+\n+test_description='Test \"contains\" argument behavior'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup ' '\n+\tgit init . &&\n+\techo \"this is a test\" >file &&\n+\tgit add -A &&\n+\tgit commit -am \"tag test\" &&\n+\tgit tag \"v1.0\" &&\n+\tgit tag \"v1.1\"\n+'\n+\n+test_expect_success 'tag --contains <existent_tag>' '\n+\tgit tag --contains \"v1.0\" >actual &&\n+\tgrep \"v1.0\" actual &&\n+\tgrep \"v1.1\" actual\n+'\n+\n+test_expect_success 'tag --contains <inexistent_tag>' '\n+\ttest_must_fail git tag --contains \"notag\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'tag --no-contains <existent_tag>' '\n+\tgit tag --no-contains \"v1.1\" >actual &&\n+\ttest_line_count = 0 actual\n+'\n+\n+test_expect_success 'tag --no-contains <inexistent_tag>' '\n+\ttest_must_fail git tag --no-contains \"notag\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'tag usage error' '\n+\ttest_must_fail git tag --noopt 2>actual &&\n+\ttest_i18ngrep \"usage\" actual\n+'\n+\n+test_expect_success 'branch --contains <existent_commit>' '\n+\tgit branch --contains \"master\" >actual &&\n+\ttest_i18ngrep \"master\" actual\n+'\n+\n+test_expect_success 'branch --contains <inexistent_commit>' '\n+\ttest_must_fail git branch --no-contains \"nocommit\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'branch --no-contains <existent_commit>' '\n+\tgit branch --no-contains \"master\" >actual &&\n+\ttest_line_count = 0 actual\n+'\n+\n+test_expect_success 'branch --no-contains <inexistent_commit>' '\n+\ttest_must_fail git branch --no-contains \"nocommit\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'branch usage error' '\n+\ttest_must_fail git branch --noopt 2>actual &&\n+\ttest_i18ngrep \"usage\" actual\n+'\n+\n+test_expect_success 'for-each-ref --contains <existent_object>' '\n+\tgit for-each-ref --contains \"master\" >actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'for-each-ref --contains <inexistent_object>' '\n+\ttest_must_fail git for-each-ref --no-contains \"noobject\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'for-each-ref --no-contains <existent_object>' '\n+\tgit for-each-ref --no-contains \"master\" >actual &&\n+\ttest_line_count = 0 actual\n+'\n+\n+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '\n+\ttest_must_fail git for-each-ref --no-contains \"noobject\" 2>actual &&\n+\ttest_i18ngrep \"error\" actual\n+'\n+\n+test_expect_success 'for-each-ref usage error' '\n+\ttest_must_fail git for-each-ref --noopt 2>actual &&\n+\ttest_i18ngrep \"usage\" actual\n+'\n+\n+test_done\n-- \n2.16.2.346.g22874a30c\n\n"},{"id":"341085","messageId":"xmqqzi3m6ooi.fsf@gitster-ct.c.googlers.com","threadId":"47971","inReplyTo":"20180303210938.32474-1-ungureanupaulsebastian@gmail.com","subject":"Re: [GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-06T00:19:25Z","receivedAt":"2018-03-06T00:19: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> 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\nI am guessing that this was sent as a replacement for fcfba373\n(\"ref-filter: make \"--contains <id>\" less chatty if <id> is\ninvalid\", 2018-02-23) that was merged to 'next' at 9623d681 (\"Merge\nbranch 'ps/contains-id-error-message' into next\", 2018-02-27).\n\nIn general, we do not drop and replace what is already merged to\n'next' with a new version; once a topic is merged to 'next', we go\nincremental and further refinements are made on top instead.\n\nI however strongly suspect that the approach taken by this round is\na lot better, and it is sufficiently different that an \"incremental\"\nthat applies on top of the previous patches would essentially revert\nthem and builds what we see in this message afresh.\n\nSo I am tempted to revert the previous one out of 'next' and then\ntreat this one as if it were a new/different topic.\n\n\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 58d1c2d28..eeee1c170 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -1060,6 +1060,8 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n>  \t\tswitch (parseopt_state) {\n>  \t\tcase PARSE_OPT_HELP:\n>  \t\t\texit(129);\n> +\t\tcase PARSE_OPT_ERROR:\n> +\t\t\texit(1);\n\nOK, so things like\n\n\t$ git update-index --index-version\n\t$ git update-index --index-version NOT_AN_INTEGER\n\nused to give the full usage (just like your primary and original\nfocus that was what \"tag --contains\" etc. did), but with this\nchange, they just throw an error and stop.  I guess this is a very\ngood thing ;-)\n\nAlso the exit status is changed from 129 to 1.  It is not clear if\nthat is a desirable change (I am not yet saying we shouldn't change\nit, though).  Calling scripts probably only care about non-zero ness\nof the exit status, so this change probably does not hurt people in\npractice, I guess.\n\n> @@ -504,7 +503,7 @@ 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\nAn error return -1 from parse_long_opt() unfortunately includes the\ncase where the given string is ambiguous.  The test-parse-options\ncommand in t/helpers e.g. has --string and --string2 option, that\ntake a string argument, so \"test-parse-options --stri\" says \"error:\nAmbigous option: stri (could be --string or --string2)\", and without\nthis patch, it goes on to show the usage help.  With the patch,\nhowever, we no longer get the help, and I think that is a regression;\nthe user likely wants to see the help text that describes these two\npotential options to decide between the two.\n\nOf course, \"test-parse-options --string\" fails with \"error: options\n`string' requires a value\" and stops without the usage help---and\nthat is definitely an improvement.\n\nTaking these together, I _think_ this patch is moving things in the\nright direction, in that it allows callers of parse_options_step()\nto tell \"user knew what option s/he wanted, and sufficient error\nmessage has already be given\" and \"user gave us a nonsense option\nand would be helped by usage text\" cases apart by introducing a new\nvalue PARSE_OPT_ERROR, but in order to be able to correctly give\nPARSE_OPT_ERROR back to the caller, parse_long_opt() and\nparse_short_opt() (possibly, but I didn't check) would need a bit of\ntweak to help their callers in this function.\n\n>  test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n> @@ -291,6 +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> +\t>expect.err &&\n>  \ttest_i18ncmp expect.err output.err\n>  '\n\nThe way the existing script sets test vectors up is not that great,\nbut this \"expect.err\" file is created by getting the usage output at\nthe very beginning of the test (into \"expect\") and a few tests refer\nto it, expecting it to have the usage help (see check_unknown_i18n\nhelper).  We should treat its contents as a constant, and shouldn't\ntouch it like this.  Instead, perhaps do\n\n\ttest_must_fail ... 2>actual.err &&\n\ttest_must_be_empty actual.err\n\nhere.\n\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index ef2887bd8..e6a0766f8 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -921,7 +921,7 @@ test_expect_success 'rebase -i --exec without <CMD>' '\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> +\t>expected &&\n>  \ttest_cmp expected actual &&\n>  \tgit checkout master\n>  '\n\nThis used to test\n\n - \"git rebase -i --exec\" fails (due to lack of <cmd>).\n\n - its error output is of no interest--expected to be some \"error\"\n   message that talks about the lack of <cmd>.\n\n - after that first line of the error message, the remainder must be\n   the usage help.\n\nWith the change, the third point is no longer the case.  I wonder if\nwe want to change the second point above to more actively test that\nwe get the error about the lack of <cmd>, e.g. something like\n\n\t...\n\tset_fake_editor &&\n\ttest_must_fail git rebase -i --exec 2>actual &&\n\ttest_i18ngrep \"requires a value\" actual\n\t...\n\n> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh\n> index b1602718f..157cbcdb2 100755\n> --- a/t/t3502-cherry-pick-merge.sh\n> +++ b/t/t3502-cherry-pick-merge.sh\n> @@ -34,10 +34,10 @@ test_expect_success setup '\n>  test_expect_success 'cherry-pick -m complains of bogus numbers' '\n>  \t# expect 129 here to distinguish between cases where\n>  \t# there was nothing to cherry-pick\n> -\ttest_expect_code 129 git cherry-pick -m &&\n> -\ttest_expect_code 129 git cherry-pick -m foo b &&\n> -\ttest_expect_code 129 git cherry-pick -m -1 b &&\n> -\ttest_expect_code 129 git cherry-pick -m 0 b\n> +\ttest_expect_code 1 git cherry-pick -m &&\n> +\ttest_expect_code 1 git cherry-pick -m foo b &&\n> +\ttest_expect_code 1 git cherry-pick -m -1 b &&\n> +\ttest_expect_code 1 git cherry-pick -m 0 b\n\nThe comment in the pre-context is something to ponder on.  Perhaps\nwe would want to find a way to do this change without affecting the\nexit status code.\n"},{"id":"341147","messageId":"1520365472.27478.7.camel@gmail.com","threadId":"47971","inReplyTo":"xmqqzi3m6ooi.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v3] Make options that expect object ids less chatty if id is invalid","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-03-06T19:44:32Z","receivedAt":"2018-03-06T19:45:46Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"Hello Junio,\n\nThank you for reviewing my code. I appreciate it. I made the changes\nhere [1].\n\nOn Mon, 2018-03-05 at 16:19 -0800, Junio C Hamano wrote:\n> Taking these together, I _think_ this patch is moving things in the\n> right direction, in that it allows callers of parse_options_step()\n> to tell \"user knew what option s/he wanted, and sufficient error\n> message has already be given\" and \"user gave us a nonsense option\n> and would be helped by usage text\" cases apart by introducing a new\n> value PARSE_OPT_ERROR, but in order to be able to correctly give\n> PARSE_OPT_ERROR back to the caller, parse_long_opt() and\n> parse_short_opt() (possibly, but I didn't check) would need a bit of\n> tweak to help their callers in this function.\n\nI am not sure I got this right, but I believe this is already done.\n\n- If an error occurs during value parsing in parse_short_opt or\nparse_long_opt then -1 is returned and parse_option_step returns\nPARSE_OPT_ERROR.\n\n- If parse_short_opt and parse_long_opt encounter an unknown option\nthen -2 is returned and parse_option_step returns PARSE_OPT_UNKNOWN\n(but only if PARSE_OPT_KEEP_UNKNOWN is not specified).\n\n- If usage is shown by calling usage_with_options_internal then\nPARSE_OPT_HELP is going to be forwarded and also returned by\nparse_options_step.\n\nWhat I also changed in the new patch [1] is to make parse_long_opt\nreturn -3 when ambiguous option are found, in which case\nparse_options_step will handle this return value by showing usage and\nreturning PARSE_OPT_HELP.\n\nPlease correct me if I am wrong.\n\n[1] https://public-inbox.org/git/20180306193116.23876-1-ungureanupaulse\nbastian@gmail.com/T/#u\n\nBest regards,\nPaul Ungureanu\n\n"}]}