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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> On Mon, Jul 25, 2022 at 04:43:30PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > diff --git a/builtin/blame.c b/builtin/blame.c
>> > index 02e39420b6..a9fe8cf7a6 100644
>> > --- a/builtin/blame.c
>> > +++ b/builtin/blame.c
>> > @@ -920,6 +920,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>> >  			break;
>> >  		case PARSE_OPT_HELP:
>> >  		case PARSE_OPT_ERROR:
>> > +		case PARSE_OPT_SUBCOMMAND:
>> >  			exit(129);
>> >  		case PARSE_OPT_COMPLETE:
>> >  			exit(0);
>> > diff --git a/builtin/shortlog.c b/builtin/shortlog.c
>> > index 086dfee45a..7a1e1fe7c0 100644
>> > --- a/builtin/shortlog.c
>> > +++ b/builtin/shortlog.c
>> > @@ -381,6 +381,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
>> >  			break;
>> >  		case PARSE_OPT_HELP:
>> >  		case PARSE_OPT_ERROR:
>> > +		case PARSE_OPT_SUBCOMMAND:
>> >  			exit(129);
>> >  		case PARSE_OPT_COMPLETE:
>> >  			exit(0);
>> 
>> 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?
>
> I think it's as good as unreachable, because neither of these two
> commands have subcommands.  However, without these hunks the compiler
> invoked with '-Wswitch' (implied by '-Wall') does indeed complain.

Yeah, we should add a case, but let's just do:

	case PARSE_OPT_SUBCOMMAND:
		BUG("unreachable");

>> ...but why not...
>> 
>> >  static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
>> >  				  int argc, const char **argv, const char *prefix,
>> >  				  const struct option *options,
>> > @@ -515,6 +547,19 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
>> >  	ctx->prefix = prefix;
>> >  	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
>> >  	ctx->flags = flags;
>> > +	ctx->has_subcommands = has_subcommands(options);
>> > +	if (!ctx->has_subcommands && (flags & PARSE_OPT_SUBCOMMAND_OPTIONAL))
>> > +		BUG("Using PARSE_OPT_SUBCOMMAND_OPTIONAL without subcommands");
>> > +	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 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");
>> > +		}
>> > +	}
>> 
>> ...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?".
>
> The checks added in this hunk (and the existing checks in the hunk's
> after-context) are not about the elements of the 'struct option'
> array, like the checks in parse_options_check(), but rather about the
> sensibility of parse_options()'s 'parse_opt_flags' parameter.
> usage_with_options() doesn't have (and doesn't at all need) such a
> parameter.

Ah, sorry, I was just confused. FWIW it's because I split out *that*
part into another helper a while ago:
https://github.com/avar/git/commit/55dda82a409

Which might be worthhile doing/stealing heere while we're at it,
i.e. the flags checking has become quite ab ig part of
parse_options_start_1(), or just leave it for later...

>> > +				error(_("unknown subcommand: %s"), arg);
>> 
>> s/%s/'%s'/ while we're at it, perhaps?
>
> Indeed, though it should be `%s', because that's what surrounds
> unknown switches and options in the corresponding error messages.
>
>> > +				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?
>
> All these are impossible, so I don't think it matters.  This is
> another case of the compiler with '-Wswitch' complaining, and follows
> suit of similar switch statements after the calls to parse_short_opt()
> and parse_long_opt() functions.

*nod*, and this one's a BUG(), which is good...

>> > @@ -206,6 +217,11 @@ struct option {
>> >  #define OPT_ALIAS(s, l, source_long_name) \
>> >  	{ OPTION_ALIAS, (s), (l), (source_long_name) }
>> >  
>> > +#define OPT_SUBCOMMAND(l, v, fn)      { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \
>> > +					NULL, 0, NULL, 0, NULL, 0, (fn) }
>> > +#define OPT_SUBCOMMAND_F(l, v, fn, f) { OPTION_SUBCOMMAND, 0, (l), (v), NULL, \
>> > +					NULL, (f), NULL, 0, NULL, 0, (fn) }
>> 
>> 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...
>
> Oh, I love this idea!  But are we there yet?  I remember the weather
> balloon about designated initializers, but I'm not sure whether we've
> already made the decision to allow them.

Yes, we've got a thoroughly hard dependency on that part of C99 for a
while now, and it's OK to add new ones (especially in cases like these,
where it makes thigs easier to read).

> If we do, then I'm inclined
> to volunteer to clean up all those OPT_* macros in 'parse-options.h'
> with designated initializers, 

Sounds good, you might want to steal this & perhaps some things on the
same branch: https://github.com/avar/git/commit/a1a5e9c68c8

I didn't convert them to designated init, but some macros are "missing",
some are needlessy copy/pasted when X_F() could be defined in terms of
X() etc.

FWIW I thought it would eventually make sense to rename the members of
the struct itself, so we'd e.g. just have a "t" and "l" name, so we
could use that inline instead of the OPT_*() (we could use the long
names too, but that would probably be too verbose).

That would allow adding optional arguments, which e.g. would be handy
for things like "...and here's a list of what options this is
incompatible with".

>
>> > +{
>> > +	int i;
>> 
>> Nit: missing \n (usual style of variable decl);
>
> Hm, speaking of newer C features, what about 'for (int i = 0; ...)'?

Junio says this November, but see:
https://lore.kernel.org/git/220725.86zggxpfed.gmgdl@evledraar.gmail.com/

>> > +		error("'cmd' is mandatory");
>> > +		usage_with_options(usage, test_flag_options);
>> 
>> nit: I think you want usage_msg_optf() or usage_msg_opt().
>
> Maybe... but I don't know what they do ;)  Though I remember removing
> a couple of similar error() and usage_with_options() pairs from the
> builtin commands.

It's just helpers for "usage_with_options, except with a message, e.g.:

	$ ./git cat-file a b c
	fatal: only two arguments allowed in <type> <object> mode, not 3

	usage: git cat-file <type> <object>