[PATCH v2 3/4] parse-options: allow grouping subcommands

[Patrick Steinhardt <ps@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 <ps@pks.im>
---
 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 <n>    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