# [PATCH 1/3] parse-options: fix completion format when first option is skipped 14 messages from 2026-10-01 to 2026-10-02. Participants: Patrick Steinhardt, Junio C Hamano. Thread: https://gitlist.dev/t/66434 ## Patrick Steinhardt, 2026-10-01 10:13 Subject: [PATCH 0/3] builtin/refs: introduce subcommand groups Message-ID: <20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im> ``` Hi, the git-refs(1) command has grown quite a bunch of different subcommands by now. These subcommands can easily be grouped into three categories: - Operations that span across the whole reference database (migrate, verify, optimize). - Operations that read references (list, exists). - Operations that write references (create, delete, update, rename). This patch series thus adapts the parse-options subsystem to support grouping subcommands and then introduces the grouping for git-refs(1). This results in the following output: usage: git refs migrate --ref-format= [--no-reflog] [--dry-run] or: git refs verify [--strict] [--verbose] or: git refs list [--count=] [--shell|--perl|--python|--tcl] [(--sort=)...] [--format=] [--include-root-refs] [--points-at=] [--merged[=]] [--no-merged[=]] [--contains[=]] [--no-contains[=]] [(--exclude=)...] [--start-after=] [ --stdin | (...)] or: git refs exists or: git refs optimize [--all] [--no-prune] [--auto] [--include ] [--exclude ] or: git refs create [--message=] [--no-deref] [--create-reflog] or: git refs delete [--message=] [--no-deref] [] or: git refs update [--message=] [--no-deref] [--create-reflog] [] or: git refs rename [--message=] 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 I expect that going forward, we'll probably have more use cases where we can use these new capabilities (e.g. an upcoming git-objects(1) command, which is going to be the equivalent to git-refs(1)). Thanks! Patrick --- Patrick Steinhardt (3): parse-options: fix completion format when first option is skipped parse-options: allow grouping subcommands builtin/refs: introduce subcommand groups Documentation/git-refs.adoc | 2 +- Documentation/technical/api-parse-options.adoc | 10 ++++- builtin/refs.c | 32 ++++++++----- parse-options.c | 62 ++++++++++++++------------ parse-options.h | 7 +++ t/helper/test-parse-options.c | 4 +- t/t0040-parse-options.sh | 16 +++++++ 7 files changed, 92 insertions(+), 41 deletions(-) --- base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 change-id: 20261001-b4-pks-parse-options-subcommand-groups-59b3f27da06c ``` ## Patrick Steinhardt, 2026-10-01 10:13 Subject: [PATCH 1/3] parse-options: fix completion format when first option is skipped Message-ID: <20261001-b4-pks-parse-options-subcommand-groups-v1-1-01eb2f4a4c32@pks.im> In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im> ``` 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 --- 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..356eeff016 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 first = true; 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", first ? "" : " ", prefix, opts->long_name, suffix); + first = false; } 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"), -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-01 10:13 Subject: [PATCH 2/3] parse-options: allow grouping subcommands Message-ID: <20261001-b4-pks-parse-options-subcommand-groups-v1-2-01eb2f4a4c32@pks.im> In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im> ``` 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 the parse-options interfaces to support this use case: the new `OPT_SUBCOMMAND_H()` macro can be used to specify a subcommand that has a description attached to it, and subcommands like these are now being considered for `OPT_GROUP()`. Signed-off-by: Patrick Steinhardt --- Documentation/technical/api-parse-options.adoc | 10 ++++- parse-options.c | 58 ++++++++++++++------------ parse-options.h | 7 ++++ t/helper/test-parse-options.c | 3 +- t/t0040-parse-options.sh | 16 +++++++ 5 files changed, 65 insertions(+), 29 deletions(-) diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc index 95b7924e84..38dff82f72 100644 --- a/Documentation/technical/api-parse-options.adoc +++ b/Documentation/technical/api-parse-options.adoc @@ -243,6 +243,8 @@ 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 defined with + `OPT_SUBCOMMAND_H()`. `OPT_HIDDEN_GROUP(description)`:: Like `OPT_GROUP()`, but the group header carries @@ -362,7 +364,13 @@ 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. + +`OPT_SUBCOMMAND_H(long, &fn_ptr, subcommand_fn, description)`:: + Like `OPT_SUBCOMMAND()`, but the subcommand is listed in the usage + output together with its `description`. This can be used together with + `OPT_GROUP()` to group together subcommands. The last element of the array must be `OPT_END()`. diff --git a/parse-options.c b/parse-options.c index 356eeff016..75f14b9767 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1414,7 +1414,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t const char *cp, *np; const char *positive_name = NULL; - if (opts->type == OPTION_SUBCOMMAND) + if (opts->type == OPTION_SUBCOMMAND && !opts->help) continue; if (!full && (opts->flags & PARSE_OPT_HIDDEN)) continue; @@ -1432,35 +1432,39 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t } 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_SUBCOMMAND) { + pos += fprintf(outfile, "%s", opts->long_name); + } else { + 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->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->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; + 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) { diff --git a/parse-options.h b/parse-options.h index d7f896a933..5249404b46 100644 --- a/parse-options.h +++ b/parse-options.h @@ -401,6 +401,13 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED; .subcommand_fn = (fn), \ } #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) +#define OPT_SUBCOMMAND_H(l, v, fn, h) { \ + .type = OPTION_SUBCOMMAND, \ + .long_name = (l), \ + .value = (v), \ + .help = (h), \ + .subcommand_fn = (fn), \ +} /* * 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..4a146bd016 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_H("subcmd-one", &fn, subcmd_one, "the first subcommand"), 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 && -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-01 10:13 Subject: [PATCH 3/3] builtin/refs: introduce subcommand groups Message-ID: <20261001-b4-pks-parse-options-subcommand-groups-v1-3-01eb2f4a4c32@pks.im> In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im> ``` 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 --- 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..f46abd6268 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_H("migrate", &fn, cmd_refs_migrate, + N_("migrate the reference database to a different format")), + OPT_SUBCOMMAND_H("verify", &fn, cmd_refs_verify, + N_("verify the consistency of the reference database")), + OPT_SUBCOMMAND_H("optimize", &fn, cmd_refs_optimize, + N_("optimize the reference database")), + OPT_GROUP(N_("Reading references")), + OPT_SUBCOMMAND_H("list", &fn, cmd_refs_list, + N_("list references")), + OPT_SUBCOMMAND_H("exists", &fn, cmd_refs_exists, + N_("check whether a reference exists")), + OPT_GROUP(N_("Writing references")), + OPT_SUBCOMMAND_H("create", &fn, cmd_refs_create, + N_("create a new reference")), + OPT_SUBCOMMAND_H("delete", &fn, cmd_refs_delete, + N_("delete a reference")), + OPT_SUBCOMMAND_H("update", &fn, cmd_refs_update, + N_("update an existing reference")), + OPT_SUBCOMMAND_H("rename", &fn, cmd_refs_rename, + N_("rename a reference")), OPT_END(), }; -- 2.56.0.353.g0856645cf6.dirty ``` ## Junio C Hamano, 2026-10-01 17:38 Subject: Re: [PATCH 1/3] parse-options: fix completion format when first option is skipped Message-ID: In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-1-01eb2f4a4c32@pks.im> ``` Patrick Steinhardt writes: > 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. Very clearly articulated. I would have chosen 'shown' as the variable name to so do, but 'first' may also be OK. > > Signed-off-by: Patrick Steinhardt > --- > 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..356eeff016 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 first = true; > > 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", first ? "" : " ", > prefix, opts->long_name, suffix); > + first = false; > } > 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"), ``` ## Junio C Hamano, 2026-10-01 17:46 Subject: Re: [PATCH 2/3] parse-options: allow grouping subcommands Message-ID: In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-2-01eb2f4a4c32@pks.im> ``` Patrick Steinhardt writes: > @@ -1432,35 +1432,39 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t > } > > 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); > - } It may have made it easier to follow if a preliminary step pushed the above to a helper function. It would have also prevented the nesting becoming too deep as we see below. > + if (opts->type == OPTION_SUBCOMMAND) { > + pos += fprintf(outfile, "%s", opts->long_name); > + } else { > + 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 > diff --git a/parse-options.h b/parse-options.h > index d7f896a933..5249404b46 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -401,6 +401,13 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED; > .subcommand_fn = (fn), \ > } > #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) > +#define OPT_SUBCOMMAND_H(l, v, fn, h) { \ > + .type = OPTION_SUBCOMMAND, \ > + .long_name = (l), \ > + .value = (v), \ > + .help = (h), \ > + .subcommand_fn = (fn), \ > +} As presented, _F does not allow you to give it a help, and _H does not allow you to give it a flag word. I would have preferred to see OPT_SUBCOMMAND_F to be extended to also take the help text, as we only have two existing users in *.c code, rather than adding _H variant that is incomplete and keeping _F incomplete. ``` ## Junio C Hamano, 2026-10-01 17:46 Subject: Re: [PATCH 3/3] builtin/refs: introduce subcommand groups Message-ID: In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-3-01eb2f4a4c32@pks.im> ``` Patrick Steinhardt writes: > 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 Nice. ``` ## Patrick Steinhardt, 2026-10-02 07:19 Subject: Re: [PATCH 1/3] parse-options: fix completion format when first option is skipped Message-ID: In-Reply-To: ``` On Thu, Oct 01, 2026 at 10:38:08AM -0700, Junio C Hamano wrote: > Patrick Steinhardt writes: > > > 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. > > Very clearly articulated. I would have chosen 'shown' as the > variable name to so do, but 'first' may also be OK. That's a fair point. We explicitly _don't_ care whether it's the first entry or not, as that would match the old logic that caused this bug in the first place. So `shown` is a better name indeed. Patrick ``` ## Patrick Steinhardt, 2026-10-02 07:19 Subject: Re: [PATCH 2/3] parse-options: allow grouping subcommands Message-ID: In-Reply-To: ``` On Thu, Oct 01, 2026 at 10:46:24AM -0700, Junio C Hamano wrote: > Patrick Steinhardt writes: > > > @@ -1432,35 +1432,39 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t > > } > > > > 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); > > - } > > It may have made it easier to follow if a preliminary step pushed > the above to a helper function. It would have also prevented the > nesting becoming too deep as we see below. Will do. > > diff --git a/parse-options.h b/parse-options.h > > index d7f896a933..5249404b46 100644 > > --- a/parse-options.h > > +++ b/parse-options.h > > @@ -401,6 +401,13 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED; > > .subcommand_fn = (fn), \ > > } > > #define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0) > > +#define OPT_SUBCOMMAND_H(l, v, fn, h) { \ > > + .type = OPTION_SUBCOMMAND, \ > > + .long_name = (l), \ > > + .value = (v), \ > > + .help = (h), \ > > + .subcommand_fn = (fn), \ > > +} > > As presented, _F does not allow you to give it a help, and _H does > not allow you to give it a flag word. I would have preferred to see > OPT_SUBCOMMAND_F to be extended to also take the help text, as we > only have two existing users in *.c code, rather than adding _H > variant that is incomplete and keeping _F incomplete. I was a bit torn here because I honestly wasn't quite sure whether the _F suffix stands for "full" or "flag". But okay, let's not introduce a new macro then. Patrick ``` ## Patrick Steinhardt, 2026-10-02 08:09 Subject: [PATCH v2 0/4] builtin/refs: introduce subcommand groups Message-ID: <20261002-b4-pks-parse-options-subcommand-groups-v2-0-3299bee52dea@pks.im> In-Reply-To: <20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im> ``` Hi, the git-refs(1) command has grown quite a bunch of different subcommands by now. These subcommands can easily be grouped into three categories: - Operations that span across the whole reference database (migrate, verify, optimize). - Operations that read references (list, exists). - Operations that write references (create, delete, update, rename). This patch series thus adapts the parse-options subsystem to support grouping subcommands and then introduces the grouping for git-refs(1). This results in the following output: usage: git refs migrate --ref-format= [--no-reflog] [--dry-run] or: git refs verify [--strict] [--verbose] or: git refs list [--count=] [--shell|--perl|--python|--tcl] [(--sort=)...] [--format=] [--include-root-refs] [--points-at=] [--merged[=]] [--no-merged[=]] [--contains[=]] [--no-contains[=]] [(--exclude=)...] [--start-after=] [ --stdin | (...)] or: git refs exists or: git refs optimize [--all] [--no-prune] [--auto] [--include ] [--exclude ] or: git refs create [--message=] [--no-deref] [--create-reflog] or: git refs delete [--message=] [--no-deref] [] or: git refs update [--message=] [--no-deref] [--create-reflog] [] or: git refs rename [--message=] 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 I expect that going forward, we'll probably have more use cases where we can use these new capabilities (e.g. an upcoming git-objects(1) command, which is going to be the equivalent to git-refs(1)). Changes in v2: - Add a preliminary refactoring for `usage_with_options_internal()` so that we don't have to reindent a bunch of its code. - Drop `OPT_SUBCOMMAND_H()` and extend `OPT_SUBCOMMAND_F()` instead. - Rename `bool first` to `bool shown`. - Link to v1: https://patch.msgid.link/20261001-b4-pks-parse-options-subcommand-groups-v1-0-01eb2f4a4c32@pks.im Thanks! Patrick --- Patrick Steinhardt (4): parse-options: fix completion format when first option is skipped parse-options: extract functions to print single option parse-options: allow grouping subcommands builtin/refs: introduce subcommand groups Documentation/git-refs.adoc | 2 +- Documentation/technical/api-parse-options.adoc | 4 +- builtin/refs.c | 32 +++-- builtin/remote.c | 2 +- builtin/stash.c | 2 +- parse-options.c | 176 ++++++++++++++----------- parse-options.h | 5 +- t/helper/test-parse-options.c | 4 +- t/t0040-parse-options.sh | 16 +++ 9 files changed, 150 insertions(+), 93 deletions(-) Range-diff versus v1: 1: 7ba8a5ca15 ! 1: 10b2249fa6 parse-options: fix completion format when first option is skipped @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all { const struct option *original_opts = opts; int nr_noopts = 0; -+ bool first = true; ++ bool shown = false; for (; opts->type != OPTION_END; opts++) { const char *prefix = "--"; @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all if (starts_with(opts->long_name, "no-")) nr_noopts++; - printf("%s%s%s%s", opts == original_opts ? "" : " ", -+ printf("%s%s%s%s", first ? "" : " ", ++ printf("%s%s%s%s", shown ? " " : "", prefix, opts->long_name, suffix); -+ first = false; ++ shown = true; } show_negated_gitcomp(original_opts, show_all, -1); show_negated_gitcomp(original_opts, show_all, nr_noopts); 2: 8176cf838d < -: ---------- parse-options: allow grouping subcommands -: ---------- > 2: 31e5f08cdb parse-options: extract functions to print single option -: ---------- > 3: 20da51f904 parse-options: allow grouping subcommands 3: 9599245895 ! 4: 11cfbe5cae builtin/refs: introduce subcommand groups @@ builtin/refs.c: int cmd_refs(int argc, - OPT_SUBCOMMAND("update", &fn, cmd_refs_update), - OPT_SUBCOMMAND("rename", &fn, cmd_refs_rename), + OPT_GROUP(N_("Reference database")), -+ OPT_SUBCOMMAND_H("migrate", &fn, cmd_refs_migrate, -+ N_("migrate the reference database to a different format")), -+ OPT_SUBCOMMAND_H("verify", &fn, cmd_refs_verify, -+ N_("verify the consistency of the reference database")), -+ OPT_SUBCOMMAND_H("optimize", &fn, cmd_refs_optimize, -+ N_("optimize the 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_H("list", &fn, cmd_refs_list, -+ N_("list references")), -+ OPT_SUBCOMMAND_H("exists", &fn, cmd_refs_exists, -+ N_("check whether a reference exists")), ++ 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_H("create", &fn, cmd_refs_create, -+ N_("create a new reference")), -+ OPT_SUBCOMMAND_H("delete", &fn, cmd_refs_delete, -+ N_("delete a reference")), -+ OPT_SUBCOMMAND_H("update", &fn, cmd_refs_update, -+ N_("update an existing reference")), -+ OPT_SUBCOMMAND_H("rename", &fn, cmd_refs_rename, -+ N_("rename a reference")), ++ 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(), }; --- base-commit: a018953688f1b10bddf91bff8747068f5f4746a4 change-id: 20261001-b4-pks-parse-options-subcommand-groups-59b3f27da06c ``` ## Patrick Steinhardt, 2026-10-02 08:09 Subject: [PATCH v2 1/4] parse-options: fix completion format when first option is skipped Message-ID: <20261002-b4-pks-parse-options-subcommand-groups-v2-1-3299bee52dea@pks.im> In-Reply-To: <20261002-b4-pks-parse-options-subcommand-groups-v2-0-3299bee52dea@pks.im> ``` 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 --- 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"), -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-02 08:09 Subject: [PATCH v2 2/4] parse-options: extract functions to print single option Message-ID: <20261002-b4-pks-parse-options-subcommand-groups-v2-2-3299bee52dea@pks.im> In-Reply-To: <20261002-b4-pks-parse-options-subcommand-groups-v2-0-3299bee52dea@pks.im> ``` 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 --- 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) -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-02 08:09 Subject: [PATCH v2 3/4] parse-options: allow grouping subcommands Message-ID: <20261002-b4-pks-parse-options-subcommand-groups-v2-3-3299bee52dea@pks.im> In-Reply-To: <20261002-b4-pks-parse-options-subcommand-groups-v2-0-3299bee52dea@pks.im> ``` 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 --- 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 && -- 2.56.0.353.g0856645cf6.dirty ``` ## Patrick Steinhardt, 2026-10-02 08:09 Subject: [PATCH v2 4/4] builtin/refs: introduce subcommand groups Message-ID: <20261002-b4-pks-parse-options-subcommand-groups-v2-4-3299bee52dea@pks.im> In-Reply-To: <20261002-b4-pks-parse-options-subcommand-groups-v2-0-3299bee52dea@pks.im> ``` 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 --- 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(), }; -- 2.56.0.353.g0856645cf6.dirty ```