Skip to content

Commit 0025dde

Browse files
rscharfegitster
rscharfe
authored and
gitster
committed
parse-options: make CMDMODE errors more precise
Only a single PARSE_OPT_CMDMODE option can be specified for the same variable at the same time. This is enforced by get_value(), but the error messages are imprecise in three ways: 1. If a non-PARSE_OPT_CMDMODE option changes the value variable of a PARSE_OPT_CMDMODE option then an ominously vague message is shown: $ t/helper/test-tool parse-options --set23 --mode1 error: option `mode1' : incompatible with something else Worse: If the order of options is reversed then no error is reported at all: $ t/helper/test-tool parse-options --mode1 --set23 boolean: 0 integer: 23 magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 verbose: -1 quiet: 0 dry run: no file: (not set) Fortunately this can currently only happen in the test helper; actual Git commands don't share the same variable for the value of options with and without the flag PARSE_OPT_CMDMODE. 2. If there are multiple options with the same value (synonyms), then the one that is defined first is shown rather than the one actually given on the command line, which is confusing: $ git am --resolved --quit error: option `quit' is incompatible with --continue 3. Arguments of PARSE_OPT_CMDMODE options are not handled by the parse-option machinery. This is left to the callback function. We currently only have a single affected option, --show-current-patch of git am. Errors for it can show an argument that was not actually given on the command line: $ git am --show-current-patch --show-current-patch=diff error: options '--show-current-patch=diff' and '--show-current-patch=raw' cannot be used together The options --show-current-patch and --show-current-patch=raw are synonyms, but the error accuses the user of input they did not actually made. Or it can awkwardly print a NULL pointer: $ git am --show-current-patch=diff --show-current-patch error: options '--show-current-patch=(null)' and '--show-current-patch=diff' cannot be used together The reasons for these shortcomings is that the current code checks incompatibility only when encountering a PARSE_OPT_CMDMODE option at the command line, and that it searches the previous incompatible option by value. Fix the first two points by checking all PARSE_OPT_CMDMODE variables after parsing each option and by storing all relevant details if their value changed. Do that whether or not the changing options has the flag PARSE_OPT_CMDMODE set. Report an incompatibility only if two options change the variable to different values and at least one of them is a PARSE_OPT_CMDMODE option. This changes the output of the first three examples above to: $ t/helper/test-tool parse-options --set23 --mode1 error: --mode1 is incompatible with --set23 $ t/helper/test-tool parse-options --mode1 --set23 error: --set23 is incompatible with --mode1 $ git am --resolved --quit error: --quit is incompatible with --resolved Store the argument of PARSE_OPT_CMDMODE options of type OPTION_CALLBACK as well to allow taking over the responsibility for compatibility checking from the callback function. The next patch will use this capability to fix the messages for git am --show-current-patch. Use a linked list for storing the PARSE_OPT_CMDMODE variables. This somewhat outdated data structure is simple and suffices, as the number of elements per command is currently only zero or one. We do support multiple different command modes variables per command, but I don't expect that we'd ever use a significant number of them. Once we do we can switch to a hashmap. Since we no longer need to search the conflicting option, the all_opts parameter of get_value() is no longer used. Remove it. Extend the tests to check for both conflicting option names, but don't insist on a particular order. Signed-off-by: René Scharfe <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 2e8e77c commit 0025dde

File tree

4 files changed

+139
-57
lines changed

4 files changed

+139
-57
lines changed

‎parse-options.c

Lines changed: 92 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -70,42 +70,10 @@ static void fix_filename(const char *prefix, char **file)
7070
*file = prefix_filename_except_for_dash(prefix, *file);
7171
}
7272

