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@xxxxxxxxxxxxxxxxxxx/
>> > + 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>
