diff mbox series

[v2,04/15] builtin/multi-pack-index.c: split sub-commands

Message ID bc42b56ea2c3ac7ead1cad2a95ac01a7adfbb28f.1614193703.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series midx: implement a multi-pack reverse index | expand

Commit Message

Taylor Blau Feb. 24, 2021, 7:09 p.m. UTC
Handle sub-commands of the 'git multi-pack-index' builtin (e.g.,
"write", "repack", etc.) separately from one another. This allows
sub-commands with unique options, without forcing cmd_multi_pack_index()
to reject invalid combinations itself.

This comes at the cost of some duplication and boilerplate. Luckily, the
duplication is reduced to a minimum, since common options are shared
among sub-commands due to a suggestion by Ævar. (Sub-commands do have to
retain the common options, too, since this builtin accepts common
options on either side of the sub-command).

Roughly speaking, cmd_multi_pack_index() parses options (including
common ones), and stops at the first non-option, which is the
sub-command. It then dispatches to the appropriate sub-command, which
parses the remaining options (also including common options).

Unknown options are kept by the sub-commands in order to detect their
presence (and complain that too many arguments were given).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c | 131 ++++++++++++++++++++++++++++++-------
 1 file changed, 106 insertions(+), 25 deletions(-)

Comments

Jonathan Tan March 2, 2021, 4:06 a.m. UTC | #1
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index eea498e026..caf0248a98 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -5,17 +5,33 @@
>  #include "midx.h"
>  #include "trace2.h"
>  
> +static char const * const builtin_multi_pack_index_write_usage[] = {
>  #define BUILTIN_MIDX_WRITE_USAGE \
>  	N_("git multi-pack-index [<options>] write")
> +	BUILTIN_MIDX_WRITE_USAGE,
> +	NULL
> +};

I think this way of writing is vulnerable to confusing errors if a
missing or extra backslash happens, so I would prefer the #define to be
outside the variable declaration.

