| Message ID | 20220819160411.1791200-16-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 54ef7676bab9292c041e6ada73bb48a4c261c71b |
| Headers | show |
| Series | parse-options: handle subcommands | expand |
On Fri, Aug 19 2022, SZEDER Gábor wrote: > - int result; > const char *override_notes_ref = NULL; > + parse_opt_subcommand_fn *fn = list; > struct option options[] = { > OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"), > N_("use notes from <notes-ref>")), > + OPT_SUBCOMMAND("list", &fn, list), > + OPT_SUBCOMMAND("add", &fn, add), > + OPT_SUBCOMMAND("copy", &fn, copy), > + OPT_SUBCOMMAND("append", &fn, append_edit), > + OPT_SUBCOMMAND("edit", &fn, append_edit), > + OPT_SUBCOMMAND("show", &fn, show), > + OPT_SUBCOMMAND("merge", &fn, merge), > + OPT_SUBCOMMAND("remove", &fn, remove_cmd), > + OPT_SUBCOMMAND("prune", &fn, prune), > + OPT_SUBCOMMAND("get-ref", &fn, get_ref), > OPT_END() > }; > > git_config(git_default_config, NULL); > argc = parse_options(argc, argv, prefix, options, git_notes_usage, > - PARSE_OPT_STOP_AT_NON_OPTION); > + PARSE_OPT_SUBCOMMAND_OPTIONAL); > + if (fn == list && argc && strcmp(argv[0], "list")) { > + error(_("unknown subcommand: %s"), argv[0]); > + usage_with_options(git_notes_usage, options); > + } I wanted to ask why the API can't smartly handle this, but your "Found an unknown option given to a command with" comment in an earlier patch answered it. I think something in this direction would be a bit more readble/obvious, as it avoids hardcoding "list": diff --git a/builtin/notes.c b/builtin/notes.c index 42cbae46598..43d59b1a98e 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -995,7 +995,7 @@ static int get_ref(int argc, const char **argv, const char *prefix) int cmd_notes(int argc, const char **argv, const char *prefix) { const char *override_notes_ref = NULL; - parse_opt_subcommand_fn *fn = list; + parse_opt_subcommand_fn *fn = NULL; struct option options[] = { OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"), N_("use notes from <notes-ref>")), @@ -1015,10 +1015,11 @@ int cmd_notes(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, options, git_notes_usage, PARSE_OPT_SUBCOMMAND_OPTIONAL); - if (fn == list && argc && strcmp(argv[0], "list")) { - error(_("unknown subcommand: %s"), argv[0]); - usage_with_options(git_notes_usage, options); - } + if (!fn && argc) + usage_msg_optf(_("unknown subcommand: %s"), + git_notes_usage, options, argv[0]); + else if (!fn) + fn = list; if (override_notes_ref) { struct strbuf sb = STRBUF_INIT; I.e. we rely on the API setting it to non-NULL if it finds a subcommand, otherwise we just set it to "list" after checking whether we have excess arguments. > [...] > - else if (!strcmp(argv[0], "get-ref")) > - result = get_ref(argc, argv, prefix); > - else { > - result = error(_("unknown subcommand: %s"), argv[0]); > - usage_with_options(git_notes_usage, options); > - } > - > - return result ? 1 : 0; > + return !!fn(argc, argv, prefix); > } In any case this is a lot nicer, ditto previous comment about maybe skipping the refactoring of this end code, but I'm also fine with keeping it.
On Fri, Aug 19, 2022 at 08:01:55PM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Aug 19 2022, SZEDER Gábor wrote: > > > - int result; > > const char *override_notes_ref = NULL; > > + parse_opt_subcommand_fn *fn = list; > > struct option options[] = { > > OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"), > > N_("use notes from <notes-ref>")), > > + OPT_SUBCOMMAND("list", &fn, list), > > + OPT_SUBCOMMAND("add", &fn, add), > > + OPT_SUBCOMMAND("copy", &fn, copy), > > + OPT_SUBCOMMAND("append", &fn, append_edit), > > + OPT_SUBCOMMAND("edit", &fn, append_edit), > > + OPT_SUBCOMMAND("show", &fn, show), > > + OPT_SUBCOMMAND("merge", &fn, merge), > > + OPT_SUBCOMMAND("remove", &fn, remove_cmd), > > + OPT_SUBCOMMAND("prune", &fn, prune), > > + OPT_SUBCOMMAND("get-ref", &fn, get_ref), > > OPT_END() > > }; > > > > git_config(git_default_config, NULL); > > argc = parse_options(argc, argv, prefix, options, git_notes_usage, > > - PARSE_OPT_STOP_AT_NON_OPTION); > > + PARSE_OPT_SUBCOMMAND_OPTIONAL); > > + if (fn == list && argc && strcmp(argv[0], "list")) { > > + error(_("unknown subcommand: %s"), argv[0]); This should have been `%s' here, and in cmd_remote() as well. > > + usage_with_options(git_notes_usage, options); > > + } > > I wanted to ask why the API can't smartly handle this, but your "Found > an unknown option given to a command with" comment in an earlier patch > answered it. It's not about unknown options but rather about (non-option) arguments. 'git notes list' doesn't accept any --options, and since this 'list' is the default operation mode, parse_options() is invoked without the PARSE_OPT_KEEP_UNKNOWN_OPT flag, so 'git notes --foo' errors out even without any of the extra checks in the above hunk. However, while 'git notes list' does accept non-option arguments (objects or refs), 'git notes' does not. Alas, currently there is no way to tell parse_options() to error out upon finding a (non-option and non-subcommand) argument, it always keeps them in 'argv'; that's why we need these additional checks. Now, while we could add such a flag, of course, it would not be limited to this one particular use case, so when the error is triggered inside parse_options() I doubt that we could have this specific "unknown subcommand" error message. > I think something in this direction would be a bit more readble/obvious, > as it avoids hardcoding "list": > > diff --git a/builtin/notes.c b/builtin/notes.c > index 42cbae46598..43d59b1a98e 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -995,7 +995,7 @@ static int get_ref(int argc, const char **argv, const char *prefix) > int cmd_notes(int argc, const char **argv, const char *prefix) > { > const char *override_notes_ref = NULL; > - parse_opt_subcommand_fn *fn = list; > + parse_opt_subcommand_fn *fn = NULL; > struct option options[] = { > OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"), > N_("use notes from <notes-ref>")), > @@ -1015,10 +1015,11 @@ int cmd_notes(int argc, const char **argv, const char *prefix) > git_config(git_default_config, NULL); > argc = parse_options(argc, argv, prefix, options, git_notes_usage, > PARSE_OPT_SUBCOMMAND_OPTIONAL); > - if (fn == list && argc && strcmp(argv[0], "list")) { > - error(_("unknown subcommand: %s"), argv[0]); > - usage_with_options(git_notes_usage, options); > - } > + if (!fn && argc) > + usage_msg_optf(_("unknown subcommand: %s"), > + git_notes_usage, options, argv[0]); > + else if (!fn) > + fn = list; > > if (override_notes_ref) { > struct strbuf sb = STRBUF_INIT; > > I.e. we rely on the API setting it to non-NULL if it finds a subcommand, > otherwise we just set it to "list" after checking whether we have excess > arguments. Oh, that does look nicer indeed. > > [...] > > - else if (!strcmp(argv[0], "get-ref")) > > - result = get_ref(argc, argv, prefix); > > - else { > > - result = error(_("unknown subcommand: %s"), argv[0]); > > - usage_with_options(git_notes_usage, options); > > - } > > - > > - return result ? 1 : 0; > > + return !!fn(argc, argv, prefix); > > } > > In any case this is a lot nicer, ditto previous comment about maybe > skipping the refactoring of this end code, but I'm also fine with > keeping it.
diff --git a/builtin/notes.c b/builtin/notes.c index a3d0d15a22..42cbae4659 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -994,17 +994,31 @@ static int get_ref(int argc, const char **argv, const char *prefix) int cmd_notes(int argc, const char **argv, const char *prefix) { - int result; const char *override_notes_ref = NULL; + parse_opt_subcommand_fn *fn = list; struct option options[] = { OPT_STRING(0, "ref", &override_notes_ref, N_("notes-ref"), N_("use notes from <notes-ref>")), + OPT_SUBCOMMAND("list", &fn, list), + OPT_SUBCOMMAND("add", &fn, add), + OPT_SUBCOMMAND("copy", &fn, copy), + OPT_SUBCOMMAND("append", &fn, append_edit), + OPT_SUBCOMMAND("edit", &fn, append_edit), + OPT_SUBCOMMAND("show", &fn, show), + OPT_SUBCOMMAND("merge", &fn, merge), + OPT_SUBCOMMAND("remove", &fn, remove_cmd), + OPT_SUBCOMMAND("prune", &fn, prune), + OPT_SUBCOMMAND("get-ref", &fn, get_ref), OPT_END() }; git_config(git_default_config, NULL); argc = parse_options(argc, argv, prefix, options, git_notes_usage, - PARSE_OPT_STOP_AT_NON_OPTION); + PARSE_OPT_SUBCOMMAND_OPTIONAL); + if (fn == list && argc && strcmp(argv[0], "list")) { + error(_("unknown subcommand: %s"), argv[0]); + usage_with_options(git_notes_usage, options); + } if (override_notes_ref) { struct strbuf sb = STRBUF_INIT; @@ -1014,28 +1028,5 @@ int cmd_notes(int argc, const char **argv, const char *prefix) strbuf_release(&sb); } - if (argc < 1 || !strcmp(argv[0], "list")) - result = list(argc, argv, prefix); - else if (!strcmp(argv[0], "add")) - result = add(argc, argv, prefix); - else if (!strcmp(argv[0], "copy")) - result = copy(argc, argv, prefix); - else if (!strcmp(argv[0], "append") || !strcmp(argv[0], "edit")) - result = append_edit(argc, argv, prefix); - else if (!strcmp(argv[0], "show")) - result = show(argc, argv, prefix); - else if (!strcmp(argv[0], "merge")) - result = merge(argc, argv, prefix); - else if (!strcmp(argv[0], "remove")) - result = remove_cmd(argc, argv, prefix); - else if (!strcmp(argv[0], "prune")) - result = prune(argc, argv, prefix); - else if (!strcmp(argv[0], "get-ref")) - result = get_ref(argc, argv, prefix); - else { - result = error(_("unknown subcommand: %s"), argv[0]); - usage_with_options(git_notes_usage, options); - } - - return result ? 1 : 0; + return !!fn(argc, argv, prefix); }
'git notes' parses its subcommands with a long list of if-else if statements. parse-options has just learned to parse subcommands, so let's use that facility instead, with the benefits of shorter code, handling unknown subcommands, and listing subcommands for Bash completion. Make sure that the default operation mode doesn't accept any arguments. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- builtin/notes.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)