git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jul 25, 2022, 19:41 UTC
Message-ID
<220725.86wnc1nf6c.gmgdl@evledraar.gmail.com>
In-Reply-To
<20220725192947.GB2543@szeder.dev>
On Mon, Jul 25 2022, SZEDER Gábor wrote:
Show 37 quoted lines
> 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");
Show 38 quoted lines
>> ...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...

Show 21 quoted lines
>> > +				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...
Show 28 quoted lines
>> > @@ -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".

Show 7 quoted lines
>
>> > +{
>> > +	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/

Show 8 quoted lines
>> > +		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>
Previous: SZEDER GáborNext: SZEDER Gábor
Message 27 of 97 in “parse-options: handle subcommands”
  1. 00/20 parse-options: handle subcommandsSZEDER Gábor, Jul 25, 2022
  2. 01/20 git.c: update NO_PARSEOPT markingsSZEDER Gábor, Jul 25, 2022
  3. Ævar Arnfjörð BjarmasonJul 25, 2022
  4. SZEDER GáborAug 2, 2022
  5. Junio C HamanoAug 2, 2022
  6. Ævar Arnfjörð BjarmasonAug 3, 2022
  7. SZEDER GáborAug 3, 2022
  8. Ævar Arnfjörð BjarmasonAug 4, 2022
  9. Junio C HamanoAug 11, 2022
  10. SZEDER GáborAug 12, 2022
  11. Junio C HamanoAug 12, 2022
  12. SZEDER GáborJul 26, 2022
  13. 02/20 t3301-notes.sh: check that default operation mode doesn't take argumentsSZEDER Gábor, Jul 25, 2022
  14. 03/20 t5505-remote.sh: check the behavior without a subcommandSZEDER Gábor, Jul 25, 2022
  15. Ævar Arnfjörð BjarmasonJul 25, 2022
  16. 04/20 t0040-parse-options: test parse_options() with various 'parse_opt_flags'SZEDER Gábor, Jul 25, 2022
  17. Ævar Arnfjörð BjarmasonJul 25, 2022
  18. SZEDER GáborAug 12, 2022
  19. 05/20 api-parse-options.txt: fix description of OPT_CMDMODESZEDER Gábor, Jul 25, 2022
  20. 06/20 parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --optionsSZEDER Gábor, Jul 25, 2022
  21. 07/20 parse-options: clarify the limitations of PARSE_OPT_NODASHSZEDER Gábor, Jul 25, 2022
  22. 08/20 parse-options: drop leading space from '--git-completion-helper' outputSZEDER Gábor, Jul 25, 2022
  23. 10/20 builtin/bundle.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  24. 09/20 parse-options: add support for parsing subcommandsSZEDER Gábor, Jul 25, 2022
  25. Ævar Arnfjörð BjarmasonJul 25, 2022
  26. SZEDER GáborJul 25, 2022
  27. Ævar Arnfjörð BjarmasonJul 25, 2022
  28. SZEDER GáborJul 25, 2022
  29. SZEDER GáborAug 12, 2022
  30. Junio C HamanoJul 25, 2022
  31. 11/20 builtin/commit-graph.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  32. 12/20 builtin/gc.c: let parse-options parse 'git maintenance's subcommandsSZEDER Gábor, Jul 25, 2022
  33. 13/20 builtin/hook.c: let parse-option parse subcommandsSZEDER Gábor, Jul 25, 2022
  34. 15/20 builtin/notes.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  35. Junio C HamanoJul 25, 2022
  36. 14/20 builtin/multi-pack-index.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  37. 16/20 builtin/reflog.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  38. 17/20 builtin/remote.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  39. 18/20 builtin/sparse-checkout.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  40. 19/20 builtin/stash.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  41. 20/20 builtin/worktree.c: let parse-options parse subcommandsSZEDER Gábor, Jul 25, 2022
  42. Derrick StoleeJul 25, 2022
  43. SZEDER GáborJul 25, 2022
  44. Derrick StoleeJul 25, 2022
  45. Ævar Arnfjörð BjarmasonJul 25, 2022
  46. Junio C HamanoJul 25, 2022
  47. Johannes SchindelinJul 26, 2022
  48. Ævar Arnfjörð BjarmasonJul 26, 2022
  49. 00/20 parse-options: handle subcommandsSZEDER Gábor, Aug 19, 2022
  50. 09/20 parse-options: add support for parsing subcommandsSZEDER Gábor, Aug 19, 2022
  51. Ævar Arnfjörð BjarmasonAug 19, 2022
  52. Ævar Arnfjörð BjarmasonAug 19, 2022
  53. 15/20 builtin/notes.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  54. Ævar Arnfjörð BjarmasonAug 19, 2022
  55. SZEDER GáborAug 21, 2022
  56. 17/20 builtin/remote.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  57. 02/20 t3301-notes.sh: check that default operation mode doesn't take argumentsSZEDER Gábor, Aug 19, 2022
  58. 13/20 builtin/hook.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  59. 10/20 builtin/bundle.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  60. Ævar Arnfjörð BjarmasonAug 19, 2022
  61. 12/20 builtin/gc.c: let parse-options parse 'git maintenance's subcommandsSZEDER Gábor, Aug 19, 2022
  62. Junio C HamanoAug 19, 2022
  63. 16/20 builtin/reflog.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  64. Ævar Arnfjörð BjarmasonAug 19, 2022
  65. 11/20 builtin/commit-graph.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  66. Ævar Arnfjörð BjarmasonAug 19, 2022
  67. Ævar Arnfjörð BjarmasonAug 19, 2022
  68. SZEDER GáborAug 19, 2022
  69. 18/20 builtin/sparse-checkout.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  70. 01/20 git.c: update NO_PARSEOPT markingsSZEDER Gábor, Aug 19, 2022
  71. 14/20 builtin/multi-pack-index.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  72. Ævar Arnfjörð BjarmasonAug 19, 2022
  73. 04/20 t0040-parse-options: test parse_options() with various 'parse_opt_flags'SZEDER Gábor, Aug 19, 2022
  74. Ævar Arnfjörð BjarmasonAug 19, 2022
  75. SZEDER GáborAug 20, 2022
  76. Junio C HamanoAug 19, 2022
  77. SZEDER GáborAug 20, 2022
  78. Junio C HamanoAug 20, 2022
  79. 08/20 parse-options: drop leading space from '--git-completion-helper' outputSZEDER Gábor, Aug 19, 2022
  80. Ævar Arnfjörð BjarmasonAug 19, 2022
  81. SZEDER GáborAug 19, 2022
  82. 19/20 builtin/stash.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  83. Ævar Arnfjörð BjarmasonAug 19, 2022
  84. SZEDER GáborAug 20, 2022
  85. 06/20 parse-options: PARSE_OPT_KEEP_UNKNOWN only applies to --optionsSZEDER Gábor, Aug 19, 2022
  86. 05/20 api-parse-options.txt: fix description of OPT_CMDMODESZEDER Gábor, Aug 19, 2022
  87. 20/20 builtin/worktree.c: let parse-options parse subcommandsSZEDER Gábor, Aug 19, 2022
  88. 07/20 parse-options: clarify the limitations of PARSE_OPT_NODASHSZEDER Gábor, Aug 19, 2022
  89. 03/20 t5505-remote.sh: check the behavior without a subcommandSZEDER Gábor, Aug 19, 2022
  90. 0/5 parse-options: minor cleanups for handling subcommandsSZEDER Gábor, Sep 5, 2022
  91. 2/5 test-parse-options.c: don't use for loop initial declarationSZEDER Gábor, Sep 5, 2022
  92. 1/5 t0040-parse-options: remove leftover debuggingSZEDER Gábor, Sep 5, 2022
  93. 3/5 test-parse-options.c: fix style of comparison with zeroSZEDER Gábor, Sep 5, 2022
  94. 4/5 notes: simplify default operation mode arguments checkSZEDER Gábor, Sep 5, 2022
  95. 5/5 notes, remote: show unknown subcommands between `'SZEDER Gábor, Sep 5, 2022
  96. Junio C HamanoSep 7, 2022
  97. SZEDER GáborSep 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.