parse-options: do not show usage upon invalid option value

Usually, the usage should be shown only if the user does not know what
options are available. If the user specifies an invalid value, the user
is already aware of the available options. In this case, there is no
point in displaying the usage anymore.

This patch applies to "git tag --contains", "git branch --contains",
"git branch --points-at", "git for-each-ref --contains" and many more.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/blame.c b/builtin/blame.c
index 005f55aaa257fc34f81517650223a3c195c03178..9803ddc0e7fad0a72891045a1526d7884f395e1a 100644 (file)
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -720,6 +720,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
        for (;;) {
                switch (parse_options_step(&ctx, options, blame_opt_usage)) {
                case PARSE_OPT_HELP:
+               case PARSE_OPT_ERROR:
                        exit(129);
                case PARSE_OPT_DONE:
                        if (ctx.argv[0])

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e29875b84389b25237e39e0112eafa8ac34599ee..be4df6a037fce46dd5af321025a898dd1d5fab87 100644 (file)
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -283,6 +283,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
        for (;;) {
                switch (parse_options_step(&ctx, options, shortlog_usage)) {
                case PARSE_OPT_HELP:
+               case PARSE_OPT_ERROR:
                        exit(129);
                case PARSE_OPT_DONE:
                        goto parse_done;

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 58d1c2d2827d61899d73f1ea7632c5ee219f3ace..34adf55a719915c6e69f8983a37851ca99e9b60b 100644 (file)
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
                        break;
                switch (parseopt_state) {
                case PARSE_OPT_HELP:
+               case PARSE_OPT_ERROR:
                        exit(129);
                case PARSE_OPT_NON_OPTION:
                case PARSE_OPT_DONE:

diff --git a/parse-options.c b/parse-options.c
index fca7159646c82cb9213e72afd94d8debc6bd4c51..a0bae92b0cacc4fe4d1beb58dc5af890ff72fd56 100644 (file)
--- a/parse-options.c
+++ b/parse-options.c
@@ -317,14 +317,16 @@ is_abbreviated:
                return get_value(p, options, all_opts, flags ^ opt_flags);
        }
 
-       if (ambiguous_option)
-               return error("Ambiguous option: %s "
+       if (ambiguous_option) {
+               error("Ambiguous option: %s "
                        "(could be --%s%s or --%s%s)",
                        arg,
                        (ambiguous_flags & OPT_UNSET) ?  "no-" : "",
                        ambiguous_option->long_name,
                        (abbrev_flags & OPT_UNSET) ?  "no-" : "",
                        abbrev_option->long_name);
+               return -3;
+       }
        if (abbrev_option)
                return get_value(p, abbrev_option, all_opts, abbrev_flags);
        return -2;
@@ -434,7 +436,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                       const char * const usagestr[])
 {
        int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
-       int err = 0;
 
        /* we must reset ->opt, unknown short option leave it dangling */
        ctx->opt = NULL;
@@ -459,7 +460,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        ctx->opt = arg + 1;
                        switch (parse_short_opt(ctx, options)) {
                        case -1:
-                               goto show_usage_error;
+                               return PARSE_OPT_ERROR;
                        case -2:
                                if (ctx->opt)
                                        check_typos(arg + 1, options);
@@ -472,7 +473,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        while (ctx->opt) {
                                switch (parse_short_opt(ctx, options)) {
                                case -1:
-                                       goto show_usage_error;
+                                       return PARSE_OPT_ERROR;
                                case -2:
                                        if (internal_help && *ctx->opt == 'h')
                                                goto show_usage;
@@ -504,9 +505,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
                        goto show_usage;
                switch (parse_long_opt(ctx, arg + 2, options)) {
                case -1:
-                       goto show_usage_error;
+                       return PARSE_OPT_ERROR;
                case -2:
                        goto unknown;
+               case -3:
+                       goto show_usage;
                }
                continue;
 unknown:
@@ -517,10 +520,8 @@ unknown:
        }
        return PARSE_OPT_DONE;
 
- show_usage_error:
-       err = 1;
  show_usage:
-       return usage_with_options_internal(ctx, usagestr, options, 0, err);
+       return usage_with_options_internal(ctx, usagestr, options, 0, 0);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -539,6 +540,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
        parse_options_start(&ctx, argc, argv, prefix, options, flags);
        switch (parse_options_step(&ctx, options, usagestr)) {
        case PARSE_OPT_HELP:
+       case PARSE_OPT_ERROR:
                exit(129);
        case PARSE_OPT_NON_OPTION:
        case PARSE_OPT_DONE:

diff --git a/parse-options.h b/parse-options.h
index af711227ae3aac7af07de0322a364ccac03b819d..c77bb3b4f8d9342178b8caa68190b02f50ea4274 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -188,6 +188,7 @@ enum {
        PARSE_OPT_HELP = -1,
        PARSE_OPT_DONE,
        PARSE_OPT_NON_OPTION,
+       PARSE_OPT_ERROR,
        PARSE_OPT_UNKNOWN
 };
 

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 0c2fc81d7b0fa401db58c41cd2fed4469e80b058..04d474c84fd69121c686f5ca5adc40ce081f0e9d 100755 (executable)
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -291,7 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 test_expect_success 'OPT_CALLBACK() and callback errors work' '
        test_must_fail test-parse-options --no-length >output 2>output.err &&
        test_i18ncmp expect output &&
-       test_i18ncmp expect.err output.err
+       test_must_be_empty output.err
 '
 
 cat >expect <<\EOF

diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
new file mode 100755 (executable)
index 0000000..5b927b7
--- /dev/null
+++ b/t/t0041-usage.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='Test commands behavior when given invalid argument value'
+
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+       test_commit "v1.0"
+'
+
+test_expect_success 'tag --contains <existent_tag>' '
+       git tag --contains "v1.0" >actual 2>actual.err &&
+       grep "v1.0" actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'tag --contains <inexistent_tag>' '
+       test_must_fail git tag --contains "notag" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'tag --no-contains <existent_tag>' '
+       git tag --no-contains "v1.0" >actual 2>actual.err  &&
+       test_line_count = 0 actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'tag --no-contains <inexistent_tag>' '
+       test_must_fail git tag --no-contains "notag" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'tag usage error' '
+       test_must_fail git tag --noopt >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "usage" actual.err
+'
+
+test_expect_success 'branch --contains <existent_commit>' '
+       git branch --contains "master" >actual 2>actual.err &&
+       test_i18ngrep "master" actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'branch --contains <inexistent_commit>' '
+       test_must_fail git branch --no-contains "nocommit" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'branch --no-contains <existent_commit>' '
+       git branch --no-contains "master" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'branch --no-contains <inexistent_commit>' '
+       test_must_fail git branch --no-contains "nocommit" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'branch usage error' '
+       test_must_fail git branch --noopt >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "usage" actual.err
+'
+
+test_expect_success 'for-each-ref --contains <existent_object>' '
+       git for-each-ref --contains "master" >actual 2>actual.err &&
+       test_line_count = 2 actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'for-each-ref --contains <inexistent_object>' '
+       test_must_fail git for-each-ref --no-contains "noobject" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'for-each-ref --no-contains <existent_object>' '
+       git for-each-ref --no-contains "master" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_line_count = 0 actual.err
+'
+
+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
+       test_must_fail git for-each-ref --no-contains "noobject" >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "error" actual.err &&
+       test_i18ngrep ! "usage" actual.err
+'
+
+test_expect_success 'for-each-ref usage error' '
+       test_must_fail git for-each-ref --noopt >actual 2>actual.err &&
+       test_line_count = 0 actual &&
+       test_i18ngrep "usage" actual.err
+'
+
+test_done

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 481a3500900d0fccb28762ba24b2ca422956a44f..3216707df77deaf8c6657e945b5bdabea17ac9be 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -915,10 +915,8 @@ test_expect_success 'rebase --exec works without -i ' '
 test_expect_success 'rebase -i --exec without <CMD>' '
        git reset --hard execute &&
        set_fake_editor &&
-       test_must_fail git rebase -i --exec 2>tmp &&
-       sed -e "1d" tmp >actual &&
-       test_must_fail git rebase -h >expected &&
-       test_cmp expected actual &&
+       test_must_fail git rebase -i --exec 2>actual &&
+       test_i18ngrep "requires a value" actual &&
        git checkout master
 '