| Message ID | 16f33e41388ed431f70e09ef68717bd30fbee67f.1613422804.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: split out sub-commands | expand |
On Mon, Feb 15 2021, Taylor Blau wrote: > trace2_cmd_mode(argv[0]); Not a new issue, but curious that in the commit-graph.c code we'll first validate, but here write garbage into the trace2_cmd_mode() before potentially dying. > > 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]); I realize this is the existing behavior, but let's just make this die() be the usage_with_options() we emit above in this case? So maybe this on top? diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index caf0248a98..6f9223d538 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -65,6 +65,8 @@ static int cmd_multi_pack_index_write(int argc, const char **argv) { struct option *options = common_opts; + trace2_cmd_mode(argv[0]); + argc = parse_options(argc, argv, NULL, options, builtin_multi_pack_index_write_usage, PARSE_OPT_KEEP_UNKNOWN); @@ -79,6 +81,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv) { struct option *options = common_opts; + trace2_cmd_mode(argv[0]); + argc = parse_options(argc, argv, NULL, options, builtin_multi_pack_index_verify_usage, PARSE_OPT_KEEP_UNKNOWN); @@ -93,6 +97,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv) { struct option *options = common_opts; + trace2_cmd_mode(argv[0]); + argc = parse_options(argc, argv, NULL, options, builtin_multi_pack_index_expire_usage, PARSE_OPT_KEEP_UNKNOWN); @@ -112,6 +118,8 @@ static int cmd_multi_pack_index_repack(int argc, const char **argv) OPT_END(), }; + trace2_cmd_mode(argv[0]); + options = parse_options_dup(builtin_multi_pack_index_repack_options); options = add_common_options(options); @@ -144,20 +152,15 @@ int cmd_multi_pack_index(int argc, const char **argv, if (!opts.object_dir) opts.object_dir = get_object_directory(); - if (argc == 0) - usage_with_options(builtin_multi_pack_index_usage, - builtin_multi_pack_index_options); - - trace2_cmd_mode(argv[0]); - - if (!strcmp(argv[0], "repack")) + if (argc && !strcmp(argv[0], "repack")) return cmd_multi_pack_index_repack(argc, argv); - else if (!strcmp(argv[0], "write")) + else if (argc && !strcmp(argv[0], "write")) return cmd_multi_pack_index_write(argc, argv); - else if (!strcmp(argv[0], "verify")) + else if (argc && !strcmp(argv[0], "verify")) return cmd_multi_pack_index_verify(argc, argv); - else if (!strcmp(argv[0], "expire")) + else if (argc && !strcmp(argv[0], "expire")) return cmd_multi_pack_index_expire(argc, argv); else - die(_("unrecognized subcommand: %s"), argv[0]); + usage_with_options(builtin_multi_pack_index_usage, + builtin_multi_pack_index_options); }
On Mon, Feb 15, 2021 at 10:54:31PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Feb 15 2021, Taylor Blau wrote: > > > trace2_cmd_mode(argv[0]); > > Not a new issue, but curious that in the commit-graph.c code we'll first > validate, but here write garbage into the trace2_cmd_mode() before > potentially dying. Yeah, that's a good catch. > I realize this is the existing behavior, but let's just make this die() > be the usage_with_options() we emit above in this case? > > So maybe this on top? I split this into two patches: one to move the trace2_cmd_mode() calls around, and another to replace the final 'die()' with the usage text. Like I said in my review of your patches to the commit-graph builtin here: https://lore.kernel.org/git/YCrDGhIq7kU57p1s@nand.local/ I don't find the 'if (argc && ...)' style more readable, so the second patch looks like this instead: diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 5ab80bc722..ce4f1a0bcb 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -165,5 +165,6 @@ int cmd_multi_pack_index(int argc, const char **argv, else if (!strcmp(argv[0], "expire")) return cmd_multi_pack_index_expire(argc, argv); else - die(_("unrecognized subcommand: %s"), argv[0]); + usage_with_options(builtin_multi_pack_index_usage, + builtin_multi_pack_index_options); } Is it OK if I use your Signed-off-by on both of those two new patches? Thanks, Taylor
On Mon, Feb 15 2021, Taylor Blau wrote: > On Mon, Feb 15, 2021 at 10:54:31PM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Feb 15 2021, Taylor Blau wrote: >> >> > trace2_cmd_mode(argv[0]); >> >> Not a new issue, but curious that in the commit-graph.c code we'll first >> validate, but here write garbage into the trace2_cmd_mode() before >> potentially dying. > > Yeah, that's a good catch. > >> I realize this is the existing behavior, but let's just make this die() >> be the usage_with_options() we emit above in this case? >> >> So maybe this on top? > > I split this into two patches: one to move the trace2_cmd_mode() calls > around, and another to replace the final 'die()' with the usage text. Thanks for picking it up. > Like I said in my review of your patches to the commit-graph builtin > here: > > https://lore.kernel.org/git/YCrDGhIq7kU57p1s@nand.local/ > > I don't find the 'if (argc && ...)' style more readable, so the second > patch looks like this instead: *Nod* FWIW (and this is getting way to nit-y) I don't disagree with you about the "argc &&" being not very readable, I just lean more on the side of getting rid of duplicate branches, you'll still need the if (!argc) usage(...) case above without that pattern, or some replacement for it. But we can have our cake (not re-check argc all the time) and eat it too (not copy/paste usage_with_options()). Isn't it beautiful? diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index caf0248a98..7ff50439f8 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -144,12 +144,8 @@ int cmd_multi_pack_index(int argc, const char **argv, if (!opts.object_dir) opts.object_dir = get_object_directory(); - if (argc == 0) - usage_with_options(builtin_multi_pack_index_usage, - builtin_multi_pack_index_options); - - trace2_cmd_mode(argv[0]); - + if (!argc) + goto usage; if (!strcmp(argv[0], "repack")) return cmd_multi_pack_index_repack(argc, argv); else if (!strcmp(argv[0], "write")) @@ -159,5 +155,7 @@ int cmd_multi_pack_index(int argc, const char **argv, else if (!strcmp(argv[0], "expire")) return cmd_multi_pack_index_expire(argc, argv); else - die(_("unrecognized subcommand: %s"), argv[0]); +usage: + usage_with_options(builtin_multi_pack_index_usage, + builtin_multi_pack_index_options); } :) > diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c > index 5ab80bc722..ce4f1a0bcb 100644 > --- a/builtin/multi-pack-index.c > +++ b/builtin/multi-pack-index.c > @@ -165,5 +165,6 @@ int cmd_multi_pack_index(int argc, const char **argv, > else if (!strcmp(argv[0], "expire")) > return cmd_multi_pack_index_expire(argc, argv); > else > - die(_("unrecognized subcommand: %s"), argv[0]); > + usage_with_options(builtin_multi_pack_index_usage, > + builtin_multi_pack_index_options); > } > > Is it OK if I use your Signed-off-by on both of those two new patches? Yes please, should have included it to begin with.
On Tue, Feb 16, 2021 at 12:11:08AM +0100, Ævar Arnfjörð Bjarmason wrote: > > I split this into two patches: one to move the trace2_cmd_mode() calls > > around, and another to replace the final 'die()' with the usage text. > > Thanks for picking it up. Of course. This has been quite a fun digression :-). > > Like I said in my review of your patches to the commit-graph builtin > > here: > > > > https://lore.kernel.org/git/YCrDGhIq7kU57p1s@nand.local/ > > > > I don't find the 'if (argc && ...)' style more readable, so the second > > patch looks like this instead: > > *Nod* FWIW (and this is getting way to nit-y) I don't disagree with you > about the "argc &&" being not very readable, > > I just lean more on the side of getting rid of duplicate branches, > you'll still need the if (!argc) usage(...) case above without that > pattern, or some replacement for it. > > But we can have our cake (not re-check argc all the time) and eat it too > (not copy/paste usage_with_options()). Isn't it beautiful? Heh; I'm not sure that I'd call adding a goto "beautiful", but I actually do find this one more readable. I dunno, honestly, I'm happy to squash it in to the last commit on top, but honestly I don't really care strongly one way or another ;). > > Is it OK if I use your Signed-off-by on both of those two new patches? > > Yes please, should have included it to begin with. Thanks, and no worries. Thanks, Taylor
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]); }
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(-)