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.
