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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.
 
> ...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.

> > +				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.

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

> > +{
> > +	int i;
> 
> Nit: missing \n (usual style of variable decl);

Hm, speaking of newer C features, what about 'for (int i = 0; ...)'?

> > +		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.