Re: [PATCH] multi-pack-index: simplify handling of unknown --options

[Junio C Hamano <gitster@pobox.com>]

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.

Thanks.

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