[PATCH 2/3] 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 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 <ps@pks.im>
---
 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 <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