| Message ID | 20220725123857.2773963-10-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parse-options: handle subcommands | expand |
On Mon, Jul 25 2022, SZEDER Gábor wrote: > Several Git commands have subcommands to implement mutually exclusive > "operation modes", and they usually parse their subcommand argument > with a bunch of if-else if statements. I'll need do look this over in more details, just some comments on the non-meaty parts for now: > diff --git a/builtin/blame.c b/builtin/blame.c > index 02e39420b6..a9fe8cf7a6 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -920,6 +920,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) > break; > case PARSE_OPT_HELP: > case PARSE_OPT_ERROR: > + case PARSE_OPT_SUBCOMMAND: > exit(129); > case PARSE_OPT_COMPLETE: > exit(0); > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > index 086dfee45a..7a1e1fe7c0 100644 > --- a/builtin/shortlog.c > +++ b/builtin/shortlog.c > @@ -381,6 +381,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) > break; > case PARSE_OPT_HELP: > case PARSE_OPT_ERROR: > + case PARSE_OPT_SUBCOMMAND: > exit(129); > case PARSE_OPT_COMPLETE: > exit(0); This feels a bit like carrying forward an API wart, i.e. shouldn't we instead BUG() if we are returning a PARSE_OPT_SUBCOMMAND from parse_options_step() for options lists that don't have PARSE_OPT_SUBCOMMAND in them? I.e. is this even reachable, or just something to suppress the compiler complaining about missing enum labels? > static void check_typos(const char *arg, const struct option *options) > { > if (strlen(arg) < 3) > @@ -442,6 +457,7 @@ static void check_typos(const char *arg, const struct option *options) > static void parse_options_check(const struct option *opts) > { > char short_opts[128]; > + void *subcommand_value = NULL; > > memset(short_opts, '\0', sizeof(short_opts)); > for (; opts->type != OPTION_END; opts++) { > @@ -489,6 +505,14 @@ static void parse_options_check(const struct option *opts) > "Are you using parse_options_step() directly?\n" > "That case is not supported yet."); > break; > + case OPTION_SUBCOMMAND: > + if (!opts->value || !opts->subcommand_fn) > + optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function"); > + if (!subcommand_value) > + subcommand_value = opts->value; > + else if (subcommand_value != opts->value) > + optbug(opts, "all OPTION_SUBCOMMANDs need the same value"); > + break; > default: > ; /* ok. (usually accepts an argument) */ > } This addition looks good... > @@ -499,6 +523,14 @@ static void parse_options_check(const struct option *opts) > BUG_if_bug("invalid 'struct option'"); > } > > +static int has_subcommands(const struct option *options) > +{ > + for (; options->type != OPTION_END; options++) > + if (options->type == OPTION_SUBCOMMAND) > + return 1; > + return 0; > +} ...but why not... > static void parse_options_start_1(struct parse_opt_ctx_t *ctx, > int argc, const char **argv, const char *prefix, > const struct option *options, > @@ -515,6 +547,19 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, > ctx->prefix = prefix; > ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); > ctx->flags = flags; > + ctx->has_subcommands = has_subcommands(options); > + if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) > + BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands"); > + if (ctx->has_subcommands) { > + if (flags & PARSE_OPT_STOP_AT_NON_OPTION) > + BUG("subcommands are incompatible with PARSE_OPT_STOP_AT_NON_OPTION"); > + if (!(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) { > + if (flags & PARSE_OPT_KEEP_UNKNOWN_OPT) > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_UNKNOWN unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); > + if (flags & PARSE_OPT_KEEP_DASHDASH) > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_DASHDASH unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); > + } > + } ...move this into parse_options_check()? I.e. we'd need to loop over the list once, but it seems like this should belong there. We have an existing bug in-tree due to usage_with_options() not doing a parse_options_check() (I have a local fix...), checking this sort of thing there instead of in parse_options_start() is therefore the right thing to do, i.e. we shoudl have a one-stop "does this options variable look sane?". > + error(_("unknown subcommand: %s"), arg); s/%s/'%s'/ while we're at it, perhaps? > + usage_with_options(usagestr, options); > + case PARSE_OPT_COMPLETE: > + case PARSE_OPT_HELP: > + case PARSE_OPT_ERROR: > + case PARSE_OPT_DONE: > + case PARSE_OPT_NON_OPTION: > + BUG("parse_subcommand() cannot return these"); nit: BUG("got bad %d", v) or whatever, i.e. say what we got? > @@ -206,6 +217,11 @@ struct option { > #define OPT_ALIAS(s, l, source_long_name) \ > { OPTION_ALIAS, (s), (l), (source_long_name) } > > +#define OPT_SUBCOMMAND(l, v, fn) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > + NULL, 0, NULL, 0, NULL, 0, (fn) } > +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > + NULL, (f), NULL, 0, NULL, 0, (fn) } Nit, I know you're carrying forward existing patterns, but since that all pre-dated designated init perhaps we could just (untested): #define OPT_SUBCOMMAND_F(l, v, fn, f) { \ .type = OPTION_SUBCOMMAND, \ .long_name = (l), \ .value = (v), \ .ll_callback = (fn), \ } #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) Which IMO is much nicer. I have some patches somewhere to convert these to saner patterns (I think not designated init, but the X() can be defined in terms of X_F() like that, but since this is new we can use designated init all the way... > +{ > + int i; Nit: missing \n (usual style of variable decl); > + error("'cmd' is mandatory"); > + usage_with_options(usage, test_flag_options); nit: I think you want usage_msg_optf() or usage_msg_opt(). > +test_expect_success 'subcommand - no subcommand shows error and usage' ' > + test_expect_code 129 test-tool parse-subcommand cmd 2>err && > + grep "^error: need a subcommand" err && > + grep ^usage: err > +' > + > +test_expect_success 'subcommand - subcommand after -- shows error and usage' ' > + test_expect_code 129 test-tool parse-subcommand cmd -- subcmd-one 2>err && > + grep "^error: need a subcommand" err && > + grep ^usage: err > +' > + > +test_expect_success 'subcommand - subcommand after --end-of-options shows error and usage' ' > + test_expect_code 129 test-tool parse-subcommand cmd --end-of-options subcmd-one 2>err && > + grep "^error: need a subcommand" err && > + grep ^usage: err > +' > + > +test_expect_success 'subcommand - unknown subcommand shows error and usage' ' > + test_expect_code 129 test-tool parse-subcommand cmd nope 2>err && > + grep "^error: unknown subcommand: nope" err && > + grep ^usage: err > +' > + > +test_expect_success 'subcommand - subcommands cannot be abbreviated' ' > + test_expect_code 129 test-tool parse-subcommand cmd subcmd-o 2>err && > + grep "^error: unknown subcommand: subcmd-o$" err && > + grep ^usage: err > +' > + > +test_expect_success 'subcommand - no negated subcommands' ' > + test_expect_code 129 test-tool parse-subcommand cmd no-subcmd-one 2>err && > + grep "^error: unknown subcommand: no-subcmd-one" err && > + grep ^usage: err > +' Creating a trivial helper for this seems worthile, then something like: that_helper "expected error here" -- arg u ments to test-tool parse-subcommand > +test_expect_success 'subcommand - simple' ' > + test-tool parse-subcommand cmd subcmd-two >actual && > + cat >expect <<-\EOF && > + opt: 0 > + fn: subcmd_two > + arg 00: subcmd-two > + EOF > + test_cmp expect actual > +' Ditto, perhaps? I.e. new_hepler arg u ments <<-\EOF expected goes here EOF
SZEDER Gábor <szeder.dev@gmail.com> writes: > The approach is guided by the following observations: > ... > So in the end subcommand declaration and parsing would look something > like this: > > parse_opt_subcommand_fn *fn = NULL; > struct option builtin_commit_graph_options[] = { > OPT_STRING(0, "object-dir", &opts.obj_dir, N_("dir"), > N_("the object directory to store the graph")), > OPT_SUBCOMMAND("verify", &fn, graph_verify), > OPT_SUBCOMMAND("write", &fn, graph_write), > OPT_END(), > }; > argc = parse_options(argc, argv, prefix, options, > builtin_commit_graph_usage, 0); > return fn(argc, argv, prefix); All makes sense and exactly as I expected to see, after reading the later steps [10-20/20] that make use of the facility. Nicely designed. > Here each OPT_SUBCOMMAND specifies the name of the subcommand and the > function implementing it, and the same pointer to 'fn' where > parse_options() will store the function associated with the given > subcommand. Puzzling. Isn't parse_options() an read-only operation with respect to the elements of the struct option array? With s/store/expect to find/, I would understand the above, though. > There is no need to check whether 'fn' is non-NULL before > invoking it: Again, this is puzzling. "There is no need to"---to whom does this statement mean to advise? The implementor of the new codepath IN parse_options() to handle OPT_SUBCOMMAND()? > if there were no subcommands given on the command line > then the parse_options() call would error out and show usage. I think that you mean to say When one or more OPT_SUBCOMMAND elements exist in an array of struct option, parse_options() will error out if none of them triggers. and that makes sense, but I cannot quite tell how it relates to the previous sentence about fn possibly being NULL. Ahh, OK. Let's try to rephrase to see if I can make it easier to understand. Here each OPT_SUBCOMMAND specifies ... with the given subcommand. After parse_options() returns, the variable 'fn' would have been updated to point at the function to call, if one of the subcommand specified by the OPT_SUBCOMMAND() is given on the command line. - When the PARSE_OPT_SUBCOMMAND_OPTIONAL flag is given to parse_options(), and no subcommand is given on the command line, the variable 'fn' is left intact. This can be used to implement a command with the "default" subcommand by initializing the variable 'fn' to the default value. - Otherwise, parse_options() will error out and show usage, when no subcommand is given. - If more than one subcommands are given from the command line, parse_options() will stop at the first one, and treat the remainder of the command line as arguments to the subcommand. > In case > a command has a default operation mode, 'fn' should be initialized to > the function implementing that mode, and parse_options() should be > invoked with the PARSE_OPT_SUBCOMMAND_OPTIONAL flag. OK. > Some thoughts about the implementation: > > - Arguably it is a bit weird that the same pointer to 'fn' have to > be specified as 'value' for each OPT_SUBCOMMAND, but we need a way > to tell parse_options() where to put the function associated with > the given subcommand, and I didn't like the alternatives: I do not find this so disturbing. This is similar to CMDMODE in spirit in that only one has to be chosen. CMDMODE needs to go an extra mile to ensure that by checking the current value in the variable pointed by the pointer because the parser does not stop immediately after getting one, but SUBCOMMAND can afford to be simpler because the parser immediately stops once a subcommand is found. In a sense, OPT_SUBCOMMAND() does not have to exist as the first-class mechanism. With just two primitives: - a flag that says "after seeing this option, immediately stop parsing". - something similar to OPT_SET_INT() but works with a function pointer. we can go 80% of the way to implement OPT_SUBCOMMAND() as a thin wrapper (there is "one of the OPT_SET_FUNC() must be triggered" that needs new code specific to the need, which is the other 20%). > - I decided against automatically calling the subcommand function > from within parse_options(), because: > > - There are commands that have to perform additional actions > after option parsing but before calling the function > implementing the specified subcommand. > > - The return code of the subcommand is usually the return code > of the git command, but preserving the return code of the > automatically called subcommand function would have made the > API awkward. Yes, the above makes perfect sense. Also I suspect that we would find some cases where being able to use two or more variables become handy. > + case PARSE_OPT_COMPLETE: > + case PARSE_OPT_HELP: > + case PARSE_OPT_ERROR: > + case PARSE_OPT_DONE: > + case PARSE_OPT_NON_OPTION: > + BUG("parse_subcommand() cannot return these"); > + } "these" without giving the violating value? A "%d" would be cheap addition, but I see this was copied from elsewhere. Thanks.
On Mon, Jul 25, 2022 at 04:43:30PM +0200, Ævar Arnfjörð Bjarmason wrote: > > diff --git a/builtin/blame.c b/builtin/blame.c > > index 02e39420b6..a9fe8cf7a6 100644 > > --- a/builtin/blame.c > > +++ b/builtin/blame.c > > @@ -920,6 +920,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) > > break; > > case PARSE_OPT_HELP: > > case PARSE_OPT_ERROR: > > + case PARSE_OPT_SUBCOMMAND: > > exit(129); > > case PARSE_OPT_COMPLETE: > > exit(0); > > diff --git a/builtin/shortlog.c b/builtin/shortlog.c > > index 086dfee45a..7a1e1fe7c0 100644 > > --- a/builtin/shortlog.c > > +++ b/builtin/shortlog.c > > @@ -381,6 +381,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) > > break; > > case PARSE_OPT_HELP: > > case PARSE_OPT_ERROR: > > + case PARSE_OPT_SUBCOMMAND: > > exit(129); > > case PARSE_OPT_COMPLETE: > > exit(0); > > This feels a bit like carrying forward an API wart, i.e. shouldn't we > instead BUG() if we are returning a PARSE_OPT_SUBCOMMAND from > parse_options_step() for options lists that don't have > PARSE_OPT_SUBCOMMAND in them? > > I.e. is this even reachable, or just something to suppress the compiler > complaining about missing enum labels? I think it's as good as unreachable, because neither of these two commands have subcommands. However, without these hunks the compiler invoked with '-Wswitch' (implied by '-Wall') does indeed complain. > ...but why not... > > > static void parse_options_start_1(struct parse_opt_ctx_t *ctx, > > int argc, const char **argv, const char *prefix, > > const struct option *options, > > @@ -515,6 +547,19 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, > > ctx->prefix = prefix; > > ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); > > ctx->flags = flags; > > + ctx->has_subcommands = has_subcommands(options); > > + if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) > > + BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands"); > > + if (ctx->has_subcommands) { > > + if (flags & PARSE_OPT_STOP_AT_NON_OPTION) > > + BUG("subcommands are incompatible with PARSE_OPT_STOP_AT_NON_OPTION"); > > + if (!(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) { > > + if (flags & PARSE_OPT_KEEP_UNKNOWN_OPT) > > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_UNKNOWN unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); > > + if (flags & PARSE_OPT_KEEP_DASHDASH) > > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_DASHDASH unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); > > + } > > + } > > ...move this into parse_options_check()? I.e. we'd need to loop over the > list once, but it seems like this should belong there. > > We have an existing bug in-tree due to usage_with_options() not doing a > parse_options_check() (I have a local fix...), checking this sort of > thing there instead of in parse_options_start() is therefore the right > thing to do, i.e. we shoudl have a one-stop "does this options variable > look sane?". The checks added in this hunk (and the existing checks in the hunk's after-context) are not about the elements of the 'struct option' array, like the checks in parse_options_check(), but rather about the sensibility of parse_options()'s 'parse_opt_flags' parameter. usage_with_options() doesn't have (and doesn't at all need) such a parameter. > > + error(_("unknown subcommand: %s"), arg); > > s/%s/'%s'/ while we're at it, perhaps? Indeed, though it should be `%s', because that's what surrounds unknown switches and options in the corresponding error messages. > > + usage_with_options(usagestr, options); > > + case PARSE_OPT_COMPLETE: > > + case PARSE_OPT_HELP: > > + case PARSE_OPT_ERROR: > > + case PARSE_OPT_DONE: > > + case PARSE_OPT_NON_OPTION: > > + BUG("parse_subcommand() cannot return these"); > > nit: BUG("got bad %d", v) or whatever, i.e. say what we got? All these are impossible, so I don't think it matters. This is another case of the compiler with '-Wswitch' complaining, and follows suit of similar switch statements after the calls to parse_short_opt() and parse_long_opt() functions. > > @@ -206,6 +217,11 @@ struct option { > > #define OPT_ALIAS(s, l, source_long_name) \ > > { OPTION_ALIAS, (s), (l), (source_long_name) } > > > > +#define OPT_SUBCOMMAND(l, v, fn) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > > + NULL, 0, NULL, 0, NULL, 0, (fn) } > > +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > > + NULL, (f), NULL, 0, NULL, 0, (fn) } > > Nit, I know you're carrying forward existing patterns, but since that > all pre-dated designated init perhaps we could just (untested): > > #define OPT_SUBCOMMAND_F(l, v, fn, f) { \ > .type = OPTION_SUBCOMMAND, \ > .long_name = (l), \ > .value = (v), \ > .ll_callback = (fn), \ > } > #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) > > Which IMO is much nicer. I have some patches somewhere to convert these > to saner patterns (I think not designated init, but the X() can be > defined in terms of X_F() like that, but since this is new we can use > designated init all the way... Oh, I love this idea! But are we there yet? I remember the weather balloon about designated initializers, but I'm not sure whether we've already made the decision to allow them. If we do, then I'm inclined to volunteer to clean up all those OPT_* macros in 'parse-options.h' with designated initializers, > > +{ > > + int i; > > Nit: missing \n (usual style of variable decl); Hm, speaking of newer C features, what about 'for (int i = 0; ...)'? > > + error("'cmd' is mandatory"); > > + usage_with_options(usage, test_flag_options); > > nit: I think you want usage_msg_optf() or usage_msg_opt(). Maybe... but I don't know what they do ;) Though I remember removing a couple of similar error() and usage_with_options() pairs from the builtin commands.
On Mon, Jul 25 2022, SZEDER Gábor wrote: > On Mon, Jul 25, 2022 at 04:43:30PM +0200, Ævar Arnfjörð Bjarmason wrote: >> > diff --git a/builtin/blame.c b/builtin/blame.c >> > index 02e39420b6..a9fe8cf7a6 100644 >> > --- a/builtin/blame.c >> > +++ b/builtin/blame.c >> > @@ -920,6 +920,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) >> > break; >> > case PARSE_OPT_HELP: >> > case PARSE_OPT_ERROR: >> > + case PARSE_OPT_SUBCOMMAND: >> > exit(129); >> > case PARSE_OPT_COMPLETE: >> > exit(0); >> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c >> > index 086dfee45a..7a1e1fe7c0 100644 >> > --- a/builtin/shortlog.c >> > +++ b/builtin/shortlog.c >> > @@ -381,6 +381,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) >> > break; >> > case PARSE_OPT_HELP: >> > case PARSE_OPT_ERROR: >> > + case PARSE_OPT_SUBCOMMAND: >> > exit(129); >> > case PARSE_OPT_COMPLETE: >> > exit(0); >> >> This feels a bit like carrying forward an API wart, i.e. shouldn't we >> instead BUG() if we are returning a PARSE_OPT_SUBCOMMAND from >> parse_options_step() for options lists that don't have >> PARSE_OPT_SUBCOMMAND in them? >> >> I.e. is this even reachable, or just something to suppress the compiler >> complaining about missing enum labels? > > I think it's as good as unreachable, because neither of these two > commands have subcommands. However, without these hunks the compiler > invoked with '-Wswitch' (implied by '-Wall') does indeed complain. Yeah, we should add a case, but let's just do: case PARSE_OPT_SUBCOMMAND: BUG("unreachable"); >> ...but why not... >> >> > static void parse_options_start_1(struct parse_opt_ctx_t *ctx, >> > int argc, const char **argv, const char *prefix, >> > const struct option *options, >> > @@ -515,6 +547,19 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, >> > ctx->prefix = prefix; >> > ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); >> > ctx->flags = flags; >> > + ctx->has_subcommands = has_subcommands(options); >> > + if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) >> > + BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands"); >> > + if (ctx->has_subcommands) { >> > + if (flags & PARSE_OPT_STOP_AT_NON_OPTION) >> > + BUG("subcommands are incompatible with PARSE_OPT_STOP_AT_NON_OPTION"); >> > + if (!(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) { >> > + if (flags & PARSE_OPT_KEEP_UNKNOWN_OPT) >> > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_UNKNOWN unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); >> > + if (flags & PARSE_OPT_KEEP_DASHDASH) >> > + BUG("subcommands are incompatible with PARSE_OPT_KEEP_DASHDASH unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); >> > + } >> > + } >> >> ...move this into parse_options_check()? I.e. we'd need to loop over the >> list once, but it seems like this should belong there. >> >> We have an existing bug in-tree due to usage_with_options() not doing a >> parse_options_check() (I have a local fix...), checking this sort of >> thing there instead of in parse_options_start() is therefore the right >> thing to do, i.e. we shoudl have a one-stop "does this options variable >> look sane?". > > The checks added in this hunk (and the existing checks in the hunk's > after-context) are not about the elements of the 'struct option' > array, like the checks in parse_options_check(), but rather about the > sensibility of parse_options()'s 'parse_opt_flags' parameter. > usage_with_options() doesn't have (and doesn't at all need) such a > parameter. Ah, sorry, I was just confused. FWIW it's because I split out *that* part into another helper a while ago: https://github.com/avar/git/commit/55dda82a409 Which might be worthhile doing/stealing heere while we're at it, i.e. the flags checking has become quite ab ig part of parse_options_start_1(), or just leave it for later... >> > + error(_("unknown subcommand: %s"), arg); >> >> s/%s/'%s'/ while we're at it, perhaps? > > Indeed, though it should be `%s', because that's what surrounds > unknown switches and options in the corresponding error messages. > >> > + usage_with_options(usagestr, options); >> > + case PARSE_OPT_COMPLETE: >> > + case PARSE_OPT_HELP: >> > + case PARSE_OPT_ERROR: >> > + case PARSE_OPT_DONE: >> > + case PARSE_OPT_NON_OPTION: >> > + BUG("parse_subcommand() cannot return these"); >> >> nit: BUG("got bad %d", v) or whatever, i.e. say what we got? > > All these are impossible, so I don't think it matters. This is > another case of the compiler with '-Wswitch' complaining, and follows > suit of similar switch statements after the calls to parse_short_opt() > and parse_long_opt() functions. *nod*, and this one's a BUG(), which is good... >> > @@ -206,6 +217,11 @@ struct option { >> > #define OPT_ALIAS(s, l, source_long_name) \ >> > { OPTION_ALIAS, (s), (l), (source_long_name) } >> > >> > +#define OPT_SUBCOMMAND(l, v, fn) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ >> > + NULL, 0, NULL, 0, NULL, 0, (fn) } >> > +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ >> > + NULL, (f), NULL, 0, NULL, 0, (fn) } >> >> Nit, I know you're carrying forward existing patterns, but since that >> all pre-dated designated init perhaps we could just (untested): >> >> #define OPT_SUBCOMMAND_F(l, v, fn, f) { \ >> .type = OPTION_SUBCOMMAND, \ >> .long_name = (l), \ >> .value = (v), \ >> .ll_callback = (fn), \ >> } >> #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) >> >> Which IMO is much nicer. I have some patches somewhere to convert these >> to saner patterns (I think not designated init, but the X() can be >> defined in terms of X_F() like that, but since this is new we can use >> designated init all the way... > > Oh, I love this idea! But are we there yet? I remember the weather > balloon about designated initializers, but I'm not sure whether we've > already made the decision to allow them. Yes, we've got a thoroughly hard dependency on that part of C99 for a while now, and it's OK to add new ones (especially in cases like these, where it makes thigs easier to read). > If we do, then I'm inclined > to volunteer to clean up all those OPT_* macros in 'parse-options.h' > with designated initializers, Sounds good, you might want to steal this & perhaps some things on the same branch: https://github.com/avar/git/commit/a1a5e9c68c8 I didn't convert them to designated init, but some macros are "missing", some are needlessy copy/pasted when X_F() could be defined in terms of X() etc. FWIW I thought it would eventually make sense to rename the members of the struct itself, so we'd e.g. just have a "t" and "l" name, so we could use that inline instead of the OPT_*() (we could use the long names too, but that would probably be too verbose). That would allow adding optional arguments, which e.g. would be handy for things like "...and here's a list of what options this is incompatible with". > >> > +{ >> > + int i; >> >> Nit: missing \n (usual style of variable decl); > > Hm, speaking of newer C features, what about 'for (int i = 0; ...)'? Junio says this November, but see: https://lore.kernel.org/git/220725.86zggxpfed.gmgdl@evledraar.gmail.com/ >> > + error("'cmd' is mandatory"); >> > + usage_with_options(usage, test_flag_options); >> >> nit: I think you want usage_msg_optf() or usage_msg_opt(). > > Maybe... but I don't know what they do ;) Though I remember removing > a couple of similar error() and usage_with_options() pairs from the > builtin commands. It's just helpers for "usage_with_options, except with a message, e.g.: $ ./git cat-file a b c fatal: only two arguments allowed in <type> <object> mode, not 3 usage: git cat-file <type> <object>
On Mon, Jul 25, 2022 at 09:41:52PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > + error("'cmd' is mandatory"); > >> > + usage_with_options(usage, test_flag_options); > >> > >> nit: I think you want usage_msg_optf() or usage_msg_opt(). > > > > Maybe... but I don't know what they do ;) Though I remember removing > > a couple of similar error() and usage_with_options() pairs from the > > builtin commands. > > It's just helpers for "usage_with_options, except with a message, e.g.: > > $ ./git cat-file a b c > fatal: only two arguments allowed in <type> <object> mode, not 3 > > usage: git cat-file <type> <object> I've looked them up in the meantime. The problem is that they both output the given message with a "fatal:" prefix, but option parsing related errors are usually prefixed with "error:".
On Mon, Jul 25, 2022 at 09:41:52PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > @@ -206,6 +217,11 @@ struct option { > >> > #define OPT_ALIAS(s, l, source_long_name) \ > >> > { OPTION_ALIAS, (s), (l), (source_long_name) } > >> > > >> > +#define OPT_SUBCOMMAND(l, v, fn) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > >> > + NULL, 0, NULL, 0, NULL, 0, (fn) } > >> > +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ > >> > + NULL, (f), NULL, 0, NULL, 0, (fn) } > >> > >> Nit, I know you're carrying forward existing patterns, but since that > >> all pre-dated designated init perhaps we could just (untested): > >> > >> #define OPT_SUBCOMMAND_F(l, v, fn, f) { \ > >> .type = OPTION_SUBCOMMAND, \ > >> .long_name = (l), \ > >> .value = (v), \ > >> .ll_callback = (fn), \ > >> } > >> #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) > >> > >> Which IMO is much nicer. I have some patches somewhere to convert these > >> to saner patterns (I think not designated init, but the X() can be > >> defined in terms of X_F() like that, but since this is new we can use > >> designated init all the way... > > > > Oh, I love this idea! But are we there yet? I remember the weather > > balloon about designated initializers, but I'm not sure whether we've > > already made the decision to allow them. > > Yes, we've got a thoroughly hard dependency on that part of C99 for a > while now, and it's OK to add new ones (especially in cases like these, > where it makes thigs easier to read). Good. I updated this hunk to use designated initializers as you suggested, because all those unused 0/NULL fields in there are just... ugly. > > If we do, then I'm inclined > > to volunteer to clean up all those OPT_* macros in 'parse-options.h' > > with designated initializers, But I'll leave this for later, because it's awfully easy to make a mistake and assign a macro parameter to the wrong field, and I find the resulting diff very hard to review.
diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 4412377fa3..845111d6a8 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -8,7 +8,8 @@ Basics ------ The argument vector `argv[]` may usually contain mandatory or optional -'non-option arguments', e.g. a filename or a branch, and 'options'. +'non-option arguments', e.g. a filename or a branch, 'options', and +'subcommands'. Options are optional arguments that start with a dash and that allow to change the behavior of a command. @@ -48,6 +49,33 @@ The parse-options API allows: option, e.g. `-a -b --option -- --this-is-a-file` indicates that `--this-is-a-file` must not be processed as an option. +Subcommands are special in a couple of ways: + +* Subcommands only have long form, and they have no double dash prefix, no + negated form, and no description, and they don't take any arguments, and + can't be abbreviated. + +* There must be exactly one subcommand among the arguments, or zero if the + command has a default operation mode. + +* All arguments following the subcommand are considered to be arguments of + the subcommand, and, conversely, arguments meant for the subcommand may + not preceed the subcommand. + +Therefore, if the options array contains at least one subcommand and +`parse_options()` encounters the first dashless argument, it will either: + +* stop and return, if that dashless argument is a known subcommand, setting + `value` to the function pointer associated with that subcommand, storing + the name of the subcommand in argv[0], and leaving the rest of the + arguments unprocessed, or + +* stop and return, if it was invoked with the `PARSE_OPT_SUBCOMMAND_OPTIONAL` + flag and that dashless argument doesn't match any subcommands, leaving + `value` unchanged and the rest of the arguments unprocessed, or + +* show error and usage, and abort. + Steps to parse options ---------------------- @@ -110,6 +138,13 @@ Flags are the bitwise-or of: turns it off and allows one to add custom handlers for these options, or to just leave them unknown. +`PARSE_OPT_SUBCOMMAND_OPTIONAL`:: + Don't error out when no subcommand is specified. + +Note that `PARSE_OPT_STOP_AT_NON_OPTION` is incompatible with subcommands; +while `PARSE_OPT_KEEP_DASHDASH` and `PARSE_OPT_KEEP_UNKNOWN` can only be +used with subcommands when combined with `PARSE_OPT_SUBCOMMAND_OPTIONAL`. + Data Structure -------------- @@ -241,7 +276,11 @@ There are some macros to easily define options: can be given by the user. `int_var` is set to `enum_val` when the option is used, but an error is reported if other "operating mode" option has already set its value to the same `int_var`. + In new commands consider using subcommands instead. +`OPT_SUBCOMMAND(long, &fn_ptr, subcommand_fn)`:: + Define a subcommand. `subcommand_fn` is put into `fn_ptr` when + this subcommand is used. The last element of the array must be `OPT_END()`. diff --git a/builtin/blame.c b/builtin/blame.c index 02e39420b6..a9fe8cf7a6 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -920,6 +920,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: + case PARSE_OPT_SUBCOMMAND: exit(129); case PARSE_OPT_COMPLETE: exit(0); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 086dfee45a..7a1e1fe7c0 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -381,6 +381,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: + case PARSE_OPT_SUBCOMMAND: exit(129); case PARSE_OPT_COMPLETE: exit(0); diff --git a/parse-options.c b/parse-options.c index 8748f88e6f..7c4c805d51 100644 --- a/parse-options.c +++ b/parse-options.c @@ -324,6 +324,8 @@ static enum parse_opt_result parse_long_opt( const char *rest, *long_name = options->long_name; enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG; + if (options->type == OPTION_SUBCOMMAND) + continue; if (!long_name) continue; @@ -419,6 +421,19 @@ static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p, return PARSE_OPT_ERROR; } +static enum parse_opt_result parse_subcommand(const char *arg, + const struct option *options) +{ + for (; options->type != OPTION_END; options++) + if (options->type == OPTION_SUBCOMMAND && + !strcmp(options->long_name, arg)) { + *(parse_opt_subcommand_fn **)options->value = options->subcommand_fn; + return PARSE_OPT_SUBCOMMAND; + } + + return PARSE_OPT_UNKNOWN; +} + static void check_typos(const char *arg, const struct option *options) { if (strlen(arg) < 3) @@ -442,6 +457,7 @@ static void check_typos(const char *arg, const struct option *options) static void parse_options_check(const struct option *opts) { char short_opts[128]; + void *subcommand_value = NULL; memset(short_opts, '\0', sizeof(short_opts)); for (; opts->type != OPTION_END; opts++) { @@ -489,6 +505,14 @@ static void parse_options_check(const struct option *opts) "Are you using parse_options_step() directly?\n" "That case is not supported yet."); break; + case OPTION_SUBCOMMAND: + if (!opts->value || !opts->subcommand_fn) + optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function"); + if (!subcommand_value) + subcommand_value = opts->value; + else if (subcommand_value != opts->value) + optbug(opts, "all OPTION_SUBCOMMANDs need the same value"); + break; default: ; /* ok. (usually accepts an argument) */ } @@ -499,6 +523,14 @@ static void parse_options_check(const struct option *opts) BUG_if_bug("invalid 'struct option'"); } +static int has_subcommands(const struct option *options) +{ + for (; options->type != OPTION_END; options++) + if (options->type == OPTION_SUBCOMMAND) + return 1; + return 0; +} + static void parse_options_start_1(struct parse_opt_ctx_t *ctx, int argc, const char **argv, const char *prefix, const struct option *options, @@ -515,6 +547,19 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, ctx->prefix = prefix; ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0); ctx->flags = flags; + ctx->has_subcommands = has_subcommands(options); + if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) + BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands"); + if (ctx->has_subcommands) { + if (flags & PARSE_OPT_STOP_AT_NON_OPTION) + BUG("subcommands are incompatible with PARSE_OPT_STOP_AT_NON_OPTION"); + if (!(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) { + if (flags & PARSE_OPT_KEEP_UNKNOWN_OPT) + BUG("subcommands are incompatible with PARSE_OPT_KEEP_UNKNOWN unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); + if (flags & PARSE_OPT_KEEP_DASHDASH) + BUG("subcommands are incompatible with PARSE_OPT_KEEP_DASHDASH unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL"); + } + } if ((flags & PARSE_OPT_KEEP_UNKNOWN_OPT) && (flags & PARSE_OPT_STOP_AT_NON_OPTION) && !(flags & PARSE_OPT_ONE_SHOT)) @@ -589,6 +634,7 @@ static int show_gitcomp(const struct option *opts, int show_all) int nr_noopts = 0; for (; opts->type != OPTION_END; opts++) { + const char *prefix = "--"; const char *suffix = ""; if (!opts->long_name) @@ -598,6 +644,9 @@ static int show_gitcomp(const struct option *opts, int show_all) continue; switch (opts->type) { + case OPTION_SUBCOMMAND: + prefix = ""; + break; case OPTION_GROUP: continue; case OPTION_STRING: @@ -620,8 +669,8 @@ static int show_gitcomp(const struct option *opts, int show_all) suffix = "="; if (starts_with(opts->long_name, "no-")) nr_noopts++; - printf("%s--%s%s", opts == original_opts ? "" : " ", - opts->long_name, suffix); + printf("%s%s%s%s", opts == original_opts ? "" : " ", + prefix, opts->long_name, suffix); } show_negated_gitcomp(original_opts, show_all, -1); show_negated_gitcomp(original_opts, show_all, nr_noopts); @@ -744,10 +793,37 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, if (*arg != '-' || !arg[1]) { if (parse_nodash_opt(ctx, arg, options) == 0) continue; - if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION) - return PARSE_OPT_NON_OPTION; - ctx->out[ctx->cpidx++] = ctx->argv[0]; - continue; + if (!ctx->has_subcommands) { + if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION) + return PARSE_OPT_NON_OPTION; + ctx->out[ctx->cpidx++] = ctx->argv[0]; + continue; + } + switch (parse_subcommand(arg, options)) { + case PARSE_OPT_SUBCOMMAND: + return PARSE_OPT_SUBCOMMAND; + case PARSE_OPT_UNKNOWN: + if (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL) + /* + * arg is neither a short or long + * option nor a subcommand. Since + * this command has a default + * operation mode, we have to treat + * this arg and all remaining args + * as args meant to that default + * operation mode. + * So we are done parsing. + */ + return PARSE_OPT_DONE; + error(_("unknown subcommand: %s"), arg); + usage_with_options(usagestr, options); + case PARSE_OPT_COMPLETE: + case PARSE_OPT_HELP: + case PARSE_OPT_ERROR: + case PARSE_OPT_DONE: + case PARSE_OPT_NON_OPTION: + BUG("parse_subcommand() cannot return these"); + } } /* lone -h asks for help */ @@ -775,6 +851,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, goto show_usage; goto unknown; case PARSE_OPT_NON_OPTION: + case PARSE_OPT_SUBCOMMAND: case PARSE_OPT_HELP: case PARSE_OPT_COMPLETE: BUG("parse_short_opt() cannot return these"); @@ -800,6 +877,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, *(char *)ctx->argv[0] = '-'; goto unknown; case PARSE_OPT_NON_OPTION: + case PARSE_OPT_SUBCOMMAND: case PARSE_OPT_COMPLETE: case PARSE_OPT_HELP: BUG("parse_short_opt() cannot return these"); @@ -831,6 +909,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, case PARSE_OPT_HELP: goto show_usage; case PARSE_OPT_NON_OPTION: + case PARSE_OPT_SUBCOMMAND: case PARSE_OPT_COMPLETE: BUG("parse_long_opt() cannot return these"); case PARSE_OPT_DONE: @@ -840,8 +919,21 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, unknown: if (ctx->flags & PARSE_OPT_ONE_SHOT) break; + if (ctx->has_subcommands && + (ctx->flags & PARSE_OPT_SUBCOMMAND_OPTIONAL) && + (ctx->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)) { + /* + * Found an unknown option given to a command with + * subcommands that has a default operation mode: + * we treat this option and all remaining args as + * arguments meant to that default operation mode. + * So we are done parsing. + */ + return PARSE_OPT_DONE; + } if (!(ctx->flags & PARSE_OPT_KEEP_UNKNOWN_OPT)) return PARSE_OPT_UNKNOWN; + ctx->kept_unknown = 1; ctx->out[ctx->cpidx++] = ctx->argv[0]; ctx->opt = NULL; } @@ -885,7 +977,14 @@ int parse_options(int argc, const char **argv, case PARSE_OPT_COMPLETE: exit(0); case PARSE_OPT_NON_OPTION: + case PARSE_OPT_SUBCOMMAND: + break; case PARSE_OPT_DONE: + if (ctx.has_subcommands && + !(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) { + error(_("need a subcommand")); + usage_with_options(usagestr, options); + } break; case PARSE_OPT_UNKNOWN: if (ctx.argv[0][1] == '-') { @@ -1010,6 +1109,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t size_t pos; int pad; + if (opts->type == OPTION_SUBCOMMAND) + continue; if (opts->type == OPTION_GROUP) { fputc('\n', outfile); need_newline = 0; diff --git a/parse-options.h b/parse-options.h index 8cbfc7e8bf..b089b6fa03 100644 --- a/parse-options.h +++ b/parse-options.h @@ -11,6 +11,7 @@ enum parse_opt_type { OPTION_GROUP, OPTION_NUMBER, OPTION_ALIAS, + OPTION_SUBCOMMAND, /* options with no arguments */ OPTION_BIT, OPTION_NEGBIT, @@ -34,6 +35,7 @@ enum parse_opt_flags { PARSE_OPT_NO_INTERNAL_HELP = 1 << 4, PARSE_OPT_ONE_SHOT = 1 << 5, PARSE_OPT_SHELL_EVAL = 1 << 6, + PARSE_OPT_SUBCOMMAND_OPTIONAL = 1 << 7, }; enum parse_opt_option_flags { @@ -56,6 +58,7 @@ enum parse_opt_result { PARSE_OPT_ERROR = -1, /* must be the same as error() */ PARSE_OPT_DONE = 0, /* fixed so that "return 0" works */ PARSE_OPT_NON_OPTION, + PARSE_OPT_SUBCOMMAND, PARSE_OPT_UNKNOWN }; @@ -67,6 +70,9 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, const struct option *opt, const char *arg, int unset); +typedef int parse_opt_subcommand_fn(int argc, const char **argv, + const char *prefix); + /* * `type`:: * holds the type of the option, you must have an OPTION_END last in your @@ -76,7 +82,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * the character to use as a short option name, '\0' if none. * * `long_name`:: - * the long option name, without the leading dashes, NULL if none. + * the long option (without the leading dashes) or subcommand name, + * NULL if none. * * `value`:: * stores pointers to the values to be filled. @@ -93,7 +100,7 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * * `help`:: * the short help associated to what the option does. - * Must never be NULL (except for OPTION_END). + * Must never be NULL (except for OPTION_END and OPTION_SUBCOMMAND). * OPTION_GROUP uses this pointer to store the group header. * Should be wrapped by N_() for translation. * @@ -131,6 +138,9 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * `ll_callback`:: * pointer to the callback to use for OPTION_LOWLEVEL_CALLBACK * + * `subcommand_fn`:: + * pointer to a function to use for OPTION_SUBCOMMAND. + * It will be put in value when the subcommand is given on the command line. */ struct option { enum parse_opt_type type; @@ -145,6 +155,7 @@ struct option { intptr_t defval; parse_opt_ll_cb *ll_callback; intptr_t extra; + parse_opt_subcommand_fn *subcommand_fn; }; #define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \ @@ -206,6 +217,11 @@ struct option { #define OPT_ALIAS(s, l, source_long_name) \ { OPTION_ALIAS, (s), (l), (source_long_name) } +#define OPT_SUBCOMMAND(l, v, fn) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ + NULL, 0, NULL, 0, NULL, 0, (fn) } +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \ + NULL, (f), NULL, 0, NULL, 0, (fn) } + /* * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and @@ -295,6 +311,8 @@ struct parse_opt_ctx_t { int argc, cpidx, total; const char *opt; enum parse_opt_flags flags; + unsigned has_subcommands:1, + kept_unknown:1; const char *prefix; const char **alias_groups; /* must be in groups of 3 elements! */ struct option *updated_options; diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index a715c772bd..07a4e185ef 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -233,6 +233,9 @@ static const struct option test_flag_options[] = { OPT_BIT(0, "no-internal-help", &test_flags, "pass PARSE_OPT_NO_INTERNAL_HELP to parse_options()", PARSE_OPT_NO_INTERNAL_HELP), + OPT_BIT(0, "subcommand-optional", &test_flags, + "pass PARSE_OPT_SUBCOMMAND_OPTIONAL to parse_options()", + PARSE_OPT_SUBCOMMAND_OPTIONAL), OPT_END() }; @@ -253,3 +256,68 @@ int cmd__parse_options_flags(int argc, const char **argv) return parse_options_flags__cmd(argc, argv, test_flags); } + +static void print_subcommand_args(const char *fn_name, int argc, + const char **argv) +{ + int i; + printf("fn: %s\n", fn_name); + for (i = 0; i < argc; i++) + printf("arg %02d: %s\n", i, argv[i]); +} + +static int subcmd_one(int argc, const char **argv, const char *prefix) +{ + print_subcommand_args("subcmd_one", argc, argv); + return 0; +} + +static int subcmd_two(int argc, const char **argv, const char *prefix) +{ + print_subcommand_args("subcmd_two", argc, argv); + return 0; +} + +static int parse_subcommand__cmd(int argc, const char **argv, + enum parse_opt_flags test_flags) +{ + const char *usage[] = { + "<...> cmd subcmd-one", + "<...> cmd subcmd-two", + NULL + }; + parse_opt_subcommand_fn *fn = NULL; + int opt = 0; + struct option options[] = { + OPT_SUBCOMMAND("subcmd-one", &fn, subcmd_one), + OPT_SUBCOMMAND("subcmd-two", &fn, subcmd_two), + OPT_INTEGER('o', "opt", &opt, "an integer option"), + OPT_END() + }; + + if (test_flags & PARSE_OPT_SUBCOMMAND_OPTIONAL) + fn = subcmd_one; + argc = parse_options(argc, argv, NULL, options, usage, test_flags); + + printf("opt: %d\n", opt); + + return fn(argc, argv, NULL); +} + +int cmd__parse_subcommand(int argc, const char **argv) +{ + const char *usage[] = { + "test-tool parse-subcommand [flag-options] cmd <subcommand>", + NULL + }; + + argc = parse_options(argc, argv, NULL, test_flag_options, usage, + PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc == 0 || strcmp(argv[0], "cmd")) { + error("'cmd' is mandatory"); + usage_with_options(usage, test_flag_options); + } + + return parse_subcommand__cmd(argc, argv, test_flags); +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 6e62282b60..49b30057f6 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -53,6 +53,7 @@ static struct test_cmd cmds[] = { { "parse-options", cmd__parse_options }, { "parse-options-flags", cmd__parse_options_flags }, { "parse-pathspec-file", cmd__parse_pathspec_file }, + { "parse-subcommand", cmd__parse_subcommand }, { "partial-clone", cmd__partial_clone }, { "path-utils", cmd__path_utils }, { "pcre2-config", cmd__pcre2_config }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index d8e8403d70..487d84da60 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -43,6 +43,7 @@ int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); int cmd__parse_options_flags(int argc, const char **argv); int cmd__parse_pathspec_file(int argc, const char** argv); +int cmd__parse_subcommand(int argc, const char **argv); int cmd__partial_clone(int argc, const char **argv); int cmd__path_utils(int argc, const char **argv); int cmd__pcre2_config(int argc, const char **argv); diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 673a01ca71..a5f9ef363f 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -526,4 +526,189 @@ test_expect_success 'KEEP_UNKNOWN_OPT | NO_INTERNAL_HELP works' ' test_cmp expect actual ' +test_expect_success 'subcommand - no subcommand shows error and usage' ' + test_expect_code 129 test-tool parse-subcommand cmd 2>err && + grep "^error: need a subcommand" err && + grep ^usage: err +' + +test_expect_success 'subcommand - subcommand after -- shows error and usage' ' + test_expect_code 129 test-tool parse-subcommand cmd -- subcmd-one 2>err && + grep "^error: need a subcommand" err && + grep ^usage: err +' + +test_expect_success 'subcommand - subcommand after --end-of-options shows error and usage' ' + test_expect_code 129 test-tool parse-subcommand cmd --end-of-options subcmd-one 2>err && + grep "^error: need a subcommand" err && + grep ^usage: err +' + +test_expect_success 'subcommand - unknown subcommand shows error and usage' ' + test_expect_code 129 test-tool parse-subcommand cmd nope 2>err && + grep "^error: unknown subcommand: nope" err && + grep ^usage: err +' + +test_expect_success 'subcommand - subcommands cannot be abbreviated' ' + test_expect_code 129 test-tool parse-subcommand cmd subcmd-o 2>err && + grep "^error: unknown subcommand: subcmd-o$" err && + grep ^usage: err +' + +test_expect_success 'subcommand - no negated subcommands' ' + test_expect_code 129 test-tool parse-subcommand cmd no-subcmd-one 2>err && + grep "^error: unknown subcommand: no-subcmd-one" err && + grep ^usage: err +' + +test_expect_success 'subcommand - simple' ' + test-tool parse-subcommand cmd subcmd-two >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_two + arg 00: subcmd-two + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - stop parsing at the first subcommand' ' + test-tool parse-subcommand cmd --opt=1 subcmd-two subcmd-one --opt=2 >actual && + cat >expect <<-\EOF && + opt: 1 + fn: subcmd_two + arg 00: subcmd-two + arg 01: subcmd-one + arg 02: --opt=2 + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - KEEP_ARGV0' ' + test-tool parse-subcommand --keep-argv0 cmd subcmd-two >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_two + arg 00: cmd + arg 01: subcmd-two + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL + subcommand not given' ' + test-tool parse-subcommand --subcommand-optional cmd >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL + given subcommand' ' + test-tool parse-subcommand --subcommand-optional cmd subcmd-two branch file >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_two + arg 00: subcmd-two + arg 01: branch + arg 02: file + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL + subcommand not given + unknown dashless args' ' + test-tool parse-subcommand --subcommand-optional cmd branch file >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: branch + arg 01: file + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL + subcommand not given + unknown option' ' + test_expect_code 129 test-tool parse-subcommand --subcommand-optional cmd --subcommand-opt 2>err && + grep "^error: unknown option" err && + grep ^usage: err +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL | KEEP_UNKNOWN_OPT + subcommand not given + unknown option' ' + test-tool parse-subcommand --subcommand-optional --keep-unknown-opt cmd --subcommand-opt >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: --subcommand-opt + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL | KEEP_UNKNOWN_OPT + subcommand ignored after unknown option' ' + test-tool parse-subcommand --subcommand-optional --keep-unknown-opt cmd --subcommand-opt subcmd-two >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: --subcommand-opt + arg 01: subcmd-two + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL | KEEP_UNKNOWN_OPT + command and subcommand options cannot be mixed' ' + test-tool parse-subcommand --subcommand-optional --keep-unknown-opt cmd --subcommand-opt branch --opt=1 >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: --subcommand-opt + arg 01: branch + arg 02: --opt=1 + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL | KEEP_UNKNOWN_OPT | KEEP_ARGV0' ' + test-tool parse-subcommand --subcommand-optional --keep-unknown-opt --keep-argv0 cmd --subcommand-opt branch >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: cmd + arg 01: --subcommand-opt + arg 02: branch + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - SUBCOMMAND_OPTIONAL | KEEP_UNKNOWN_OPT | KEEP_DASHDASH' ' + test-tool parse-subcommand --subcommand-optional --keep-unknown-opt --keep-dashdash cmd -- --subcommand-opt file >actual && + cat >expect <<-\EOF && + opt: 0 + fn: subcmd_one + arg 00: -- + arg 01: --subcommand-opt + arg 02: file + EOF + test_cmp expect actual +' + +test_expect_success 'subcommand - completion helper' ' + test-tool parse-subcommand cmd --git-completion-helper >actual && + echo "subcmd-one subcmd-two --opt= --no-opt" >expect && + test_cmp expect actual +' + +test_expect_success 'subcommands are incompatible with STOP_AT_NON_OPTION' ' + test_must_fail test-tool parse-subcommand --stop-at-non-option cmd subcmd-one 2>err && + grep ^BUG err +' + +test_expect_success 'subcommands are incompatible with KEEP_UNKNOWN_OPT unless in combination with SUBCOMMAND_OPTIONAL' ' + test_must_fail test-tool parse-subcommand --keep-unknown-opt cmd subcmd-two 2>err && + grep ^BUG err +' + +test_expect_success 'subcommands are incompatible with KEEP_DASHDASH unless in combination with SUBCOMMAND_OPTIONAL' ' + test_must_fail test-tool parse-subcommand --keep-dashdash cmd subcmd-two 2>err && + grep ^BUG err +' + test_done
Several Git commands have subcommands to implement mutually exclusive "operation modes", and they usually parse their subcommand argument with a bunch of if-else if statements. Teach parse-options to handle subcommands as well, which will result in shorter and simpler code with consistent error handling and error messages on unknown or missing subcommand, and it will also make possible for our Bash completion script to handle subcommands programmatically. The approach is guided by the following observations: - Most subcommands [1] are implemented in dedicated functions, and most of those functions [2] either have a signature matching the 'int cmd_foo(int argc, const char **argc, const char *prefix)' signature of builtin commands or can be trivially converted to that signature, because they miss only that last prefix parameter or have no parameters at all. - Subcommand arguments only have long form, and they have no double dash prefix, no negated form, and no description, and they don't take any arguments, and can't be abbreviated. - There must be exactly one subcommand among the arguments, or zero if the command has a default operation mode. - All arguments following the subcommand are considered to be arguments of the subcommand, and, conversely, arguments meant for the subcommand may not preceed the subcommand. So in the end subcommand declaration and parsing would look something like this: parse_opt_subcommand_fn *fn = NULL; struct option builtin_commit_graph_options[] = { OPT_STRING(0, "object-dir", &opts.obj_dir, N_("dir"), N_("the object directory to store the graph")), OPT_SUBCOMMAND("verify", &fn, graph_verify), OPT_SUBCOMMAND("write", &fn, graph_write), OPT_END(), }; argc = parse_options(argc, argv, prefix, options, builtin_commit_graph_usage, 0); return fn(argc, argv, prefix); Here each OPT_SUBCOMMAND specifies the name of the subcommand and the function implementing it, and the same pointer to 'fn' where parse_options() will store the function associated with the given subcommand. There is no need to check whether 'fn' is non-NULL before invoking it: if there were no subcommands given on the command line then the parse_options() call would error out and show usage. In case a command has a default operation mode, 'fn' should be initialized to the function implementing that mode, and parse_options() should be invoked with the PARSE_OPT_SUBCOMMAND_OPTIONAL flag. Some thoughts about the implementation: - Arguably it is a bit weird that the same pointer to 'fn' have to be specified as 'value' for each OPT_SUBCOMMAND, but we need a way to tell parse_options() where to put the function associated with the given subcommand, and I didn't like the alternatives: - Change parse_options()'s signature by adding a pointer to subcommand function to be set to the function associated with the given subcommand, affecting all callsites, even those that don't have subcommands. - Introduce a specific parse_options_and_subcommand() variant with that extra funcion parameter. - I decided against automatically calling the subcommand function from within parse_options(), because: - There are commands that have to perform additional actions after option parsing but before calling the function implementing the specified subcommand. - The return code of the subcommand is usually the return code of the git command, but preserving the return code of the automatically called subcommand function would have made the API awkward. - Also add a OPT_SUBCOMMAND_F() variant to allow specifying an option flag: we have two subcommands that are purposefully excluded from completion ('git remote rm' and 'git stash save'), so they'll have to be specified with the PARSE_OPT_NOCOMPLETE flag. - Some of the 'parse_opt_flags' don't make sense with subcommands, and using them is probably just an oversight or misunderstanding. Therefore parse_options() will BUG() when invoked with any of the following flags while the options array contains at least one OPT_SUBCOMMAND: - PARSE_OPT_KEEP_DASHDASH: parse_options() stops parsing arguments when encountering a "--" argument, so it doesn't make sense to expect and keep one before a subcommand, because it would prevent the parsing of the subcommand. However, this flag is allowed in combination with the PARSE_OPT_SUBCOMMAND_OPTIONAL flag, because the double dash might be meaningful for the command's default operation mode, e.g. to disambiguate refs and pathspecs. - PARSE_OPT_STOP_AT_NON_OPTION: As its name suggests, this flag tells parse_options() to stop as soon as it encouners a non-option argument, but subcommands are by definition not options... so how could they be parsed, then?! - PARSE_OPT_KEEP_UNKNOWN: This flag can be used to collect any unknown --options and then pass them to a different command or subsystem. Surely if a command has subcommands, then this functionality should rather be delegated to one of those subcommands, and not performed by the command itself. However, this flag is allowed in combination with the PARSE_OPT_SUBCOMMAND_OPTIONAL flag, making possible to pass --options to the default operation mode. - If the command with subcommands has a default operation mode, then all arguments to the command must preceed the arguments of the subcommand. AFAICT we don't have any commands where this makes a difference, because in those commands either only the command accepts any arguments ('notes' and 'remote'), or only the default subcommand ('reflog' and 'stash'), but never both. - The 'argv' array passed to subcommand functions currently starts with the name of the subcommand. Keep this behavior. AFAICT no subcommand functions depend on the actual content of 'argv[0]', but the parse_options() call handling their options expects that the options start at argv[1]. - To support handling subcommands programmatically in our Bash completion script, 'git cmd --git-completion-helper' will now list both subcommands and regular --options, if any. This means that the completion script will have to separate subcommands (i.e. words without a double dash prefix) from --options on its own, but that's rather easy to do, and it's not much work either, because the number of subcommands a command might have is rather low, and those commands accept only a single --option or none at all. An alternative would be to introduce a separate option that lists only subcommands, but then the completion script would need not one but two git invocations and command substitutions for commands with subcommands. Note that this change doesn't affect the behavior of our Bash completion script, because when completing the --option of a command with subcommands, e.g. for 'git notes --<TAB>', then all subcommands will be filtered out anyway, as none of them will match the word to be completed starting with that double dash prefix. [1] Except 'git rerere', because many of its subcommands are implemented in the bodies of the if-else if statements parsing the command's subcommand argument. [2] Except 'credential', 'credential-store' and 'fsmonitor--daemon', because some of the functions implementing their subcommands take special parameters. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Documentation/technical/api-parse-options.txt | 41 +++- builtin/blame.c | 1 + builtin/shortlog.c | 1 + parse-options.c | 113 ++++++++++- parse-options.h | 22 ++- t/helper/test-parse-options.c | 68 +++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0040-parse-options.sh | 185 ++++++++++++++++++ 9 files changed, 424 insertions(+), 9 deletions(-)