Re: [PATCH 2/3] parse-options: allow grouping subcommands

[Patrick Steinhardt <ps@pks.im>]

On Thu, Oct 01, 2026 at 10:46:24AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -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);
> > -		}
> 
> It may have made it easier to follow if a preliminary step pushed
> the above to a helper function.  It would have also prevented the
> nesting becoming too deep as we see below.

Will do.

> > 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), \
> > +}
> 
> As presented, _F does not allow you to give it a help, and _H does
> not allow you to give it a flag word.  I would have preferred to see
> OPT_SUBCOMMAND_F to be extended to also take the help text, as we
> only have two existing users in *.c code, rather than adding _H
> variant that is incomplete and keeping _F incomplete.

I was a bit torn here because I honestly wasn't quite sure whether the
_F suffix stands for "full" or "flag". But okay, let's not introduce a
new macro then.

Patrick