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.