| Message ID | d084f90466813597feb27975654e57de36eb62a4.1615482270.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | midx: implement a multi-pack reverse index | expand |
On Thu, Mar 11, 2021 at 12:04:49PM -0500, Taylor Blau wrote: > 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). Makes sense, and the implementation looks pretty clean. A few small nits: > +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; > +} This free(prev) pattern is copied from builtin/checkout.c, where we have multiple layers of options, each added by a function. So it requires that callers duplicate the base set of options, and each subsequent "add_foo_options()" concatenates that and frees the old one. But here, we only have one layer, so in the caller which uses it: > +static int cmd_multi_pack_index_repack(int argc, const char **argv) > +{ > [..] > + options = parse_options_dup(builtin_multi_pack_index_repack_options); > + options = add_common_options(options); we do a rather pointless dup() followed by free(). Perhaps not that big a deal, and this would naturally extend to adding other option sets, so it may even be considered future-proofing. But it did confuse me for a moment. However, we do end up leaking the return value from add_common_options() at the end of the function: > + 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); > +} This is definitely a harmless leak in the sense that we are going to exit the program after midx_repack() returns anyway. But it might be worth keeping things tidy, as we've recently seen a renewed effort to do some leak-checking of the test suite. I _think_ we can just free the options struct (even though we are still using the values themselves, we don't care about the "struct options" anymore). But even if not, an UNLEAK(options) annotation would do it. (This doesn't apply to the other functions, because they just use common_opts directly). -Peff
On Mon, Mar 29, 2021 at 07:36:21AM -0400, Jeff King wrote: > This is definitely a harmless leak in the sense that we are going to > exit the program after midx_repack() returns anyway. But it might be > worth keeping things tidy, as we've recently seen a renewed effort to do > some leak-checking of the test suite. I _think_ we can just free the > options struct (even though we are still using the values themselves, we > don't care about the "struct options" anymore). But even if not, an > UNLEAK(options) annotation would do it. I see what you're saying. Let me make sure that I got the right idea in mind after reading your email. I'm thinking of squashing the following diff into this patch. For what it's worth, it causes 'valgrind --leak-check=full ./git-multi-pack-index repack' to exit cleanly (when it didn't before). Does this match your expectations? --- >8 --- diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 23e51dfeb4..a78640c061 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -56,9 +56,7 @@ static struct option common_opts[] = { static struct option *add_common_options(struct option *prev) { - struct option *with_common = parse_options_concat(common_opts, prev); - free(prev); - return with_common; + return parse_options_concat(common_opts, prev); } static int cmd_multi_pack_index_write(int argc, const char **argv) @@ -112,8 +110,7 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) OPT_END(), }; - options = parse_options_dup(builtin_multi_pack_index_repack_options); - options = add_common_options(options); + options = add_common_options(builtin_multi_pack_index_repack_options); argc = parse_options(argc, argv, NULL, options, @@ -123,6 +120,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) usage_with_options(builtin_multi_pack_index_repack_usage, options); + FREE_AND_NULL(options); + return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size, opts.flags); }
On Mon, Mar 29, 2021 at 04:38:33PM -0400, Taylor Blau wrote: > On Mon, Mar 29, 2021 at 07:36:21AM -0400, Jeff King wrote: > > This is definitely a harmless leak in the sense that we are going to > > exit the program after midx_repack() returns anyway. But it might be > > worth keeping things tidy, as we've recently seen a renewed effort to do > > some leak-checking of the test suite. I _think_ we can just free the > > options struct (even though we are still using the values themselves, we > > don't care about the "struct options" anymore). But even if not, an > > UNLEAK(options) annotation would do it. > > I see what you're saying. Let me make sure that I got the right idea in > mind after reading your email. I'm thinking of squashing the following > diff into this patch. For what it's worth, it causes 'valgrind > --leak-check=full ./git-multi-pack-index repack' to exit cleanly (when > it didn't before). > > Does this match your expectations? Yes, though... > diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c > index 23e51dfeb4..a78640c061 100644 > --- a/builtin/multi-pack-index.c > +++ b/builtin/multi-pack-index.c > @@ -56,9 +56,7 @@ static struct option common_opts[] = { > > static struct option *add_common_options(struct option *prev) > { > - struct option *with_common = parse_options_concat(common_opts, prev); > - free(prev); > - return with_common; > + return parse_options_concat(common_opts, prev); > } This simplification is orthogonal to the leak, and I'd be OK if you wanted to retain it as it was before (because it future-proofs against adding more add_foo_options() later, though for now it is a useless dup/free pair). > @@ -123,6 +120,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) > usage_with_options(builtin_multi_pack_index_repack_usage, > options); > > + FREE_AND_NULL(options); > + And this is the leak fix I care about. We'd want the same thing in the later caller that adds another use of add_common_options(), of course. -Peff
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index eea498e026..23e51dfeb4 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -17,6 +17,22 @@ #define BUILTIN_MIDX_REPACK_USAGE \ N_("git multi-pack-index [<options>] repack [--batch-size=<size>]") +static char const * const builtin_multi_pack_index_write_usage[] = { + BUILTIN_MIDX_WRITE_USAGE, + NULL +}; +static char const * const builtin_multi_pack_index_verify_usage[] = { + BUILTIN_MIDX_VERIFY_USAGE, + NULL +}; +static char const * const builtin_multi_pack_index_expire_usage[] = { + BUILTIN_MIDX_EXPIRE_USAGE, + NULL +}; +static char const * const builtin_multi_pack_index_repack_usage[] = { + BUILTIN_MIDX_REPACK_USAGE, + NULL +}; static char const * const builtin_multi_pack_index_usage[] = { BUILTIN_MIDX_WRITE_USAGE, BUILTIN_MIDX_VERIFY_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]); }
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(-)