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

On Mon, Jul 25, 2022 at 04:43:30PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

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");

quoted

...but why not...

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+{
+	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/ (local)

quoted

quoted

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