git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/8] submodule--helper: use OPT_SUBCOMMAND() API

From
Glen Choo <chooglen@google.com>
Date
Nov 4, 2022, 17:02 UTC
Message-ID
<kl6l8rkqy7no.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<221104.86wn8bzeus.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 140 quoted lines
> 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.
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 18 of 35 in “submodule: tests, cleanup to prepare for built-in”
  1. 0/8 submodule: tests, cleanup to prepare for built-inÆvar Arnfjörð Bjarmason, Nov 2, 2022
  2. 1/8 submodule--helper: move "config" to a test-toolÆvar Arnfjörð Bjarmason, Nov 2, 2022
  3. Glen ChooNov 3, 2022
  4. 2/8 submodule tests: add tests for top-level flag outputÆvar Arnfjörð Bjarmason, Nov 2, 2022
  5. Glen ChooNov 3, 2022
  6. 3/8 submodule tests: test for a "foreach" blind-spotÆvar Arnfjörð Bjarmason, Nov 2, 2022
  7. 4/8 submodule.c: refactor recursive block out of absorb functionÆvar Arnfjörð Bjarmason, Nov 2, 2022
  8. 5/8 submodule API & "absorbgitdirs": remove "----recursive" optionÆvar Arnfjörð Bjarmason, Nov 2, 2022
  9. Glen ChooNov 3, 2022
  10. Ævar Arnfjörð BjarmasonNov 4, 2022
  11. FW: [PATCH 5/8] submodule API & "absorbgitdirs": remove "----recursive" optionSimpson, Phyllis, Nov 4, 2022
  12. Glen ChooNov 4, 2022
  13. 6/8 submodule--helper: remove --prefix from "absorbgitdirs"Ævar Arnfjörð Bjarmason, Nov 2, 2022
  14. 7/8 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"Ævar Arnfjörð Bjarmason, Nov 2, 2022
  15. 8/8 submodule--helper: use OPT_SUBCOMMAND() APIÆvar Arnfjörð Bjarmason, Nov 2, 2022
  16. Glen ChooNov 3, 2022
  17. Ævar Arnfjörð BjarmasonNov 4, 2022
  18. Glen ChooNov 4, 2022
  19. Ævar Arnfjörð BjarmasonNov 5, 2022
  20. Glen ChooNov 7, 2022
  21. Glen ChooNov 4, 2022
  22. Taylor BlauNov 4, 2022
  23. 0/9 submodule: tests, cleanup to prepare for built-inÆvar Arnfjörð Bjarmason, Nov 8, 2022
  24. 1/9 submodule--helper: move "config" to a test-toolÆvar Arnfjörð Bjarmason, Nov 8, 2022
  25. 2/9 submodule tests: add tests for top-level flag outputÆvar Arnfjörð Bjarmason, Nov 8, 2022
  26. 3/9 submodule--helper: fix a memory leak in "status"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  27. 5/9 submodule.c: refactor recursive block out of absorb functionÆvar Arnfjörð Bjarmason, Nov 8, 2022
  28. 6/9 submodule API & "absorbgitdirs": remove "----recursive" optionÆvar Arnfjörð Bjarmason, Nov 8, 2022
  29. 7/9 submodule--helper: remove --prefix from "absorbgitdirs"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  30. 4/9 submodule tests: test for a "foreach" blind-spotÆvar Arnfjörð Bjarmason, Nov 8, 2022
  31. 8/9 submodule--helper: drop "update --prefix <pfx>" for "-C <pfx> update"Ævar Arnfjörð Bjarmason, Nov 8, 2022
  32. 9/9 submodule--helper: use OPT_SUBCOMMAND() APIÆvar Arnfjörð Bjarmason, Nov 8, 2022
  33. Glen ChooNov 8, 2022
  34. Ævar Arnfjörð BjarmasonNov 8, 2022
  35. Glen ChooNov 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.