> +static int cmd_multi_pack_index_repack(int argc, const char **argv)
> +{
> +	struct option *options;
> +	static struct option builtin_multi_pack_index_repack_options[] = {
>  		OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
>  		  N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
>  		OPT_END(),
>  	};
>  
> +	options = parse_options_dup(builtin_multi_pack_index_repack_options);
> +	options = add_common_options(options);

I looked for where this was freed, but I guess freeing this struct is
not really something we're worried about (which makes sense).

The other patches up to this one look good.
Taylor Blau March 2, 2021, 7:02 p.m. UTC | #2
On Mon, Mar 01, 2021 at 08:06:25PM -0800, Jonathan Tan wrote:
> > +static char const * const builtin_multi_pack_index_write_usage[] = {
> >  #define BUILTIN_MIDX_WRITE_USAGE \
> >  	N_("git multi-pack-index [<options>] write")
> > +	BUILTIN_MIDX_WRITE_USAGE,
> > +	NULL
> > +};
>
> I think this way of writing is vulnerable to confusing errors if a
> missing or extra backslash happens, so I would prefer the #define to be
> outside the variable declaration.

Yeah, I can't say that I disagree with you. Of course, having the
#define's outside of the declaration makes the whole thing a little more
verbose, which isn't a huge deal.

But I was mirroring what Ævar was doing in the sub-thread he started at:

    https://public-inbox.org/git/20210215184118.11306-1-avarab@gmail.com/

Unless you feel strongly, I think that what we have isn't so bad here.

> > +static int cmd_multi_pack_index_repack(int argc, const char **argv)
> > +{
> > +	struct option *options;
> > +	static struct option builtin_multi_pack_index_repack_options[] = {
> >  		OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
> >  		  N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
> >  		OPT_END(),
> >  	};
> >
> > +	options = parse_options_dup(builtin_multi_pack_index_repack_options);
> > +	options = add_common_options(options);
>
> I looked for where this was freed, but I guess freeing this struct is
> not really something we're worried about (which makes sense).

Right.

Thanks,
Taylor
Jonathan Tan March 4, 2021, 1:54 a.m. UTC | #3
> On Mon, Mar 01, 2021 at 08:06:25PM -0800, Jonathan Tan wrote:
> > > +static char const * const builtin_multi_pack_index_write_usage[] = {
> > >  #define BUILTIN_MIDX_WRITE_USAGE \
> > >  	N_("git multi-pack-index [<options>] write")
> > > +	BUILTIN_MIDX_WRITE_USAGE,
> > > +	NULL
> > > +};
> >
> > I think this way of writing is vulnerable to confusing errors if a
> > missing or extra backslash happens, so I would prefer the #define to be
> > outside the variable declaration.
> 
> Yeah, I can't say that I disagree with you. Of course, having the
> #define's outside of the declaration makes the whole thing a little more
> verbose, which isn't a huge deal.

I think it's the same verbosity - you just need to move the lines?

> But I was mirroring what Ævar was doing in the sub-thread he started at:
> 
>     https://public-inbox.org/git/20210215184118.11306-1-avarab@gmail.com/
> 
> Unless you feel strongly, I think that what we have isn't so bad here.

Yeah I don't feel that strongly about it.
Taylor Blau March 4, 2021, 3:02 a.m. UTC | #4
On Wed, Mar 03, 2021 at 05:54:44PM -0800, Jonathan Tan wrote:
> > On Mon, Mar 01, 2021 at 08:06:25PM -0800, Jonathan Tan wrote:
> > > > +static char const * const builtin_multi_pack_index_write_usage[] = {
> > > >  #define BUILTIN_MIDX_WRITE_USAGE \
> > > >  	N_("git multi-pack-index [<options>] write")
> > > > +	BUILTIN_MIDX_WRITE_USAGE,
> > > > +	NULL
> > > > +};
> > >
> > > I think this way of writing is vulnerable to confusing errors if a
> > > missing or extra backslash happens, so I would prefer the #define to be
> > > outside the variable declaration.
> >
> > Yeah, I can't say that I disagree with you. Of course, having the
> > #define's outside of the declaration makes the whole thing a little more
> > verbose, which isn't a huge deal.
>
> I think it's the same verbosity - you just need to move the lines?

Yeah, you're right. I'm being too subjective, and I don't really feel
strongly, either.

>
> > But I was mirroring what Ævar was doing in the sub-thread he started at:
> >
> >     https://public-inbox.org/git/20210215184118.11306-1-avarab@gmail.com/
> >
> > Unless you feel strongly, I think that what we have isn't so bad here.
>
> Yeah I don't feel that strongly about it.

I'll take your suggestion, thanks.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index eea498e026..caf0248a98 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -5,17 +5,33 @@ 
 #include "midx.h"
 #include "trace2.h"
 
+static char const * const builtin_multi_pack_index_write_usage[] = {
 #define BUILTIN_MIDX_WRITE_USAGE \
 	N_("git multi-pack-index [<options>] write")
+	BUILTIN_MIDX_WRITE_USAGE,
+	NULL
+};
 
+static char const * const builtin_multi_pack_index_verify_usage[] = {
 #define BUILTIN_MIDX_VERIFY_USAGE \
 	N_("git multi-pack-index [<options>] verify")
+	BUILTIN_MIDX_VERIFY_USAGE,
+	NULL
+};
 
+static char const * const builtin_multi_pack_index_expire_usage[] = {
 #define BUILTIN_MIDX_EXPIRE_USAGE \
 	N_("git multi-pack-index [<options>] expire")
+	BUILTIN_MIDX_EXPIRE_USAGE,
+	NULL
+};
 
+static char const * const builtin_multi_pack_index_repack_usage[] = {
 #define BUILTIN_MIDX_REPACK_USAGE \
 	N_("git multi-pack-index [<options>] repack [--batch-size=<size>]")
+	BUILTIN_MIDX_REPACK_USAGE,
+	NULL
+};
 
 static char const * const builtin_multi_pack_index_usage[] = {
 	BUILTIN_MIDX_WRITE_USAGE,
@@ -31,25 +47,99 @@  static struct opts_multi_pack_index {
 	unsigned flags;
 } opts;
 
-int cmd_multi_pack_index(int argc, const char **argv,
-			 const char *prefix)
+static struct option common_opts[] = {
+	OPT_FILENAME(0, "object-dir", &opts.object_dir,
+	  N_("object directory containing set of packfile and pack-index pairs")),
+	OPT_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS),
+	OPT_END(),
+};
+
+static struct option *add_common_options(struct option *prev)
 {
-	static struct option builtin_multi_pack_index_options[] = {
-		OPT_FILENAME(0, "object-dir", &opts.object_dir,
-		  N_("object directory containing set of packfile and pack-index pairs")),
-		OPT_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS),
+	struct option *with_common = parse_options_concat(common_opts, prev);
+	free(prev);
+	return with_common;
+}
+
+static int cmd_multi_pack_index_write(int argc, const char **argv)
+{
+	struct option *options = common_opts;
+
+	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);
+
+	return write_midx_file(opts.object_dir, opts.flags);
+}
+
+static int cmd_multi_pack_index_verify(int argc, const char **argv)
+{
+	struct option *options = common_opts;
+
+	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);
+
+	return verify_midx_file(the_repository, opts.object_dir, opts.flags);
+}
+
+static int cmd_multi_pack_index_expire(int argc, const char **argv)
+{
+	struct option *options = common_opts;
+
+	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);
+
+	return expire_midx_packs(the_repository, opts.object_dir, opts.flags);
+}
+
+static int cmd_multi_pack_index_repack(int argc, const char **argv)
+{
+	struct option *options;
+	static struct option builtin_multi_pack_index_repack_options[] = {
 		OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
 		  N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
 		OPT_END(),
 	};
 
+	options = parse_options_dup(builtin_multi_pack_index_repack_options);
+	options = add_common_options(options);
+
+	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);
+
+	return midx_repack(the_repository, opts.object_dir,
+			   (size_t)opts.batch_size, opts.flags);
+}
+
+int cmd_multi_pack_index(int argc, const char **argv,
+			 const char *prefix)
+{
+	struct option *builtin_multi_pack_index_options = common_opts;
+
 	git_config(git_default_config, NULL);
 
 	if (isatty(2))
 		opts.flags |= MIDX_PROGRESS;
 	argc = parse_options(argc, argv, prefix,
 			     builtin_multi_pack_index_options,
-			     builtin_multi_pack_index_usage, 0);
+			     builtin_multi_pack_index_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
 
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();
@@ -58,25 +148,16 @@  int cmd_multi_pack_index(int argc, const char **argv,
 		usage_with_options(builtin_multi_pack_index_usage,
 				   builtin_multi_pack_index_options);
 
-	if (argc > 1) {
-		die(_("too many arguments"));
-		return 1;
-	}
-
 	trace2_cmd_mode(argv[0]);
 
 	if (!strcmp(argv[0], "repack"))
-		return midx_repack(the_repository, opts.object_dir,
-			(size_t)opts.batch_size, opts.flags);
-	if (opts.batch_size)
-		die(_("--batch-size option is only for 'repack' subcommand"));
-
-	if (!strcmp(argv[0], "write"))
-		return write_midx_file(opts.object_dir, opts.flags);
-	if (!strcmp(argv[0], "verify"))
-		return verify_midx_file(the_repository, opts.object_dir, opts.flags);
-	if (!strcmp(argv[0], "expire"))
-		return expire_midx_packs(the_repository, opts.object_dir, opts.flags);
-
-	die(_("unrecognized subcommand: %s"), argv[0]);
+		return cmd_multi_pack_index_repack(argc, argv);
+	else if (!strcmp(argv[0], "write"))
+		return cmd_multi_pack_index_write(argc, argv);
+	else if (!strcmp(argv[0], "verify"))
+		return cmd_multi_pack_index_verify(argc, argv);
+	else if (!strcmp(argv[0], "expire"))
+		return cmd_multi_pack_index_expire(argc, argv);
+	else
+		die(_("unrecognized subcommand: %s"), argv[0]);
 }