73-
static enum parse_opt_result opt_command_mode_error(
74-
const struct option *opt,
75-
const struct option *all_opts,
76-
enum opt_parsed flags)
77-
{
78-
const struct option *that;
79-
struct strbuf that_name = STRBUF_INIT;
80-
81-
/*
82-
* Find the other option that was used to set the variable
83-
* already, and report that this is not compatible with it.
84-
*/
85-
for (that = all_opts; that->type != OPTION_END; that++) {
86-
if (that == opt ||
87-
!(that->flags & PARSE_OPT_CMDMODE) ||
88-
that->value != opt->value ||
89-
that->defval != *(int *)opt->value)
90-
continue;
91-
92-
if (that->long_name)
93-
strbuf_addf(&that_name, "--%s", that->long_name);
94-
else
95-
strbuf_addf(&that_name, "-%c", that->short_name);
96-
error(_("%s is incompatible with %s"),
97-
optname(opt, flags), that_name.buf);
98-
strbuf_release(&that_name);
99-
return PARSE_OPT_ERROR;
100-
}
101-
return error(_("%s : incompatible with something else"),
102-
optname(opt, flags));
103-
}
104-
105-
static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
106-
const struct option *opt,
107-
const struct option *all_opts,
108-
enum opt_parsed flags)
73+
static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
74+
const struct option *opt,
75+
enum opt_parsed flags,
76+
const char **argp)
10977
{
11078
const char *s, *arg;
11179
const int unset = flags & OPT_UNSET;
@@ -118,14 +86,6 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
11886
if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
11987
return error(_("%s takes no value"), optname(opt, flags));
12088

121-
/*
122-
* Giving the same mode option twice, although unnecessary,
123-
* is not a grave error, so let it pass.
124-
*/
125-
if ((opt->flags & PARSE_OPT_CMDMODE) &&
126-
*(int *)opt->value && *(int *)opt->value != opt->defval)
127-
return opt_command_mode_error(opt, all_opts, flags);
128-
12989
switch (opt->type) {
13090
case OPTION_LOWLEVEL_CALLBACK:
13191
return opt->ll_callback(p, opt, NULL, unset);
@@ -200,6 +160,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
200160
p_unset = 0;
201161
p_arg = arg;
202162
}
163+
if (opt->flags & PARSE_OPT_CMDMODE)
164+
*argp = p_arg;
203165
if (opt->callback)
204166
return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0;
205167
else
@@ -247,16 +209,91 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
247209
}
248210
}
249211

