Skip to content

Commit 3bb0923

Browse files
ungpsgitster
ungps
authored and
gitster
committed
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 38e79b1 commit 3bb0923

File tree

8 files changed

+125
-14
lines changed

8 files changed

+125
-14
lines changed

‎builtin/blame.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -720,6 +720,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
720720
for (;;) {
721721
switch (parse_options_step(&ctx, options, blame_opt_usage)) {
722722
case PARSE_OPT_HELP:
723+
case PARSE_OPT_ERROR:
723724
exit(129);
724725
case PARSE_OPT_DONE:
725726
if (ctx.argv[0])

‎builtin/shortlog.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
283283
for (;;) {
284284
switch (parse_options_step(&ctx, options, shortlog_usage)) {
285285
case PARSE_OPT_HELP:
286+
case PARSE_OPT_ERROR:
286287
exit(129);
287288
case PARSE_OPT_DONE:
288289
goto parse_done;

‎builtin/update-index.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
10591059
break;
10601060
switch (parseopt_state) {
10611061
case PARSE_OPT_HELP:
1062+
case PARSE_OPT_ERROR:
10621063
exit(129);
10631064
case PARSE_OPT_NON_OPTION:
10641065
case PARSE_OPT_DONE:

‎parse-options.c

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -317,14 +317,16 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
317317
return get_value(p, options, all_opts, flags ^ opt_flags);
318318
}
319319

320-
if (ambiguous_option)
321-
return error("Ambiguous option: %s "
320+
if (ambiguous_option) {
321+
error("Ambiguous option: %s "
322322
"(could be --%s%s or --%s%s)",
323323
arg,
324324
(ambiguous_flags & OPT_UNSET) ? "no-" : "",
325325
ambiguous_option->long_name,
326326
(abbrev_flags & OPT_UNSET) ? "no-" : "",
327327
abbrev_option->long_name);
328+
return -3;
329+
}
328330
if (abbrev_option)
329331
return get_value(p, abbrev_option, all_opts, abbrev_flags);
330332
return -2;
@@ -434,7 +436,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
434436
const char * const usagestr[])
435437
{
436438
int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
437-
int err = 0;
438439

439440
/* we must reset ->opt, unknown short option leave it dangling */
440441
ctx->opt = NULL;
@@ -459,7 +460,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
459460
ctx->opt = arg + 1;
460461
switch (parse_short_opt(ctx, options)) {
461462
case -1:
462-
goto show_usage_error;
463+
return PARSE_OPT_ERROR;
463464
case -2:
464465
if (ctx->opt)
465466
check_typos(arg + 1, options);
@@ -472,7 +473,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
472473
while (ctx->opt) {
473474
switch (parse_short_opt(ctx, options)) {
474475
case -1:
475-
goto show_usage_error;
476+
return PARSE_OPT_ERROR;
476477
case -2:
477478
if (internal_help && *ctx->opt == 'h')
478479
goto show_usage;
@@ -504,9 +505,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
504505
goto show_usage;
505506
switch (parse_long_opt(ctx, arg + 2, options)) {
506507
case -1:
507-
goto show_usage_error;
508+
return PARSE_OPT_ERROR;
508509
case -2:
509510
goto unknown;
511+
case -3:
512+
goto show_usage;
510513
}
511514
continue;
512515
unknown:
@@ -517,10 +520,8 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
517520
}
518521
return PARSE_OPT_DONE;
519522

520-
show_usage_error:
521-
err = 1;
522523
show_usage:
523-
return usage_with_options_internal(ctx, usagestr, options, 0, err);
524+
return usage_with_options_internal(ctx, usagestr, options, 0, 0);
524525
}
525526

526527
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,
539540
parse_options_start(&ctx, argc, argv, prefix, options, flags);
540541
switch (parse_options_step(&ctx, options, usagestr)) {
541542
case PARSE_OPT_HELP:
543+
case PARSE_OPT_ERROR:
542544
exit(129);
543545
case PARSE_OPT_NON_OPTION:
544546
case PARSE_OPT_DONE:

‎parse-options.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ enum {
188188
PARSE_OPT_HELP = -1,
189189
PARSE_OPT_DONE,
190190
PARSE_OPT_NON_OPTION,
191+
PARSE_OPT_ERROR,
191192
PARSE_OPT_UNKNOWN
192193
};
193194

‎t/t0040-parse-options.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
291291
test_expect_success 'OPT_CALLBACK() and callback errors work' '
292292
test_must_fail test-parse-options --no-length >output 2>output.err &&
293293
test_i18ncmp expect output &&
294-
test_i18ncmp expect.err output.err
294+
test_must_be_empty output.err
295295
'
296296

297297
cat >expect <<\EOF

‎t/t0041-usage.sh

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
#!/bin/sh
2+
3+
test_description='Test commands behavior when given invalid argument value'
4+
5+
. ./test-lib.sh
6+
7+
test_expect_success 'setup ' '
8+
test_commit "v1.0"
9+
'
10+
11+
test_expect_success 'tag --contains <existent_tag>' '
12+
git tag --contains "v1.0" >actual 2>actual.err &&
13+
grep "v1.0" actual &&
14+
test_line_count = 0 actual.err
15+
'
16+
17+
test_expect_success 'tag --contains <inexistent_tag>' '
18+
test_must_fail git tag --contains "notag" >actual 2>actual.err &&
19+
test_line_count = 0 actual &&
20+
test_i18ngrep "error" actual.err &&
21+
test_i18ngrep ! "usage" actual.err
22+
'
23+
24+
test_expect_success 'tag --no-contains <existent_tag>' '
25+
git tag --no-contains "v1.0" >actual 2>actual.err &&
26+
test_line_count = 0 actual &&
27+
test_line_count = 0 actual.err
28+
'
29+
30+
test_expect_success 'tag --no-contains <inexistent_tag>' '
31+
test_must_fail git tag --no-contains "notag" >actual 2>actual.err &&
32+
test_line_count = 0 actual &&
33+
test_i18ngrep "error" actual.err &&
34+
test_i18ngrep ! "usage" actual.err
35+
'
36+
37+
test_expect_success 'tag usage error' '
38+
test_must_fail git tag --noopt >actual 2>actual.err &&
39+
test_line_count = 0 actual &&
40+
test_i18ngrep "usage" actual.err
41+
'
42+
43+
test_expect_success 'branch --contains <existent_commit>' '
44+
git branch --contains "master" >actual 2>actual.err &&
45+
test_i18ngrep "master" actual &&
46+
test_line_count = 0 actual.err
47+
'
48+
49+
test_expect_success 'branch --contains <inexistent_commit>' '
50+
test_must_fail git branch --no-contains "nocommit" >actual 2>actual.err &&
51+
test_line_count = 0 actual &&
52+
test_i18ngrep "error" actual.err &&
53+
test_i18ngrep ! "usage" actual.err
54+
'
55+
56+
test_expect_success 'branch --no-contains <existent_commit>' '
57+
git branch --no-contains "master" >actual 2>actual.err &&
58+
test_line_count = 0 actual &&
59+
test_line_count = 0 actual.err
60+
'
61+
62+
test_expect_success 'branch --no-contains <inexistent_commit>' '
63+
test_must_fail git branch --no-contains "nocommit" >actual 2>actual.err &&
64+
test_line_count = 0 actual &&
65+
test_i18ngrep "error" actual.err &&
66+
test_i18ngrep ! "usage" actual.err
67+
'
68+
69+
test_expect_success 'branch usage error' '
70+
test_must_fail git branch --noopt >actual 2>actual.err &&
71+
test_line_count = 0 actual &&
72+
test_i18ngrep "usage" actual.err
73+
'
74+
75+
test_expect_success 'for-each-ref --contains <existent_object>' '
76+
git for-each-ref --contains "master" >actual 2>actual.err &&
77+
test_line_count = 2 actual &&
78+
test_line_count = 0 actual.err
79+
'
80+
81+
test_expect_success 'for-each-ref --contains <inexistent_object>' '
82+
test_must_fail git for-each-ref --no-contains "noobject" >actual 2>actual.err &&
83+
test_line_count = 0 actual &&
84+
test_i18ngrep "error" actual.err &&
85+
test_i18ngrep ! "usage" actual.err
86+
'
87+
88+
test_expect_success 'for-each-ref --no-contains <existent_object>' '
89+
git for-each-ref --no-contains "master" >actual 2>actual.err &&
90+
test_line_count = 0 actual &&
91+
test_line_count = 0 actual.err
92+
'
93+
94+
test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
95+
test_must_fail git for-each-ref --no-contains "noobject" >actual 2>actual.err &&
96+
test_line_count = 0 actual &&
97+
test_i18ngrep "error" actual.err &&
98+
test_i18ngrep ! "usage" actual.err
99+
'
100+
101+
test_expect_success 'for-each-ref usage error' '
102+
test_must_fail git for-each-ref --noopt >actual 2>actual.err &&
103+
test_line_count = 0 actual &&
104+
test_i18ngrep "usage" actual.err
105+
'
106+
107+
test_done

‎t/t3404-rebase-interactive.sh

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -915,10 +915,8 @@ test_expect_success 'rebase --exec works without -i ' '
915915
test_expect_success 'rebase -i --exec without <CMD>' '
916916
git reset --hard execute &&
917917
set_fake_editor &&
918-
test_must_fail git rebase -i --exec 2>tmp &&
919-
sed -e "1d" tmp >actual &&
920-
test_must_fail git rebase -h >expected &&
921-
test_cmp expected actual &&
918+
test_must_fail git rebase -i --exec 2>actual &&
919+
test_i18ngrep "requires a value" actual &&
922920
git checkout master
923921
'
924922

0 commit comments

Comments
 (0)