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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.