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>
This commit is contained in:
Paul-Sebastian Ungureanu 2018-03-22 20:43:51 +02:00 committed by Junio C Hamano
parent 38e79b1fda
commit 3bb0923f06
8 changed files with 125 additions and 14 deletions

View file

@ -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])

View file

@ -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;

View file

@ -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:

View file

@ -317,14 +317,16 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
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 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
}
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:

View file

@ -188,6 +188,7 @@ enum {
PARSE_OPT_HELP = -1,
PARSE_OPT_DONE,
PARSE_OPT_NON_OPTION,
PARSE_OPT_ERROR,
PARSE_OPT_UNKNOWN
};

View file

@ -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

107
t/t0041-usage.sh Executable file
View file

@ -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

View file

@ -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
'