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.
