From 7a7d4c4613241be873b38c3a9d15e510161b93bb Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 2 Oct 2026 10:09:46 +0200 Subject: [PATCH 1/4] parse-options: fix completion format when first option is skipped The "--git-completion-helper" option can be passed to any command or subcommand that uses the parse-options interface. The output it generates is a space-separated list of subcommands or options understood by the command. The format is slightly broken though in the case where the first option is not being printed, like for example a group or a hidden option. In that case, `show_gitcomp()` will of course skip that first entry. But when printing the next option it checks for `opts == original_opts` to verify whether we're printing the first option. The check will evaluate to false though as we have skipped it, and thus we'll print a leading space even though we have printed nothing else yet. Fix that bug by tracking whether we have already printed anything via a local variable. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- parse-options.c | 4 +++- t/helper/test-parse-options.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index 4519ead9dc..8bb30ec116 100644 --- a/parse-options.c +++ b/parse-options.c @@ -845,6 +845,7 @@ static int show_gitcomp(const struct option *opts, int show_all) { const struct option *original_opts = opts; int nr_noopts = 0; + bool shown = false; for (; opts->type != OPTION_END; opts++) { const char *prefix = "--"; @@ -882,8 +883,9 @@ static int show_gitcomp(const struct option *opts, int show_all) suffix = "="; if (starts_with(opts->long_name, "no-")) nr_noopts++; - printf("%s%s%s%s", opts == original_opts ? "" : " ", + printf("%s%s%s%s", shown ? " " : "", prefix, opts->long_name, suffix); + shown = true; } show_negated_gitcomp(original_opts, show_all, -1); show_negated_gitcomp(original_opts, show_all, nr_noopts); diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index f181f0c02d..fbafd67756 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -351,6 +351,7 @@ static int parse_subcommand__cmd(int argc, const char **argv, parse_opt_subcommand_fn *fn = NULL; int opt = 0; struct option options[] = { + OPT_GROUP("Subcommands"), OPT_SUBCOMMAND("subcmd-one", &fn, subcmd_one), OPT_SUBCOMMAND("subcmd-two", &fn, subcmd_two), OPT_INTEGER('o', "opt", &opt, "an integer option"), From 1c4ee0aaa152e00e73549d6359db061c0a13cc77 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 2 Oct 2026 10:09:47 +0200 Subject: [PATCH 2/4] parse-options: extract functions to print single option The logic to print a single option has grown somewhat long. Extract the logic into two functions to print a single option and a flag, specifically. This refactoring makes a subsequent change easier to implement. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- parse-options.c | 169 +++++++++++++++++++++++++++--------------------- 1 file changed, 94 insertions(+), 75 deletions(-) diff --git a/parse-options.c b/parse-options.c index 8bb30ec116..fdcb29f2a1 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1323,6 +1323,97 @@ static const struct option *find_option_by_long_name(const struct option *opts, return NULL; } +static int usage_print_flag(const struct option *opt, + FILE *outfile, + const char **positive_name) +{ + int off = 0; + + if (opt->short_name) { + if (opt->flags & PARSE_OPT_NODASH) + off += fprintf(outfile, "%c", opt->short_name); + else + off += fprintf(outfile, "-%c", opt->short_name); + } + if (opt->long_name && opt->short_name) + off += fprintf(outfile, ", "); + if (opt->long_name) { + const char *long_name = opt->long_name; + if ((opt->flags & PARSE_OPT_NONEG) || + skip_prefix(long_name, "no-", positive_name)) + off += fprintf(outfile, "--%s", long_name); + else + off += fprintf(outfile, "--[no-]%s", long_name); + } + + if (opt->type == OPTION_NUMBER) + off += utf8_fprintf(outfile, _("-NUM")); + + if ((opt->flags & PARSE_OPT_LITERAL_ARGHELP) || + !(opt->flags & PARSE_OPT_NOARG)) + off += usage_argh(opt, outfile); + + return off; +} + +static void usage_print_option(const struct option *opt, + const struct option *all_opts, + int full, + int *need_newline, + FILE *outfile) +{ + const char *positive_name = NULL; + const char *cp, *np; + size_t pos; + + if (opt->type == OPTION_SUBCOMMAND) + return; + if (!full && (opt->flags & PARSE_OPT_HIDDEN)) + return; + if (opt->type == OPTION_GROUP) { + fputc('\n', outfile); + *need_newline = 0; + if (*opt->help) + fprintf(outfile, "%s\n", _(opt->help)); + return; + } + + if (*need_newline) { + fputc('\n', outfile); + *need_newline = 0; + } + + pos = usage_indent(outfile); + pos += usage_print_flag(opt, outfile, &positive_name); + + if (opt->type == OPTION_ALIAS) { + usage_padding(outfile, pos); + fprintf_ln(outfile, _("alias of --%s"), + (const char *)opt->value); + return; + } + + for (cp = opt->help ? _(opt->help) : ""; *cp; cp = np) { + np = strchrnul(cp, '\n'); + if (*np) + np++; + usage_padding(outfile, pos); + fwrite(cp, 1, np - cp, outfile); + pos = 0; + } + fputc('\n', outfile); + + if (positive_name) { + if (find_option_by_long_name(all_opts, positive_name)) + return; + pos = usage_indent(outfile); + pos += fprintf(outfile, "--%s", positive_name); + usage_padding(outfile, pos); + fprintf_ln(outfile, _("opposite of --no-%s"), + positive_name); + } +} + static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx, const char * const *usagestr, const struct option *opts, @@ -1408,81 +1499,9 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t } need_newline = 1; - - for (; opts->type != OPTION_END; opts++) { - size_t pos; - const char *cp, *np; - const char *positive_name = NULL; - - if (opts->type == OPTION_SUBCOMMAND) - continue; - if (!full && (opts->flags & PARSE_OPT_HIDDEN)) - continue; - if (opts->type == OPTION_GROUP) { - fputc('\n', outfile); - need_newline = 0; - if (*opts->help) - fprintf(outfile, "%s\n", _(opts->help)); - continue; - } - - if (need_newline) { - fputc('\n', outfile); - need_newline = 0; - } - - pos = usage_indent(outfile); - if (opts->short_name) { - if (opts->flags & PARSE_OPT_NODASH) - pos += fprintf(outfile, "%c", opts->short_name); - else - pos += fprintf(outfile, "-%c", opts->short_name); - } - if (opts->long_name && opts->short_name) - pos += fprintf(outfile, ", "); - if (opts->long_name) { - const char *long_name = opts->long_name; - if ((opts->flags & PARSE_OPT_NONEG) || - skip_prefix(long_name, "no-", &positive_name)) - pos += fprintf(outfile, "--%s", long_name); - else - pos += fprintf(outfile, "--[no-]%s", long_name); - } - - if (opts->type == OPTION_NUMBER) - pos += utf8_fprintf(outfile, _("-NUM")); - - if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) || - !(opts->flags & PARSE_OPT_NOARG)) - pos += usage_argh(opts, outfile); - - if (opts->type == OPTION_ALIAS) { - usage_padding(outfile, pos); - fprintf_ln(outfile, _("alias of --%s"), - (const char *)opts->value); - continue; - } - - for (cp = opts->help ? _(opts->help) : ""; *cp; cp = np) { - np = strchrnul(cp, '\n'); - if (*np) - np++; - usage_padding(outfile, pos); - fwrite(cp, 1, np - cp, outfile); - pos = 0; - } - fputc('\n', outfile); - - if (positive_name) { - if (find_option_by_long_name(all_opts, positive_name)) - continue; - pos = usage_indent(outfile); - pos += fprintf(outfile, "--%s", positive_name); - usage_padding(outfile, pos); - fprintf_ln(outfile, _("opposite of --no-%s"), - positive_name); - } - } + for (; opts->type != OPTION_END; opts++) + usage_print_option(opts, all_opts, full, + &need_newline, outfile); fputc('\n', outfile); if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) From 4a2e53930b7a765aa97e11eba9dcc131b1ae4afc Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 2 Oct 2026 10:09:48 +0200 Subject: [PATCH 3/4] parse-options: allow grouping subcommands The `OPT_GROUP()` macro can be used to create a new group. These groups can only be used to group options though, they do not have any effect when used in combination with subcommands. As our use of subcommands grows though it can be quite useful to group these, as well. Extend `OPT_SUBCOMMAND_F()` to take an optional help string. If given, such subcommands will be considered as part of `OPT_GROUP()` and printed with that help string. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/technical/api-parse-options.adoc | 4 +++- builtin/remote.c | 2 +- builtin/stash.c | 2 +- parse-options.c | 7 +++++-- parse-options.h | 5 +++-- t/helper/test-parse-options.c | 3 ++- t/t0040-parse-options.sh | 16 ++++++++++++++++ 7 files changed, 31 insertions(+), 8 deletions(-) diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc index 95b7924e84..6dfea34220 100644 --- a/Documentation/technical/api-parse-options.adoc +++ b/Documentation/technical/api-parse-options.adoc @@ -243,6 +243,7 @@ with `flags` set to `0`. Start an option group. `description` is a short string that describes the group or an empty string. Start the description with an upper-case letter. + Groups apply to options and subcommands that have a help string. `OPT_HIDDEN_GROUP(description)`:: Like `OPT_GROUP()`, but the group header carries @@ -362,7 +363,8 @@ with `flags` set to `0`. `OPT_SUBCOMMAND(long, &fn_ptr, subcommand_fn)`:: Define a subcommand. `subcommand_fn` is put into `fn_ptr` when - this subcommand is used. + this subcommand is used. The subcommand is not listed in the + usage output. The last element of the array must be `OPT_END()`. diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3ba..fb7e5b114f 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1940,7 +1940,7 @@ int cmd_remote(int argc, OPT__VERBOSE(&verbose, N_("be verbose; must be placed before a subcommand")), OPT_SUBCOMMAND("add", &fn, add), OPT_SUBCOMMAND("rename", &fn, mv), - OPT_SUBCOMMAND_F("rm", &fn, rm, PARSE_OPT_NOCOMPLETE), + OPT_SUBCOMMAND_F("rm", &fn, rm, NULL, PARSE_OPT_NOCOMPLETE), OPT_SUBCOMMAND("remove", &fn, rm), OPT_SUBCOMMAND("set-head", &fn, set_head), OPT_SUBCOMMAND("set-branches", &fn, set_branches), diff --git a/builtin/stash.c b/builtin/stash.c index 7a9843413b..8d606ee11d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -2475,7 +2475,7 @@ int cmd_stash(int argc, OPT_SUBCOMMAND("push", &fn, push_stash_unassumed), OPT_SUBCOMMAND("export", &fn, export_stash), OPT_SUBCOMMAND("import", &fn, import_stash), - OPT_SUBCOMMAND_F("save", &fn, save_stash, PARSE_OPT_NOCOMPLETE), + OPT_SUBCOMMAND_F("save", &fn, save_stash, NULL, PARSE_OPT_NOCOMPLETE), OPT_END() }; const char **args_copy; diff --git a/parse-options.c b/parse-options.c index fdcb29f2a1..2b59932d2c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1366,7 +1366,7 @@ static void usage_print_option(const struct option *opt, const char *cp, *np; size_t pos; - if (opt->type == OPTION_SUBCOMMAND) + if (opt->type == OPTION_SUBCOMMAND && !opt->help) return; if (!full && (opt->flags & PARSE_OPT_HIDDEN)) return; @@ -1384,7 +1384,10 @@ static void usage_print_option(const struct option *opt, } pos = usage_indent(outfile); - pos += usage_print_flag(opt, outfile, &positive_name); + if (opt->type == OPTION_SUBCOMMAND) + pos += fprintf(outfile, "%s", opt->long_name); + else + pos += usage_print_flag(opt, outfile, &positive_name); if (opt->type == OPTION_ALIAS) { usage_padding(outfile, pos); diff --git a/parse-options.h b/parse-options.h index d7f896a933..99c12c77cb 100644 --- a/parse-options.h +++ b/parse-options.h @@ -393,14 +393,15 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED; .value = (char *)(source_long_name), \ } -#define OPT_SUBCOMMAND_F(l, v, fn, f) { \ +#define OPT_SUBCOMMAND_F(l, v, fn, h, f) { \ .type = OPTION_SUBCOMMAND, \ .long_name = (l), \ .value = (v), \ + .help = (h), \ .flags = (f), \ .subcommand_fn = (fn), \ } -#define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) +#define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), NULL, 0) /* * parse_options() will filter out the processed options and leave the diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index fbafd67756..950db78673 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -352,8 +352,9 @@ static int parse_subcommand__cmd(int argc, const char **argv, int opt = 0; struct option options[] = { OPT_GROUP("Subcommands"), - OPT_SUBCOMMAND("subcmd-one", &fn, subcmd_one), + OPT_SUBCOMMAND_F("subcmd-one", &fn, subcmd_one, "the first subcommand", 0), OPT_SUBCOMMAND("subcmd-two", &fn, subcmd_two), + OPT_GROUP("Options"), OPT_INTEGER('o', "opt", &opt, "an integer option"), OPT_END() }; diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 449fff4d34..ec55bb1414 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -629,6 +629,22 @@ test_expect_success 'KEEP_UNKNOWN_OPT | NO_INTERNAL_HELP works' ' test_cmp expect actual ' +test_expect_success 'subcommand - usage lists subcommands with help text under their group' ' + test-tool parse-subcommand cmd -h >actual && + cat >expect <<-\EOF && + usage: <...> cmd subcmd-one + or: <...> cmd subcmd-two + + Subcommands + subcmd-one the first subcommand + + Options + -o, --[no-]opt an integer option + + EOF + test_cmp expect actual +' + test_expect_success 'subcommand - no subcommand shows error and usage' ' test_expect_code 129 test-tool parse-subcommand cmd 2>err && test_grep "^error: need a subcommand" err && From a69a982f48a172cf9f3382e0e502ff6e1a005419 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 2 Oct 2026 10:09:49 +0200 Subject: [PATCH 4/4] builtin/refs: introduce subcommand groups The git-refs(1) command nowadays has a bunch of different subcommands, which makes it hard to figure out what's what at a glance. Now that the parse-options subsystem supports grouping subcommands though we can do better. The commands roughly fall into the following categories: - Operations that span across the whole reference database. - Operations that read references. - Operations that write references. Introduce these groups accordingly, which results in the following help output: Reference database migrate migrate the reference database to a different format verify verify the consistency of the reference database optimize optimize the reference database Reading references list list references exists check whether a reference exists Writing references create create a new reference delete delete a reference update update an existing reference rename rename a reference Reorder the usage strings to match the new grouping. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- Documentation/git-refs.adoc | 2 +- builtin/refs.c | 32 ++++++++++++++++++++++---------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/Documentation/git-refs.adoc b/Documentation/git-refs.adoc index 9dc08cbca9..da7260c416 100644 --- a/Documentation/git-refs.adoc +++ b/Documentation/git-refs.adoc @@ -11,6 +11,7 @@ SYNOPSIS [synopsis] git refs migrate --ref-format= [--no-reflog] [--dry-run] git refs verify [--strict] [--verbose] +git refs optimize [--all] [--no-prune] [--auto] [--include ] [--exclude ] git refs list [--count=] [--shell|--perl|--python|--tcl] [(--sort=)...] [--format=] [--include-root-refs] [--points-at=] @@ -19,7 +20,6 @@ git refs list [--count=] [--shell|--perl|--python|--tcl] [(--exclude=)...] [--start-after=] [ --stdin | (...)] git refs exists -git refs optimize [--all] [--no-prune] [--auto] [--include ] [--exclude ] git refs create [--message=] [--no-deref] [--create-reflog] git refs delete [--message=] [--no-deref] [] git refs update [--message=] [--no-deref] [--create-reflog] [] diff --git a/builtin/refs.c b/builtin/refs.c index 5cd21c25fe..decccc4364 100644 --- a/builtin/refs.c +++ b/builtin/refs.c @@ -382,9 +382,9 @@ int cmd_refs(int argc, const char * const refs_usage[] = { REFS_MIGRATE_USAGE, REFS_VERIFY_USAGE, + REFS_OPTIMIZE_USAGE, "git refs list " COMMON_USAGE_FOR_EACH_REF, REFS_EXISTS_USAGE, - REFS_OPTIMIZE_USAGE, REFS_CREATE_USAGE, REFS_DELETE_USAGE, REFS_UPDATE_USAGE, @@ -393,15 +393,27 @@ int cmd_refs(int argc, }; parse_opt_subcommand_fn *fn = NULL; struct option opts[] = { - OPT_SUBCOMMAND("migrate", &fn, cmd_refs_migrate), - OPT_SUBCOMMAND("verify", &fn, cmd_refs_verify), - OPT_SUBCOMMAND("list", &fn, cmd_refs_list), - OPT_SUBCOMMAND("exists", &fn, cmd_refs_exists), - OPT_SUBCOMMAND("optimize", &fn, cmd_refs_optimize), - OPT_SUBCOMMAND("create", &fn, cmd_refs_create), - OPT_SUBCOMMAND("delete", &fn, cmd_refs_delete), - OPT_SUBCOMMAND("update", &fn, cmd_refs_update), - OPT_SUBCOMMAND("rename", &fn, cmd_refs_rename), + OPT_GROUP(N_("Reference database")), + OPT_SUBCOMMAND_F("migrate", &fn, cmd_refs_migrate, + N_("migrate the reference database to a different format"), 0), + OPT_SUBCOMMAND_F("verify", &fn, cmd_refs_verify, + N_("verify the consistency of the reference database"), 0), + OPT_SUBCOMMAND_F("optimize", &fn, cmd_refs_optimize, + N_("optimize the reference database"), 0), + OPT_GROUP(N_("Reading references")), + OPT_SUBCOMMAND_F("list", &fn, cmd_refs_list, + N_("list references"), 0), + OPT_SUBCOMMAND_F("exists", &fn, cmd_refs_exists, + N_("check whether a reference exists"), 0), + OPT_GROUP(N_("Writing references")), + OPT_SUBCOMMAND_F("create", &fn, cmd_refs_create, + N_("create a new reference"), 0), + OPT_SUBCOMMAND_F("delete", &fn, cmd_refs_delete, + N_("delete a reference"), 0), + OPT_SUBCOMMAND_F("update", &fn, cmd_refs_update, + N_("update an existing reference"), 0), + OPT_SUBCOMMAND_F("rename", &fn, cmd_refs_rename, + N_("rename a reference"), 0), OPT_END(), };