| Message ID | patch-8.8-105853cd358-20221102T074148Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule: tests, cleanup to prepare for built-in | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API > introduced in fa83cc834da (parse-options: add support for parsing > subcommands, 2022-08-19). > > This is only a marginal reduction in line count, but once we start > unifying this with a yet-to-be-added "builtin/submodule.c" it'll be > much easier to reason about those changes, as they'll both use > OPT_SUBCOMMAND(). Even if nothing else, this is a nice standardization change :) > We don't need to worry about "argv[0]" being NULL in the die() because > we'd have errored out in parse_options() as we're not using > "PARSE_OPT_SUBCOMMAND_OPTIONAL". > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/submodule--helper.c | 78 ++++++++++++++++++------------------- > git.c | 2 +- > 2 files changed, 39 insertions(+), 41 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 2012ad31d7f..0bc25dcf5ae 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix) > return ret; > } > > -#define SUPPORT_SUPER_PREFIX (1<<0) > - > -struct cmd_struct { > - const char *cmd; > - int (*fn)(int, const char **, const char *); > - unsigned option; > -}; > - > -static struct cmd_struct commands[] = { > - {"clone", module_clone, SUPPORT_SUPER_PREFIX}, > - {"add", module_add, 0}, > - {"update", module_update, SUPPORT_SUPER_PREFIX}, > - {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, > - {"init", module_init, 0}, > - {"status", module_status, SUPPORT_SUPER_PREFIX}, > - {"sync", module_sync, SUPPORT_SUPER_PREFIX}, > - {"deinit", module_deinit, 0}, > - {"summary", module_summary, 0}, > - {"push-check", push_check, 0}, > - {"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, > - {"set-url", module_set_url, 0}, > - {"set-branch", module_set_branch, 0}, > - {"create-branch", module_create_branch, 0}, > -}; > - > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > { > - int i; > - if (argc < 2 || !strcmp(argv[1], "-h")) > - usage("git submodule--helper <command>"); > - > - for (i = 0; i < ARRAY_SIZE(commands); i++) { > - if (!strcmp(argv[1], commands[i].cmd)) { > - if (get_super_prefix() && > - !(commands[i].option & SUPPORT_SUPER_PREFIX)) > - die(_("%s doesn't support --super-prefix"), > - commands[i].cmd); > - return commands[i].fn(argc - 1, argv + 1, prefix); > - } > - } > + const char *cmd = argv[0]; > + const char *subcmd; > + parse_opt_subcommand_fn *fn = NULL; > + const char *const usage[] = { > + N_("git submodule--helper <command>"), > + NULL > + }; > + struct option options[] = { > + OPT_SUBCOMMAND("clone", &fn, module_clone), > + OPT_SUBCOMMAND("add", &fn, module_add), > + OPT_SUBCOMMAND("update", &fn, module_update), > + OPT_SUBCOMMAND("foreach", &fn, module_foreach), > + OPT_SUBCOMMAND("init", &fn, module_init), > + OPT_SUBCOMMAND("status", &fn, module_status), > + OPT_SUBCOMMAND("sync", &fn, module_sync), > + OPT_SUBCOMMAND("deinit", &fn, module_deinit), > + OPT_SUBCOMMAND("summary", &fn, module_summary), > + OPT_SUBCOMMAND("push-check", &fn, push_check), > + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), > + OPT_SUBCOMMAND("set-url", &fn, module_set_url), > + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), > + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), > + OPT_END() > + }; > + argc = parse_options(argc, argv, prefix, options, usage, 0); > + subcmd = argv[0]; > + > + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && > + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && > + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && > + get_super_prefix()) > + /* > + * xstrfmt() rather than "%s %s" to keep the translated > + * string identical to git.c's. > + */ > + die(_("%s doesn't support --super-prefix"), > + xstrfmt("'%s %s'", cmd, subcmd)); FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks in both the top level and in submodule--helper (i.e. revisiting my complaints from [1]), but I haven't sent it out yet because I haven't found the right way to motivate that change. Feel free to chime in on that series when it comes out. [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/ > - die(_("'%s' is not a valid submodule--helper " > - "subcommand"), argv[1]); > + return fn(argc, argv, prefix); > } > diff --git a/git.c b/git.c > index ee7758dcb0e..fb69e605912 100644 > --- a/git.c > +++ b/git.c > @@ -610,7 +610,7 @@ static struct cmd_struct commands[] = { > { "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE }, > { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, > { "stripspace", cmd_stripspace }, > - { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT }, > + { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX }, > { "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE }, > { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, > { "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG }, > -- > 2.38.0.1280.g8136eb6fab2
On Thu, Nov 03 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > [...] >> + struct option options[] = { >> + OPT_SUBCOMMAND("clone", &fn, module_clone), >> + OPT_SUBCOMMAND("add", &fn, module_add), >> + OPT_SUBCOMMAND("update", &fn, module_update), >> + OPT_SUBCOMMAND("foreach", &fn, module_foreach), >> + OPT_SUBCOMMAND("init", &fn, module_init), >> + OPT_SUBCOMMAND("status", &fn, module_status), >> + OPT_SUBCOMMAND("sync", &fn, module_sync), >> + OPT_SUBCOMMAND("deinit", &fn, module_deinit), >> + OPT_SUBCOMMAND("summary", &fn, module_summary), >> + OPT_SUBCOMMAND("push-check", &fn, push_check), >> + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >> + OPT_SUBCOMMAND("set-url", &fn, module_set_url), >> + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >> + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >> + OPT_END() >> + }; >> + argc = parse_options(argc, argv, prefix, options, usage, 0); >> + subcmd = argv[0]; >> + >> + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >> + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >> + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >> + get_super_prefix()) >> + /* >> + * xstrfmt() rather than "%s %s" to keep the translated >> + * string identical to git.c's. >> + */ >> + die(_("%s doesn't support --super-prefix"), >> + xstrfmt("'%s %s'", cmd, subcmd)); > > FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks > in both the top level and in submodule--helper (i.e. revisiting my > complaints from [1]), but I haven't sent it out yet because I haven't > found the right way to motivate that change. Feel free to chime in on > that series when it comes out. > > [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/ Maybe you have different plans, but keep at the WIP re-roll of what I have after this, I've also removed all of that. Basically, ending up with: --- a/builtin.h +++ b/builtin.h @@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix); int cmd_status(int argc, const char **argv, const char *prefix); int cmd_stash(int argc, const char **argv, const char *prefix); int cmd_stripspace(int argc, const char **argv, const char *prefix); +int cmd_submodule(int argc, const char **argv, const char *prefix); int cmd_submodule__helper(int argc, const char **argv, const char *prefix); +int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix); +int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix); +int cmd_submodule_foreach(int argc, const char **argv, const char *prefix); +int cmd_submodule_status(int argc, const char **argv, const char *prefix); +int cmd_submodule_sync(int argc, const char **argv, const char *prefix); +int cmd_submodule_update(int argc, const char **argv, const char *prefix); int cmd_switch(int argc, const char **argv, const char *prefix); int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); int cmd_tag(int argc, const char **argv, const char *prefix); And changes like: @@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, strvec_pushl(&cpr.args, "--super-prefix", NULL); strvec_pushf(&cpr.args, "%s/", displaypath); - strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive", + strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive", NULL); if (info->quiet) I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but when "foreach" needs to recurse it doesn't invoke a "git submodule foreach", instead it invokes "git submodule--helper-foreach". The reason for doing that is that we can't promote the helper to a built-in without also leaking implementation detail that we support the now internal-only --super-prefix in the command itself. So this code becomes: @@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix) int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { - const char *cmd = argv[0]; - const char *subcmd; parse_opt_subcommand_fn *fn = NULL; const char *const usage[] = { N_("git submodule--helper <command>"), NULL }; struct option options[] = { - OPT_SUBCOMMAND("clone", &fn, module_clone), - OPT_SUBCOMMAND("add", &fn, module_add), - OPT_SUBCOMMAND("update", &fn, module_update), - OPT_SUBCOMMAND("foreach", &fn, module_foreach), - OPT_SUBCOMMAND("init", &fn, module_init), - OPT_SUBCOMMAND("status", &fn, module_status), - OPT_SUBCOMMAND("sync", &fn, module_sync), - OPT_SUBCOMMAND("deinit", &fn, module_deinit), - OPT_SUBCOMMAND("summary", &fn, module_summary), - OPT_SUBCOMMAND("push-check", &fn, push_check), - OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), - OPT_SUBCOMMAND("set-url", &fn, module_set_url), - OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), - OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), + OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check), + OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch), OPT_END() }; argc = parse_options(argc, argv, prefix, options, usage, 0); - subcmd = argv[0]; - - if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && - strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && - strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && - get_super_prefix()) - /* - * xstrfmt() rather than "%s %s" to keep the translated - * string identical to git.c's. - */ - die(_("%s doesn't support --super-prefix"), - xstrfmt("'%s %s'", cmd, subcmd)); return fn(argc, argv, prefix); } I.e. for all the "super-prefix" ones we dispatch directly (and maybe I should just do that too for those two stragglers). It's ugly, but it's only ugly on the inside, but if you can think of a better way to do it...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Thu, Nov 03 2022, Glen Choo wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> [...] >>> + struct option options[] = { >>> + OPT_SUBCOMMAND("clone", &fn, module_clone), >>> + OPT_SUBCOMMAND("add", &fn, module_add), >>> + OPT_SUBCOMMAND("update", &fn, module_update), >>> + OPT_SUBCOMMAND("foreach", &fn, module_foreach), >>> + OPT_SUBCOMMAND("init", &fn, module_init), >>> + OPT_SUBCOMMAND("status", &fn, module_status), >>> + OPT_SUBCOMMAND("sync", &fn, module_sync), >>> + OPT_SUBCOMMAND("deinit", &fn, module_deinit), >>> + OPT_SUBCOMMAND("summary", &fn, module_summary), >>> + OPT_SUBCOMMAND("push-check", &fn, push_check), >>> + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >>> + OPT_SUBCOMMAND("set-url", &fn, module_set_url), >>> + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >>> + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >>> + OPT_END() >>> + }; >>> + argc = parse_options(argc, argv, prefix, options, usage, 0); >>> + subcmd = argv[0]; >>> + >>> + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >>> + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >>> + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >>> + get_super_prefix()) >>> + /* >>> + * xstrfmt() rather than "%s %s" to keep the translated >>> + * string identical to git.c's. >>> + */ >>> + die(_("%s doesn't support --super-prefix"), >>> + xstrfmt("'%s %s'", cmd, subcmd)); >> >> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks >> in both the top level and in submodule--helper (i.e. revisiting my >> complaints from [1]), but I haven't sent it out yet because I haven't >> found the right way to motivate that change. Feel free to chime in on >> that series when it comes out. >> >> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/ > > Maybe you have different plans, but keep at the WIP re-roll of what I > have after this, I've also removed all of that. > > Basically, ending up with: > > --- a/builtin.h > +++ b/builtin.h > @@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix); > int cmd_status(int argc, const char **argv, const char *prefix); > int cmd_stash(int argc, const char **argv, const char *prefix); > int cmd_stripspace(int argc, const char **argv, const char *prefix); > +int cmd_submodule(int argc, const char **argv, const char *prefix); > int cmd_submodule__helper(int argc, const char **argv, const char *prefix); > +int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix); > +int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix); > +int cmd_submodule_foreach(int argc, const char **argv, const char *prefix); > +int cmd_submodule_status(int argc, const char **argv, const char *prefix); > +int cmd_submodule_sync(int argc, const char **argv, const char *prefix); > +int cmd_submodule_update(int argc, const char **argv, const char *prefix); > int cmd_switch(int argc, const char **argv, const char *prefix); > int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); > int cmd_tag(int argc, const char **argv, const char *prefix); > > And changes like: > > @@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, > > strvec_pushl(&cpr.args, "--super-prefix", NULL); > strvec_pushf(&cpr.args, "%s/", displaypath); > - strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive", > + strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive", > NULL); > > if (info->quiet) > > I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but > when "foreach" needs to recurse it doesn't invoke a "git submodule > foreach", instead it invokes "git submodule--helper-foreach". > > The reason for doing that is that we can't promote the helper to a > built-in without also leaking implementation detail that we support the > now internal-only --super-prefix in the command itself. > > So this code becomes: > > @@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix) > > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > { > - const char *cmd = argv[0]; > - const char *subcmd; > parse_opt_subcommand_fn *fn = NULL; > const char *const usage[] = { > N_("git submodule--helper <command>"), > NULL > }; > struct option options[] = { > - OPT_SUBCOMMAND("clone", &fn, module_clone), > - OPT_SUBCOMMAND("add", &fn, module_add), > - OPT_SUBCOMMAND("update", &fn, module_update), > - OPT_SUBCOMMAND("foreach", &fn, module_foreach), > - OPT_SUBCOMMAND("init", &fn, module_init), > - OPT_SUBCOMMAND("status", &fn, module_status), > - OPT_SUBCOMMAND("sync", &fn, module_sync), > - OPT_SUBCOMMAND("deinit", &fn, module_deinit), > - OPT_SUBCOMMAND("summary", &fn, module_summary), > - OPT_SUBCOMMAND("push-check", &fn, push_check), > - OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), > - OPT_SUBCOMMAND("set-url", &fn, module_set_url), > - OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), > - OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), > + OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check), > + OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch), > OPT_END() > }; > argc = parse_options(argc, argv, prefix, options, usage, 0); > - subcmd = argv[0]; > - > - if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && > - strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && > - strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && > - get_super_prefix()) > - /* > - * xstrfmt() rather than "%s %s" to keep the translated > - * string identical to git.c's. > - */ > - die(_("%s doesn't support --super-prefix"), > - xstrfmt("'%s %s'", cmd, subcmd)); > > return fn(argc, argv, prefix); > } > > I.e. for all the "super-prefix" ones we dispatch directly (and maybe I > should just do that too for those two stragglers). > > It's ugly, but it's only ugly on the inside, but if you can think of a > better way to do it... If we _really_ wanted to keep the check, an alternative might be to teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I think this SUPPORT_SUPER_PREFIX check is far more trouble than it's worth, and I wouldn't want you to spend your time trying to find ways to keep it only for it to be dropped altogether. Here's a snippet from the cover letter I'm working on, which will hopefully convince you that we don't need to worry about this any more. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make super-prefix option, 2016-10-07), we specified that commands must prefix paths by it, and pathspecs must be parsed relative to it. That commit also includes safeguards to ensure that "--super-prefix" is used correctly, namely: - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix". - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX one. However, its use is inconsistent, which is a strong reason to consider using better-scoped flags instead. For example.. - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have a documented reason, but since it can be invoked by "read-tree", it's presumably also a workaround. - "read-tree" and "submodule--helper" use different values for the super prefix; "read-tree" passes the path from the root of the superproject's tree to the submodule's gitlink, while "submodule--helper" passes the relative path of the original CWD to the submodule. - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to display paths. This series fixes replaces "--super-prefix" with such better-scoped flags, and fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check. [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26) ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- I figured out all the details, but it would look something like: - Add an internal-only "--tree-super-prefix" flag to "git read-tree", which sets a global variable that is read from unpack-trees.c. - Add an internal-only "--wt-super-prefix" flag to "git submodule--helper", which sets a global variable that is read from submodule.c. Unlike "--super-prefix", this won't be gated behind a SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every subcommand is using this "--wt-super-prefix" correctly, so we can just make sure that new subcommands do too. - Remove the global "--super-prefix", the corresponding env var and SUPPORT_SUPER_PREFIX.
On Fri, Nov 04 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Thu, Nov 03 2022, Glen Choo wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> [...] >>>> + struct option options[] = { >>>> + OPT_SUBCOMMAND("clone", &fn, module_clone), >>>> + OPT_SUBCOMMAND("add", &fn, module_add), >>>> + OPT_SUBCOMMAND("update", &fn, module_update), >>>> + OPT_SUBCOMMAND("foreach", &fn, module_foreach), >>>> + OPT_SUBCOMMAND("init", &fn, module_init), >>>> + OPT_SUBCOMMAND("status", &fn, module_status), >>>> + OPT_SUBCOMMAND("sync", &fn, module_sync), >>>> + OPT_SUBCOMMAND("deinit", &fn, module_deinit), >>>> + OPT_SUBCOMMAND("summary", &fn, module_summary), >>>> + OPT_SUBCOMMAND("push-check", &fn, push_check), >>>> + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >>>> + OPT_SUBCOMMAND("set-url", &fn, module_set_url), >>>> + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >>>> + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >>>> + OPT_END() >>>> + }; >>>> + argc = parse_options(argc, argv, prefix, options, usage, 0); >>>> + subcmd = argv[0]; >>>> + >>>> + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >>>> + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >>>> + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >>>> + get_super_prefix()) >>>> + /* >>>> + * xstrfmt() rather than "%s %s" to keep the translated >>>> + * string identical to git.c's. >>>> + */ >>>> + die(_("%s doesn't support --super-prefix"), >>>> + xstrfmt("'%s %s'", cmd, subcmd)); >>> >>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks >>> in both the top level and in submodule--helper (i.e. revisiting my >>> complaints from [1]), but I haven't sent it out yet because I haven't >>> found the right way to motivate that change. Feel free to chime in on >>> that series when it comes out. >>> >>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/ >> >> Maybe you have different plans, but keep at the WIP re-roll of what I >> have after this, I've also removed all of that. >> >> Basically, ending up with: >> >> --- a/builtin.h >> +++ b/builtin.h >> @@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix); >> int cmd_status(int argc, const char **argv, const char *prefix); >> int cmd_stash(int argc, const char **argv, const char *prefix); >> int cmd_stripspace(int argc, const char **argv, const char *prefix); >> +int cmd_submodule(int argc, const char **argv, const char *prefix); >> int cmd_submodule__helper(int argc, const char **argv, const char *prefix); >> +int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix); >> +int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix); >> +int cmd_submodule_foreach(int argc, const char **argv, const char *prefix); >> +int cmd_submodule_status(int argc, const char **argv, const char *prefix); >> +int cmd_submodule_sync(int argc, const char **argv, const char *prefix); >> +int cmd_submodule_update(int argc, const char **argv, const char *prefix); >> int cmd_switch(int argc, const char **argv, const char *prefix); >> int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); >> int cmd_tag(int argc, const char **argv, const char *prefix); >> >> And changes like: >> >> @@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, >> >> strvec_pushl(&cpr.args, "--super-prefix", NULL); >> strvec_pushf(&cpr.args, "%s/", displaypath); >> - strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive", >> + strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive", >> NULL); >> >> if (info->quiet) >> >> I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but >> when "foreach" needs to recurse it doesn't invoke a "git submodule >> foreach", instead it invokes "git submodule--helper-foreach". >> >> The reason for doing that is that we can't promote the helper to a >> built-in without also leaking implementation detail that we support the >> now internal-only --super-prefix in the command itself. >> >> So this code becomes: >> >> @@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix) >> >> int cmd_submodule__helper(int argc, const char **argv, const char *prefix) >> { >> - const char *cmd = argv[0]; >> - const char *subcmd; >> parse_opt_subcommand_fn *fn = NULL; >> const char *const usage[] = { >> N_("git submodule--helper <command>"), >> NULL >> }; >> struct option options[] = { >> - OPT_SUBCOMMAND("clone", &fn, module_clone), >> - OPT_SUBCOMMAND("add", &fn, module_add), >> - OPT_SUBCOMMAND("update", &fn, module_update), >> - OPT_SUBCOMMAND("foreach", &fn, module_foreach), >> - OPT_SUBCOMMAND("init", &fn, module_init), >> - OPT_SUBCOMMAND("status", &fn, module_status), >> - OPT_SUBCOMMAND("sync", &fn, module_sync), >> - OPT_SUBCOMMAND("deinit", &fn, module_deinit), >> - OPT_SUBCOMMAND("summary", &fn, module_summary), >> - OPT_SUBCOMMAND("push-check", &fn, push_check), >> - OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >> - OPT_SUBCOMMAND("set-url", &fn, module_set_url), >> - OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >> - OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >> + OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check), >> + OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch), >> OPT_END() >> }; >> argc = parse_options(argc, argv, prefix, options, usage, 0); >> - subcmd = argv[0]; >> - >> - if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >> - strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >> - strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >> - get_super_prefix()) >> - /* >> - * xstrfmt() rather than "%s %s" to keep the translated >> - * string identical to git.c's. >> - */ >> - die(_("%s doesn't support --super-prefix"), >> - xstrfmt("'%s %s'", cmd, subcmd)); >> >> return fn(argc, argv, prefix); >> } >> >> I.e. for all the "super-prefix" ones we dispatch directly (and maybe I >> should just do that too for those two stragglers). >> >> It's ugly, but it's only ugly on the inside, but if you can think of a >> better way to do it... > > If we _really_ wanted to keep the check, an alternative might be to > teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I > think this SUPPORT_SUPER_PREFIX check is far more trouble than it's > worth, and I wouldn't want you to spend your time trying to find ways to > keep it only for it to be dropped altogether. > > Here's a snippet from the cover letter I'm working on, which will > hopefully convince you that we don't need to worry about this any more. > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- > > When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make > super-prefix option, 2016-10-07), we specified that commands must prefix paths > by it, and pathspecs must be parsed relative to it. That commit also includes > safeguards to ensure that "--super-prefix" is used correctly, namely: > > - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix". > - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a > non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX > one. > > However, its use is inconsistent, which is a strong reason to consider using > better-scoped flags instead. For example.. > > - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and > "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it > to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have > a documented reason, but since it can be invoked by "read-tree", it's > presumably also a workaround. > - "read-tree" and "submodule--helper" use different values for the super prefix; > "read-tree" passes the path from the root of the superproject's tree to the > submodule's gitlink, while "submodule--helper" passes the relative path of the > original CWD to the submodule. > - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to > display paths. > > This series fixes replaces "--super-prefix" with such better-scoped flags, and > fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check. > > [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26) > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- > > I figured out all the details, but it would look something like: > > - Add an internal-only "--tree-super-prefix" flag to "git read-tree", > which sets a global variable that is read from unpack-trees.c. > - Add an internal-only "--wt-super-prefix" flag to "git > submodule--helper", which sets a global variable that is read from > submodule.c. Unlike "--super-prefix", this won't be gated behind a > SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every > subcommand is using this "--wt-super-prefix" correctly, so we can just > make sure that new subcommands do too. > - Remove the global "--super-prefix", the corresponding env var and > SUPPORT_SUPER_PREFIX. Yes, I'm very much on the same page with removing it eventully. But as you note above that'll take some doing. So for the "submodule as a built-in" I didn't want to take it hostage to that, but just to ensure that I: * Got it working * Did not implicitly leak "this needs a --super-prefix" from the current set of commands to any other commands, or to e.g. public "submodule foreach" just because we need it for the internal-only helper. Are you proposing that you'll submit another clean-up topic that the submodule-as-a-builtin will need to be queued on top of, in addition to this topic? If so I'd really prefer we defer those cleanups, which should also be much easier once submodule's fully in C, unless they're much more trivial than I'm currently suspecting they are...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Fri, Nov 04 2022, Glen Choo wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> On Thu, Nov 03 2022, Glen Choo wrote: >>> >>>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>>> [...] >>>>> + struct option options[] = { >>>>> + OPT_SUBCOMMAND("clone", &fn, module_clone), >>>>> + OPT_SUBCOMMAND("add", &fn, module_add), >>>>> + OPT_SUBCOMMAND("update", &fn, module_update), >>>>> + OPT_SUBCOMMAND("foreach", &fn, module_foreach), >>>>> + OPT_SUBCOMMAND("init", &fn, module_init), >>>>> + OPT_SUBCOMMAND("status", &fn, module_status), >>>>> + OPT_SUBCOMMAND("sync", &fn, module_sync), >>>>> + OPT_SUBCOMMAND("deinit", &fn, module_deinit), >>>>> + OPT_SUBCOMMAND("summary", &fn, module_summary), >>>>> + OPT_SUBCOMMAND("push-check", &fn, push_check), >>>>> + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >>>>> + OPT_SUBCOMMAND("set-url", &fn, module_set_url), >>>>> + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >>>>> + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >>>>> + OPT_END() >>>>> + }; >>>>> + argc = parse_options(argc, argv, prefix, options, usage, 0); >>>>> + subcmd = argv[0]; >>>>> + >>>>> + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >>>>> + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >>>>> + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >>>>> + get_super_prefix()) >>>>> + /* >>>>> + * xstrfmt() rather than "%s %s" to keep the translated >>>>> + * string identical to git.c's. >>>>> + */ >>>>> + die(_("%s doesn't support --super-prefix"), >>>>> + xstrfmt("'%s %s'", cmd, subcmd)); >>>> >>>> FYI I'm preparing a series to get rid of the SUPPORT_SUPER_PREFIX checks >>>> in both the top level and in submodule--helper (i.e. revisiting my >>>> complaints from [1]), but I haven't sent it out yet because I haven't >>>> found the right way to motivate that change. Feel free to chime in on >>>> that series when it comes out. >>>> >>>> [1] https://lore.kernel.org/git/20220630221147.45689-5-chooglen@google.com/ >>> >>> Maybe you have different plans, but keep at the WIP re-roll of what I >>> have after this, I've also removed all of that. >>> >>> Basically, ending up with: >>> >>> --- a/builtin.h >>> +++ b/builtin.h >>> @@ -224,7 +224,14 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix); >>> int cmd_status(int argc, const char **argv, const char *prefix); >>> int cmd_stash(int argc, const char **argv, const char *prefix); >>> int cmd_stripspace(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule(int argc, const char **argv, const char *prefix); >>> int cmd_submodule__helper(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule__helper_clone(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule_absorbgitdirs(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule_foreach(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule_status(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule_sync(int argc, const char **argv, const char *prefix); >>> +int cmd_submodule_update(int argc, const char **argv, const char *prefix); >>> int cmd_switch(int argc, const char **argv, const char *prefix); >>> int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); >>> int cmd_tag(int argc, const char **argv, const char *prefix); >>> >>> And changes like: >>> >>> @@ -366,7 +365,7 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, >>> >>> strvec_pushl(&cpr.args, "--super-prefix", NULL); >>> strvec_pushf(&cpr.args, "%s/", displaypath); >>> - strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive", >>> + strvec_pushl(&cpr.args, "submodule--helper-foreach", "--recursive", >>> NULL); >>> >>> if (info->quiet) >>> >>> I.e. when we call "foreach" we dispatch to cmd_submodule_foreach(), but >>> when "foreach" needs to recurse it doesn't invoke a "git submodule >>> foreach", instead it invokes "git submodule--helper-foreach". >>> >>> The reason for doing that is that we can't promote the helper to a >>> built-in without also leaking implementation detail that we support the >>> now internal-only --super-prefix in the command itself. >>> >>> So this code becomes: >>> >>> @@ -3352,43 +3304,17 @@ static int module_add(int argc, const char **argv, const char *prefix) >>> >>> int cmd_submodule__helper(int argc, const char **argv, const char *prefix) >>> { >>> - const char *cmd = argv[0]; >>> - const char *subcmd; >>> parse_opt_subcommand_fn *fn = NULL; >>> const char *const usage[] = { >>> N_("git submodule--helper <command>"), >>> NULL >>> }; >>> struct option options[] = { >>> - OPT_SUBCOMMAND("clone", &fn, module_clone), >>> - OPT_SUBCOMMAND("add", &fn, module_add), >>> - OPT_SUBCOMMAND("update", &fn, module_update), >>> - OPT_SUBCOMMAND("foreach", &fn, module_foreach), >>> - OPT_SUBCOMMAND("init", &fn, module_init), >>> - OPT_SUBCOMMAND("status", &fn, module_status), >>> - OPT_SUBCOMMAND("sync", &fn, module_sync), >>> - OPT_SUBCOMMAND("deinit", &fn, module_deinit), >>> - OPT_SUBCOMMAND("summary", &fn, module_summary), >>> - OPT_SUBCOMMAND("push-check", &fn, push_check), >>> - OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), >>> - OPT_SUBCOMMAND("set-url", &fn, module_set_url), >>> - OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), >>> - OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), >>> + OPT_SUBCOMMAND("push-check", &fn, cmd_submodule_push_check), >>> + OPT_SUBCOMMAND("create-branch", &fn, cmd_submodule_create_branch), >>> OPT_END() >>> }; >>> argc = parse_options(argc, argv, prefix, options, usage, 0); >>> - subcmd = argv[0]; >>> - >>> - if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && >>> - strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && >>> - strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && >>> - get_super_prefix()) >>> - /* >>> - * xstrfmt() rather than "%s %s" to keep the translated >>> - * string identical to git.c's. >>> - */ >>> - die(_("%s doesn't support --super-prefix"), >>> - xstrfmt("'%s %s'", cmd, subcmd)); >>> >>> return fn(argc, argv, prefix); >>> } >>> >>> I.e. for all the "super-prefix" ones we dispatch directly (and maybe I >>> should just do that too for those two stragglers). >>> >>> It's ugly, but it's only ugly on the inside, but if you can think of a >>> better way to do it... >> >> If we _really_ wanted to keep the check, an alternative might be to >> teach the subcommand parser about SUPPORT_SUPER_PREFIX. But frankly, I >> think this SUPPORT_SUPER_PREFIX check is far more trouble than it's >> worth, and I wouldn't want you to spend your time trying to find ways to >> keep it only for it to be dropped altogether. >> >> Here's a snippet from the cover letter I'm working on, which will >> hopefully convince you that we don't need to worry about this any more. >> >> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- >> >> When we introduced the internal-only "--super-prefix" in 74866d7579 (git: make >> super-prefix option, 2016-10-07), we specified that commands must prefix paths >> by it, and pathspecs must be parsed relative to it. That commit also includes >> safeguards to ensure that "--super-prefix" is used correctly, namely: >> >> - Only commands marked SUPPORT_SUPER_PREFIX can be invoked with "--super-prefix". >> - The super prefix is propagated via the GIT_INTERNAL_SUPER_PREFIX env var, so a >> non-SUPPORT_SUPER_PREFIX command cannot be invoked by a SUPPORT_SUPER_PREFIX >> one. >> >> However, its use is inconsistent, which is a strong reason to consider using >> better-scoped flags instead. For example.. >> >> - Of the 4 commands that are SUPPORT_SUPER_PREFIX, only "read-tree" and >> "submodule--helper" do anything useful with it. "fsmonitor--daemon" has it >> to avoid the SUPPORT_SUPER_PREFIX warning [1]. "checkout--worker" doesn't have >> a documented reason, but since it can be invoked by "read-tree", it's >> presumably also a workaround. >> - "read-tree" and "submodule--helper" use different values for the super prefix; >> "read-tree" passes the path from the root of the superproject's tree to the >> submodule's gitlink, while "submodule--helper" passes the relative path of the >> original CWD to the submodule. >> - "submodule--helper" doesn't use "--super-prefix" to parse pathspecs, only to >> display paths. >> >> This series fixes replaces "--super-prefix" with such better-scoped flags, and >> fixes some bugs resulting from the SUPPORT_SUPER_PREFIX check. >> >> [1] 53fcfbc84f (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26) >> >> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- >> >> I figured out all the details, but it would look something like: >> >> - Add an internal-only "--tree-super-prefix" flag to "git read-tree", >> which sets a global variable that is read from unpack-trees.c. >> - Add an internal-only "--wt-super-prefix" flag to "git >> submodule--helper", which sets a global variable that is read from >> submodule.c. Unlike "--super-prefix", this won't be gated behind a >> SUPPORT_SUPER_PREFIX for each subcommand, since AFAICT, every >> subcommand is using this "--wt-super-prefix" correctly, so we can just >> make sure that new subcommands do too. >> - Remove the global "--super-prefix", the corresponding env var and >> SUPPORT_SUPER_PREFIX. > So for the "submodule as a built-in" I didn't want to take it hostage to > that That's a reasonable concern. > Are you proposing that you'll submit another clean-up topic that the > submodule-as-a-builtin will need to be queued on top of, in addition to > this topic? > > If so I'd really prefer we defer those cleanups, which should also be > much easier once submodule's fully in C, unless they're much more > trivial than I'm currently suspecting they are... Yes, that is what I'm proposing (unfortunately). I don't like having either of us blocked on the other's work, but it seems like it really does work out better this way, e.g. this series makes it much easier to replace "git --super-prefix" with "git submodule--helper --something-else" because we now use the options parser, and removing the "--super-prefix" check makes the eventual conversion simpler because that's one less behavior to worry about keeping. We can see if my proposal makes sense when I send something out (possibly as RFC). I'll do that today/tomorrow.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 2012ad31d7f..0bc25dcf5ae 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -3350,47 +3350,45 @@ static int module_add(int argc, const char **argv, const char *prefix) return ret; } -#define SUPPORT_SUPER_PREFIX (1<<0) - -struct cmd_struct { - const char *cmd; - int (*fn)(int, const char **, const char *); - unsigned option; -}; - -static struct cmd_struct commands[] = { - {"clone", module_clone, SUPPORT_SUPER_PREFIX}, - {"add", module_add, 0}, - {"update", module_update, SUPPORT_SUPER_PREFIX}, - {"foreach", module_foreach, SUPPORT_SUPER_PREFIX}, - {"init", module_init, 0}, - {"status", module_status, SUPPORT_SUPER_PREFIX}, - {"sync", module_sync, SUPPORT_SUPER_PREFIX}, - {"deinit", module_deinit, 0}, - {"summary", module_summary, 0}, - {"push-check", push_check, 0}, - {"absorbgitdirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX}, - {"set-url", module_set_url, 0}, - {"set-branch", module_set_branch, 0}, - {"create-branch", module_create_branch, 0}, -}; - int cmd_submodule__helper(int argc, const char **argv, const char *prefix) { - int i; - if (argc < 2 || !strcmp(argv[1], "-h")) - usage("git submodule--helper <command>"); - - for (i = 0; i < ARRAY_SIZE(commands); i++) { - if (!strcmp(argv[1], commands[i].cmd)) { - if (get_super_prefix() && - !(commands[i].option & SUPPORT_SUPER_PREFIX)) - die(_("%s doesn't support --super-prefix"), - commands[i].cmd); - return commands[i].fn(argc - 1, argv + 1, prefix); - } - } + const char *cmd = argv[0]; + const char *subcmd; + parse_opt_subcommand_fn *fn = NULL; + const char *const usage[] = { + N_("git submodule--helper <command>"), + NULL + }; + struct option options[] = { + OPT_SUBCOMMAND("clone", &fn, module_clone), + OPT_SUBCOMMAND("add", &fn, module_add), + OPT_SUBCOMMAND("update", &fn, module_update), + OPT_SUBCOMMAND("foreach", &fn, module_foreach), + OPT_SUBCOMMAND("init", &fn, module_init), + OPT_SUBCOMMAND("status", &fn, module_status), + OPT_SUBCOMMAND("sync", &fn, module_sync), + OPT_SUBCOMMAND("deinit", &fn, module_deinit), + OPT_SUBCOMMAND("summary", &fn, module_summary), + OPT_SUBCOMMAND("push-check", &fn, push_check), + OPT_SUBCOMMAND("absorbgitdirs", &fn, absorb_git_dirs), + OPT_SUBCOMMAND("set-url", &fn, module_set_url), + OPT_SUBCOMMAND("set-branch", &fn, module_set_branch), + OPT_SUBCOMMAND("create-branch", &fn, module_create_branch), + OPT_END() + }; + argc = parse_options(argc, argv, prefix, options, usage, 0); + subcmd = argv[0]; + + if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") && + strcmp(subcmd, "foreach") && strcmp(subcmd, "status") && + strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") && + get_super_prefix()) + /* + * xstrfmt() rather than "%s %s" to keep the translated + * string identical to git.c's. + */ + die(_("%s doesn't support --super-prefix"), + xstrfmt("'%s %s'", cmd, subcmd)); - die(_("'%s' is not a valid submodule--helper " - "subcommand"), argv[1]); + return fn(argc, argv, prefix); } diff --git a/git.c b/git.c index ee7758dcb0e..fb69e605912 100644 --- a/git.c +++ b/git.c @@ -610,7 +610,7 @@ static struct cmd_struct commands[] = { { "stash", cmd_stash, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, - { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT }, + { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX }, { "switch", cmd_switch, RUN_SETUP | NEED_WORK_TREE }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, { "tag", cmd_tag, RUN_SETUP | DELAY_PAGER_CONFIG },
Have the cmd_submodule__helper() use the OPT_SUBCOMMAND() API introduced in fa83cc834da (parse-options: add support for parsing subcommands, 2022-08-19). This is only a marginal reduction in line count, but once we start unifying this with a yet-to-be-added "builtin/submodule.c" it'll be much easier to reason about those changes, as they'll both use OPT_SUBCOMMAND(). We don't need to worry about "argv[0]" being NULL in the die() because we'd have errored out in parse_options() as we're not using "PARSE_OPT_SUBCOMMAND_OPTIONAL". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 78 ++++++++++++++++++------------------- git.c | 2 +- 2 files changed, 39 insertions(+), 41 deletions(-)