Re: [PATCH] multi-pack-index: simplify handling of unknown --options
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Jul 10, 2022, 15:16 UTC
- Message-ID
- <20220710151645.GA2038@szeder.dev>
- In-Reply-To
- <xmqqh73ruxc8.fsf@gitster.g>
On Fri, Jul 08, 2022 at 02:08:07PM -0700, Junio C Hamano wrote:
Show 28 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes: > > > 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.
Uh, indeed. I got too trigger-happy with deleting lines. Updated patch below.
--- >8 ---
Subject: multi-pack-index: simplify handling of unknown --options
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.
Drop that PARSE_OPT_KEEP_UNKNOWN flag to let parse_options() handle unknown options instead, which has the additional benefit that it prints not only the usage but an "error: unknown option `foo'" message as well.
Do leave the unparsed arguments check to catch any unexpected non-option arguments, though, e.g. 'git multi-pack-index write foo'.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- builtin/multi-pack-index.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 5edbb7fe86..8f24d59a75 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -134,7 +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); + 0); if (argc) usage_with_options(builtin_multi_pack_index_write_usage, options); @@ -176,7 +176,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); + 0); if (argc) usage_with_options(builtin_multi_pack_index_verify_usage, options); @@ -202,7 +202,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); + 0); if (argc) usage_with_options(builtin_multi_pack_index_expire_usage, options); @@ -232,7 +232,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); + 0); if (argc) usage_with_options(builtin_multi_pack_index_repack_usage, options);
-- 2.37.0.340.g5e8d960d32