Re: [PATCH] multi-pack-index: simplify handling of unknown --options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 8, 2022, 21:08 UTC
- Message-ID
- <xmqqh73ruxc8.fsf@gitster.g>
- In-Reply-To
- <20220708202847.662319-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 5 quoted lines
> Although parse_options() can handle unknown --options just fine, none > of 'git multi-pack-index's subcommands rely on it, but do it on their > own: they invoke parse_options() with the PARSE_OPT_KEEP_UNKNOWN flag, > then check whether there are any unparsed arguments left, and print > usage and quit if necessary.
The existing code check if there are any unparsed arguments or options.
Omitting PARSE_OPT_KEEP_UNKNOWN allows parse_options() to deal with unknown options by complaining, but it happily leaves non-options on the command line and reports how many of them there are.
Doesn't this patch make
$ git multi-pack-index write what-is-this-extra-arg-doing-here
silently ignore the extra argument instead of barfing on it?
> Let parse_options() handle unknown options instead, which, besides > simpler code, has the additional benefit that it prints not only the > usage but an "error: unknown option `foo'" message as well.
Yes, I agree that getting rid of KEEP_UNKNOWN is a very good idea for this reason. But I suspect that we still need the "did we get an extra argument we do not know what to do with?" check.
Thanks.
Show 56 quoted lines
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > builtin/multi-pack-index.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c > index 5edbb7fe86..97a87ad8cb 100644 > --- a/builtin/multi-pack-index.c > +++ b/builtin/multi-pack-index.c > @@ -134,10 +134,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_write_usage, > - PARSE_OPT_KEEP_UNKNOWN); > - if (argc) > - usage_with_options(builtin_multi_pack_index_write_usage, > - options); > + 0); > > FREE_AND_NULL(options); > > @@ -176,10 +173,7 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_verify_usage, > - PARSE_OPT_KEEP_UNKNOWN); > - if (argc) > - usage_with_options(builtin_multi_pack_index_verify_usage, > - options); > + 0); > > FREE_AND_NULL(options); > > @@ -202,10 +196,7 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv) > opts.flags |= MIDX_PROGRESS; > argc = parse_options(argc, argv, NULL, > options, builtin_multi_pack_index_expire_usage, > - PARSE_OPT_KEEP_UNKNOWN); > - if (argc) > - usage_with_options(builtin_multi_pack_index_expire_usage, > - options); > + 0); > > FREE_AND_NULL(options); > > @@ -232,10 +223,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) > argc = parse_options(argc, argv, NULL, > options, > builtin_multi_pack_index_repack_usage, > - PARSE_OPT_KEEP_UNKNOWN); > - if (argc) > - usage_with_options(builtin_multi_pack_index_repack_usage, > - options); > + 0); > > FREE_AND_NULL(options);