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

On Mon, Jul 25 2022, SZEDER Gábor wrote:

Several Git commands have subcommands to implement mutually exclusive
"operation modes", and they usually parse their subcommand argument
with a bunch of if-else if statements.

I'll need do look this over in more details, just some comments on the
non-meaty parts for now:

This feels a bit like carrying forward an API wart, i.e. shouldn't we
instead BUG() if we are returning a PARSE_OPT_SUBCOMMAND from
parse_options_step() for options lists that don't have
PARSE_OPT_SUBCOMMAND in them?

I.e. is this even reachable, or just something to suppress the compiler
complaining about missing enum labels?

This addition looks good...

...but why not...

...move this into parse_options_check()? I.e. we'd need to loop over the
list once, but it seems like this should belong there.

We have an existing bug in-tree due to usage_with_options() not doing a
parse_options_check() (I have a local fix...), checking this sort of
thing there instead of in parse_options_start() is therefore the right
thing to do, i.e. we shoudl have a one-stop "does this options variable
look sane?".

+				error(_("unknown subcommand: %s"), arg);

s/%s/'%s'/ while we're at it, perhaps?

+				usage_with_options(usagestr, options);
+			case PARSE_OPT_COMPLETE:
+			case PARSE_OPT_HELP:
+			case PARSE_OPT_ERROR:
+			case PARSE_OPT_DONE:
+			case PARSE_OPT_NON_OPTION:
+				BUG("parse_subcommand() cannot return these");

nit: BUG("got bad %d", v) or whatever, i.e. say what we got?

Nit, I know you're carrying forward existing patterns, but since that
all pre-dated designated init perhaps we could just (untested):
	
	#define OPT_SUBCOMMAND_F(l, v, fn, f) { \
		.type = OPTION_SUBCOMMAND, \
		.long_name = (l), \
		.value = (v), \
		.ll_callback = (fn), \
	}
	#define OPT_SUBCOMMAND(l, v, fn) OPT_SUBCOMMAND_F((l), (v), (fn), 0)

Which IMO is much nicer. I have some patches somewhere to convert these
to saner patterns (I think not designated init, but the X() can be
defined in terms of X_F() like that, but since this is new we can use
designated init all the way...

+{
+	int i;

Nit: missing \n (usual style of variable decl);

+		error("'cmd' is mandatory");
+		usage_with_options(usage, test_flag_options);

nit: I think you want usage_msg_optf() or usage_msg_opt().

+test_expect_success 'subcommand - no subcommand shows error and usage' '
+	test_expect_code 129 test-tool parse-subcommand cmd 2>err &&
+	grep "^error: need a subcommand" err &&
+	grep ^usage: err
+'
+
+test_expect_success 'subcommand - subcommand after -- shows error and usage' '
+	test_expect_code 129 test-tool parse-subcommand cmd -- subcmd-one 2>err &&
+	grep "^error: need a subcommand" err &&
+	grep ^usage: err
+'
+
+test_expect_success 'subcommand - subcommand after --end-of-options shows error and usage' '
+	test_expect_code 129 test-tool parse-subcommand cmd --end-of-options subcmd-one 2>err &&
+	grep "^error: need a subcommand" err &&
+	grep ^usage: err
+'
+
+test_expect_success 'subcommand - unknown subcommand shows error and usage' '
+	test_expect_code 129 test-tool parse-subcommand cmd nope 2>err &&
+	grep "^error: unknown subcommand: nope" err &&
+	grep ^usage: err
+'
+
+test_expect_success 'subcommand - subcommands cannot be abbreviated' '
+	test_expect_code 129 test-tool parse-subcommand cmd subcmd-o 2>err &&
+	grep "^error: unknown subcommand: subcmd-o$" err &&
+	grep ^usage: err
+'
+
+test_expect_success 'subcommand - no negated subcommands' '
+	test_expect_code 129 test-tool parse-subcommand cmd no-subcmd-one 2>err &&
+	grep "^error: unknown subcommand: no-subcmd-one" err &&
+	grep ^usage: err
+'

Creating a trivial helper for this seems worthile, then something like:

	that_helper "expected error here" -- arg u ments to test-tool parse-subcommand

+test_expect_success 'subcommand - simple' '
+	test-tool parse-subcommand cmd subcmd-two >actual &&
+	cat >expect <<-\EOF &&
+	opt: 0
+	fn: subcmd_two
+	arg 00: subcmd-two
+	EOF
+	test_cmp expect actual
+'

Ditto, perhaps? I.e.

	new_hepler arg u ments <<-\EOF
        expected
        goes here
       	EOF