Re: [PATCH v2 15/20] builtin/notes.c: let parse-options parse subcommands

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.