Re: [PATCH v2 09/20] parse-options: add support for parsing subcommands

On Fri, Aug 19 2022, SZEDER Gábor wrote:

+static enum parse_opt_result parse_subcommand(const char *arg,
+					      const struct option *options)
+{
+	for (; options->type != OPTION_END; options++)
+		if (options->type == OPTION_SUBCOMMAND &&
+		    !strcmp(options->long_name, arg)) {
+			*(parse_opt_subcommand_fn **)options->value = options->subcommand_fn;

Nit: Maybe do the cast by assigning to an intermediate variable? Would
make this less dense, and since we already have {}-braces...

Better split into two IMO:

	if (!opts->value)
		optbug(opts, "OPTION_SUBCOMMAND needs a value");
	if (!opts->subcommand_fn)
		optbug(opts, "OPTION_SUBCOMMAND needs a subcommand_fn");

Then if we have extra checks we don't need to keep adding to a dense ||
and amend an existing string.

+static int has_subcommands(const struct option *options)
+{
+	for (; options->type != OPTION_END; options++)
+		if (options->type == OPTION_SUBCOMMAND)
+			return 1;
+	return 0;
+}

I wondered if parse_options_check() couldn't take a "int
*has_subcommands", since we already loop through the options to check it
as part of the checks there (but we'd need to re-arrange them).

Not for optimization, just wondering if it would make the code flow
clearer, maybe not.

+	if (ctx->has_subcommands) {
+		if (flags & PARSE_OPT_STOP_AT_NON_OPTION)
+			BUG("subcommands are incompatible with PARSE_OPT_STOP_AT_NON_OPTION");
+		if (!(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) {
+			if (flags & PARSE_OPT_KEEP_UNKNOWN_OPT)
+				BUG("subcommands are incompatible with PARSE_OPT_KEEP_UNKNOWN_OPT unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL");
+			if (flags & PARSE_OPT_KEEP_DASHDASH)
+				BUG("subcommands are incompatible with PARSE_OPT_KEEP_DASHDASH unless in combination with PARSE_OPT_SUBCOMMAND_OPTIONAL");

MM, very long lines. Maybe something like:

	BUG("%s flag cannot be combined with %s, unless %s is also provided", ...);

Ah, here we have a prefix variable, but it's more of an "infix"... :)

+					return PARSE_OPT_DONE;
+				error(_("unknown subcommand: `%s'"), arg);
+				usage_with_options(usagestr, options);

ditto earlier patch comment about usage_msg_opt() (also applies below)

+			case PARSE_OPT_NON_OPTION:
+				/* Impossible. */

We could just skip this comment as...

+				BUG("parse_subcommand() cannot return these");

...this makes it clear.

 	case PARSE_OPT_DONE:
+		if (ctx.has_subcommands &&
+		    !(flags & PARSE_OPT_SUBCOMMAND_OPTIONAL)) {
+			error(_("need a subcommand"));
+			usage_with_options(usagestr, options);

ditto.

+typedef int parse_opt_subcommand_fn(int argc, const char **argv,
+				    const char *prefix);

We usually define function typedefs like:

	typedef int (*parse_opt_subcommand_fn)(...);

But I see that...

You're doing this consistently with parse_opt_ll_cb etc., fair enough.

+#define OPT_SUBCOMMAND_F(l, v, fn, f) { \
+	.type = OPTION_SUBCOMMAND, \
+	.long_name = (l), \
+	.value = (v), \
+	.flags = (f), \

Nice to see this form

+	.subcommand_fn = (fn) }

Almost all other such decls end with:

		.foo = bar, \
	}

I.e. we put the closing "}" at the start of the line.

+#define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)

I think this would be more readable as:

	#define OPT_SUBCOMMAND(l, v, fn) \
		OPT_SUBCOMMAND_F((l), (v), (fn), 0)

Which both aligns nicely, and isn't using a \t in the middle of the line
(which in any case we'll end up aligning with nothing else, as the names
are all different lengths, if other things continue using this pattern.

 	enum parse_opt_flags flags;
+	unsigned has_subcommands;

Maybe make it: "unsigned has_subcommands:1"?

+	printf("fn: subcmd_one\n");

s/printf/puts/g here

+	print_args(argc, argv);
+	return 0;
+}
+
+static int subcmd_two(int argc, const char **argv, const char *prefix)
+{
+	printf("fn: subcmd_two\n");

ditto.