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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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...