212+
struct parse_opt_cmdmode_list {
213+
int value, *value_ptr;
214+
const struct option *opt;
215+
const char *arg;
216+
enum opt_parsed flags;
217+
struct parse_opt_cmdmode_list *next;
218+
};
219+
220+
static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
221+
const struct option *opts)
222+
{
223+
ctx->cmdmode_list = NULL;
224+
225+
for (; opts->type != OPTION_END; opts++) {
226+
struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
227+
int *value_ptr = opts->value;
228+
229+
if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
230+
continue;
231+
232+
while (elem && elem->value_ptr != value_ptr)
233+
elem = elem->next;
234+
if (elem)
235+
continue;
236+
237+
CALLOC_ARRAY(elem, 1);
238+
elem->value_ptr = value_ptr;
239+
elem->value = *value_ptr;
240+
elem->next = ctx->cmdmode_list;
241+
ctx->cmdmode_list = elem;
242+
}
243+
}
244+
245+
static char *optnamearg(const struct option *opt, const char *arg,
246+
enum opt_parsed flags)
247+
{
248+
if (flags & OPT_SHORT)
249+
return xstrfmt("-%c%s", opt->short_name, arg ? arg : "");
250+
return xstrfmt("--%s%s%s%s", flags & OPT_UNSET ? "no-" : "",
251+
opt->long_name, arg ? "=" : "", arg ? arg : "");
252+
}
253+
254+
static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
255+
const struct option *opt,
256+
enum opt_parsed flags)
257+
{
258+
const char *arg = NULL;
259+
enum parse_opt_result result = do_get_value(p, opt, flags, &arg);
260+
struct parse_opt_cmdmode_list *elem = p->cmdmode_list;
261+
char *opt_name, *other_opt_name;
262+
263+
for (; elem; elem = elem->next) {
264+
if (*elem->value_ptr == elem->value)
265+
continue;
266+
267+
if (elem->opt &&
268+
(elem->opt->flags | opt->flags) & PARSE_OPT_CMDMODE)
269+
break;
270+
271+
elem->opt = opt;
272+
elem->arg = arg;
273+
elem->flags = flags;
274+
elem->value = *elem->value_ptr;
275+
}
276+
277+
if (result || !elem)
278+
return result;
279+
280+
opt_name = optnamearg(opt, arg, flags);
281+
other_opt_name = optnamearg(elem->opt, elem->arg, elem->flags);
282+
error(_("%s is incompatible with %s"), opt_name, other_opt_name);
283+
free(opt_name);
284+
free(other_opt_name);
285+
return -1;
286+
}
287+
250288
static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
251289
const struct option *options)
252290
{
253-
const struct option *all_opts = options;
254291
const struct option *numopt = NULL;
255292

256293
for (; options->type != OPTION_END; options++) {
257294
if (options->short_name == *p->opt) {
258295
p->opt = p->opt[1] ? p->opt + 1 : NULL;
259-
return get_value(p, options, all_opts, OPT_SHORT);
296+
return get_value(p, options, OPT_SHORT);
260297
}
261298

262299
/*
@@ -318,7 +355,6 @@ static enum parse_opt_result parse_long_opt(
318355
struct parse_opt_ctx_t *p, const char *arg,
319356
const struct option *options)
320357
{
321-
const struct option *all_opts = options;
322358
const char *arg_end = strchrnul(arg, '=');
323359
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
324360
enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
@@ -387,7 +423,7 @@ static enum parse_opt_result parse_long_opt(
387423
continue;
388424
p->opt = rest + 1;
389425
}
390-
return get_value(p, options, all_opts, flags ^ opt_flags);
426+
return get_value(p, options, flags ^ opt_flags);
391427
}
392428

393429
if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
@@ -405,21 +441,19 @@ static enum parse_opt_result parse_long_opt(
405441
return PARSE_OPT_HELP;
406442
}
407443
if (abbrev_option)
408-
return get_value(p, abbrev_option, all_opts, abbrev_flags);
444+
return get_value(p, abbrev_option, abbrev_flags);
409445
return PARSE_OPT_UNKNOWN;
410446
}
411447

412448
static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p,
413449
const char *arg,
414450
const struct option *options)
415451
{
416-
const struct option *all_opts = options;
417-
418452
for (; options->type != OPTION_END; options++) {
419453
if (!(options->flags & PARSE_OPT_NODASH))
420454
continue;
421455
if (options->short_name == arg[0] && arg[1] == '\0')
422-
return get_value(p, options, all_opts, OPT_SHORT);
456+
return get_value(p, options, OPT_SHORT);
423457
}
424458
return PARSE_OPT_ERROR;
425459
}
@@ -574,6 +608,7 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
574608
(flags & PARSE_OPT_KEEP_ARGV0))
575609
BUG("Can't keep argv0 if you don't have it");
576610
parse_options_check(options);
611+
build_cmdmode_list(ctx, options);
577612
}
578613

579614
void parse_options_start(struct parse_opt_ctx_t *ctx,
@@ -1006,6 +1041,11 @@ int parse_options(int argc, const char **argv,
10061041
precompose_argv_prefix(argc, argv, NULL);
10071042
free_preprocessed_options(real_options);
10081043
free(ctx.alias_groups);
1044+
for (struct parse_opt_cmdmode_list *elem = ctx.cmdmode_list; elem;) {
1045+
struct parse_opt_cmdmode_list *next = elem->next;
1046+
free(elem);
1047+
elem = next;
1048+
}
10091049
return parse_options_end(&ctx);
10101050
}
10111051

‎parse-options.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,8 @@ static inline void die_for_incompatible_opt3(int opt1, const char *opt1_name,
445445

446446
/*----- incremental advanced APIs -----*/
447447

448+
struct parse_opt_cmdmode_list;
449+
448450
/*
449451
* It's okay for the caller to consume argv/argc in the usual way.
450452
* Other fields of that structure are private to parse-options and should not
@@ -459,6 +461,7 @@ struct parse_opt_ctx_t {
459461
unsigned has_subcommands;
460462
const char *prefix;
461463
const char **alias_groups; /* must be in groups of 3 elements! */
464+
struct parse_opt_cmdmode_list *cmdmode_list;
462465
};
463466

464467
void parse_options_start(struct parse_opt_ctx_t *ctx,

‎t/helper/test-parse-options.c

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,19 @@ static struct {
2121
int unset;
2222
} length_cb;
2323

24+
static int mode34_callback(const struct option *opt, const char *arg, int unset)
25+
{
26+
if (unset)
27+
*(int *)opt->value = 0;
28+
else if (!strcmp(arg, "3"))
29+
*(int *)opt->value = 3;
30+
else if (!strcmp(arg, "4"))
31+
*(int *)opt->value = 4;
32+
else
33+
return error("invalid value for '%s': '%s'", "--mode34", arg);
34+
return 0;
35+
}
36+
2437
static int length_callback(const struct option *opt, const char *arg, int unset)
2538
{
2639
length_cb.called = 1;
@@ -124,6 +137,9 @@ int cmd__parse_options(int argc, const char **argv)
124137
OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
125138
OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
126139
OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
140+
OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)",
141+
"set integer to 3 or 4 (cmdmode option)",
142+
PARSE_OPT_CMDMODE, mode34_callback),
127143
OPT_CALLBACK('L', "length", &integer, "str",
128144
"get length of <str>", length_callback),
129145
OPT_FILENAME('F', "file", &file, "set file to <file>"),

‎t/t0040-parse-options.sh

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ usage: test-tool parse-options <options>
2828
--[no-]set23 set integer to 23
2929
--mode1 set integer to 1 (cmdmode option)
3030
--mode2 set integer to 2 (cmdmode option)
31+
--[no-]mode34 (3|4) set integer to 3 or 4 (cmdmode option)
3132
-L, --[no-]length <str>
3233
get length of <str>
3334
-F, --[no-]file <file>
@@ -366,19 +367,41 @@ test_expect_success 'OPT_NEGBIT() works' '
366367
'
367368

368369
test_expect_success 'OPT_CMDMODE() works' '
369-
test-tool parse-options --expect="integer: 1" --mode1
370+
test-tool parse-options --expect="integer: 1" --mode1 &&
371+
test-tool parse-options --expect="integer: 3" --mode34=3
370372
'
371373

372-
test_expect_success 'OPT_CMDMODE() detects incompatibility' '
374+
test_expect_success 'OPT_CMDMODE() detects incompatibility (1)' '
373375
test_must_fail test-tool parse-options --mode1 --mode2 >output 2>output.err &&
374376
test_must_be_empty output &&
375-
test_i18ngrep "incompatible with --mode" output.err
377+
test_i18ngrep "mode1" output.err &&
378+
test_i18ngrep "mode2" output.err &&
379+
test_i18ngrep "is incompatible with" output.err
376380
'
377381

378-
test_expect_success 'OPT_CMDMODE() detects incompatibility with something else' '
382+
test_expect_success 'OPT_CMDMODE() detects incompatibility (2)' '
379383
test_must_fail test-tool parse-options --set23 --mode2 >output 2>output.err &&
380384
test_must_be_empty output &&
381-
test_i18ngrep "incompatible with something else" output.err
385+
test_i18ngrep "mode2" output.err &&
386+
test_i18ngrep "set23" output.err &&
387+
test_i18ngrep "is incompatible with" output.err
388+
'
389+
390+
test_expect_success 'OPT_CMDMODE() detects incompatibility (3)' '
391+
test_must_fail test-tool parse-options --mode2 --set23 >output 2>output.err &&
392+
test_must_be_empty output &&
393+
test_i18ngrep "mode2" output.err &&
394+
test_i18ngrep "set23" output.err &&
395+
test_i18ngrep "is incompatible with" output.err
396+
'
397+
398+
test_expect_success 'OPT_CMDMODE() detects incompatibility (4)' '
399+
test_must_fail test-tool parse-options --mode2 --mode34=3 \
400+
>output 2>output.err &&
401+
test_must_be_empty output &&
402+
test_i18ngrep "mode2" output.err &&
403+
test_i18ngrep "mode34.3" output.err &&
404+
test_i18ngrep "is incompatible with" output.err
382405
'
383406

384407
test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' '

0 commit comments

Comments
 (0)