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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Jul 08, 2022 at 02:08:07PM -0700, Junio C Hamano wrote:
> 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