| Message ID | 43fc0ad276406ff77283613c45188e102a6dc515.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:36PM -0500, Taylor Blau wrote: > Subcommands of the 'git multi-pack-index' command (e.g., 'write', > 'verify', etc.) will want to optionally change a set of shared flags > that are eventually passed to the MIDX libraries. > > Right now, options and flags are handled separately. Inline them into > the same structure so that sub-commands can more easily share the > 'flags' data. This "opts" struct is kind of funny. It is used to collect the options in cmd_multi_pack_index(), but nobody ever passes it anywhere! Instead, we pass individual components of it around. So I'm not sure I buy "...so that sub-commands can more easily share the flags data", since either way they are all receiving the individual flags field already. And your patch 2 could just as easily do the same simplification by modifying the function-local "flags" variable. But. I think things get more interesting when you later introduce common_opts, because now those options have to refer back to actuals storage for each item. Which means that "flags" would have to become a global variable. And there it's nicer to have all of the options stuffed into a struct, even if it is a single global struct. So I think this is the right direction, but it took me a minute to realize quite why. -Peff
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 5bf88cd2a8..4a0ddb06c4 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -14,13 +14,12 @@ static struct opts_multi_pack_index { const char *object_dir; unsigned long batch_size; int progress; + unsigned flags; } opts; int cmd_multi_pack_index(int argc, const char **argv, const char *prefix) { - unsigned flags = 0; - 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")), @@ -40,7 +39,7 @@ int cmd_multi_pack_index(int argc, const char **argv, if (!opts.object_dir) opts.object_dir = get_object_directory(); if (opts.progress) - flags |= MIDX_PROGRESS; + opts.flags |= MIDX_PROGRESS; if (argc == 0) usage_with_options(builtin_multi_pack_index_usage, @@ -55,16 +54,16 @@ int cmd_multi_pack_index(int argc, const char **argv, if (!strcmp(argv[0], "repack")) return midx_repack(the_repository, opts.object_dir, - (size_t)opts.batch_size, flags); + (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, flags); + return write_midx_file(opts.object_dir, opts.flags); if (!strcmp(argv[0], "verify")) - return verify_midx_file(the_repository, opts.object_dir, flags); + 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, flags); + return expire_midx_packs(the_repository, opts.object_dir, opts.flags); die(_("unrecognized subcommand: %s"), argv[0]); }
Subcommands of the 'git multi-pack-index' command (e.g., 'write', 'verify', etc.) will want to optionally change a set of shared flags that are eventually passed to the MIDX libraries. Right now, options and flags are handled separately. Inline them into the same structure so that sub-commands can more easily share the 'flags' data. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/multi-pack-index.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)