Re: [RFC PATCH 1/4] submodule--helper: teach --toplevel-cwd-prefix
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 9, 2022, 02:37 UTC
- Message-ID
- <221109.867d04rfnt.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20221109004708.97668-2-chooglen@google.com>
On Tue, Nov 08 2022, Glen Choo wrote:
Show 10 quoted lines
> The top-level "--super-prefix" option (i.e. "git --super-prefix") is > overloaded: > > - "git submodule--helper" passes the relative path between the top-level > "git" process's cwd and the current repository > - "git read-tree" passes the path from the root of top-level > superproject's tree to the the current repository. > > In both cases, "--super-prefix" is only used to display meaningful paths > from the superproject to a nested submodule.
The "overloaded" here could really use some elaboration.
Yes, we have different built-in commands that use it, but their use is mostly mutually exclusive, i.e. when "submodule status" recurses into itself it using an option called "--super-prefix" doesn't impact "git read-tree", even though it uses the same option & env variable to communicate the same concept.
But I'm totally willing to buy that it's simpler to e.g. have "git submodule--helper status" or whatever use its own option to carry this forward, rather than some option to "git" itself. Or if some commands just need a --some-option, but not the environment variable.
But this doesn't really address that and ....
Show 6 quoted lines
> Let's address this overloading by breaking it up into its constituent > use cases. Teach "git submodule--helper" the "--toplevel-cwd-prefix" > option, which replaces its usage of "git --super-prefix". (A future > patch will address the "git read-tree" use case.) This value is only > used in builtin/submodule--helper.c, but it is stored in submodule.c, > since it may be needed by other builtins in the future.
...Uh, other built-ins might use it in the future? :) Wouldn't we be right back to a --super-prefix then, which is a cross-built-in semi-standard way to communicate this sort of information? So instead of a hypothetical current:
git submodule some-subcommand => git --super-prefix submodule/ submodule -C submodule/ some-subcommand => git --super-prefix submodule/ fetch
Or whatever, we'd do what? Have each of the now split-up commands take their own flag for this? E.g.:
git submodule some-subcommand => git submodule -C submodule/ --toplevel-cwd-prefix submodule/ some-subcommand => git fetch --submodule-prefix submodule/
Or are you really trying to say that it was a hassle to pass down this through the various functions to all the callers, so emulating what we did with the previous global was easier if it was a new global? I.e. it's not about passing it to other built-ins, but between submodule--helper itself?
I think doing that and chipping away at this from the bottom-up would be a much better approach, at least for some cases.
E.g. just to have "absorbgitdirs" itself support a "--super-prefix" option, not "git" or "git submodule--helper".
Then we just pass that down to absorb_git_dir_into_superproject(), which will only invoke "git submodule--helper absorbgitdirs", which we'll then tell about the super-prefix with a "--super-prefix" option to that subcommand.
Show 29 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c75e9e86b0..acb838e4d6 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -115,18 +115,18 @@ static char *resolve_relative_url(const char *rel_url, const char *up_path, int
> /* the result should be freed by the caller. */
> static char *get_submodule_displaypath(const char *path, const char *prefix)
> {
> - const char *super_prefix = get_super_prefix();
> + const char *toplevel_cwd_prefix = get_toplevel_cwd_prefix();
>
> - if (prefix && super_prefix) {
> - BUG("cannot have prefix '%s' and superprefix '%s'",
> - prefix, super_prefix);
> + if (prefix && toplevel_cwd_prefix) {
> + BUG("cannot have prefix '%s' and toplevel_cwd_prefix '%s'",
> + prefix, toplevel_cwd_prefix);
> } else if (prefix) {
> struct strbuf sb = STRBUF_INIT;
> char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> strbuf_release(&sb);
> return displaypath;
> - } else if (super_prefix) {
> - return xstrfmt("%s%s", super_prefix, path);
> + } else if (toplevel_cwd_prefix) {
> + return xstrfmt("%s%s", toplevel_cwd_prefix, path);
> } else {
> return xstrdup(path);
> }At the end of this series git.c has stopped knowing about "--super-prefix", so I don't see why we need the churn of renaming this. Even if we just make built-in commands take it now, isn't that sufficient? We'd still need...
Show 12 quoted lines
> @@ -364,9 +364,10 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, > cpr.dir = path; > prepare_submodule_repo_env(&cpr.env); > > - strvec_pushl(&cpr.args, "--super-prefix", NULL); > + strvec_pushl(&cpr.args, "submodule--helper", > + "--toplevel-cwd-prefix", NULL); > strvec_pushf(&cpr.args, "%s/", displaypath); > - strvec_pushl(&cpr.args, "submodule--helper", "foreach", "--recursive", > + strvec_pushl(&cpr.args, "foreach", "--recursive", > NULL); >
...to do this, but not all the function & variable renames.
Show 30 quoted lines
> struct option options[] = {
> + OPT_CALLBACK_F(0, "toplevel-cwd-prefix", NULL, "path",
> + "path from top level cwd to working tree root",
> + 0, option_parse_toplevel_cwd_prefix),
> OPT_SUBCOMMAND("clone", &fn, module_clone),
> OPT_SUBCOMMAND("add", &fn, module_add),
> OPT_SUBCOMMAND("update", &fn, module_update),
> @@ -3375,21 +3378,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> 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));
>
> return fn(argc, argv, prefix);
> }Re some of the things I was aiming for in the WIP submodule built-in series by splitting up "submodule--helper", before this we'll only accept it for certain subcommands, e.g.:
$ ./git --super-prefix=blah/ submodule--helper summary fatal: 'submodule--helper summary' doesn't support --super-prefix
Whereas now we'll accept it:
$ ./git submodule--helper --toplevel-cwd-prefix=blah/ summary $
I'm not necessarily against that, but it does seem to run somewhat counter to the divide-and-conquer of eventually getting rid of this super-prefix (or whatever we call it) business.
And in any case, if that's what we're going to do let's loosen that up and justify that change in its own commit, separate from a rename/refactor. I.e. you could start by removing this strcmp(...) && strcmp(...), along with an explanation for why we won't need to care about which "submodule--helper" subcommand needs it anymore.
Show 28 quoted lines
> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
> index 2859695c6d..a9efebc7ec 100755
> --- a/t/t7412-submodule-absorbgitdirs.sh
> +++ b/t/t7412-submodule-absorbgitdirs.sh
> @@ -54,15 +54,24 @@ test_expect_success 'setup nested submodule' '
> '
>
> test_expect_success 'absorb the git dir in a nested submodule' '
> + # Touch the files so that they show up in git status
> + >expect.err &&
> + >actual.err &&
> git status >expect.1 &&
> git -C sub1/nested rev-parse HEAD >expect.2 &&
> - git submodule absorbgitdirs &&
> + git submodule absorbgitdirs 2>actual.err &&
> test -f sub1/nested/.git &&
> test -d .git/modules/sub1/modules/nested &&
> git status >actual.1 &&
> git -C sub1/nested rev-parse HEAD >actual.2 &&
> test_cmp expect.1 actual.1 &&
> - test_cmp expect.2 actual.2
> + test_cmp expect.2 actual.2 &&
> + cat >expect.err <<-EOF &&
> + Migrating git directory of ${SQ}sub1/nested${SQ} from
> + ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/sub1/nested/.git${SQ} to
> + ${SQ}/Users/chooglen/Code/git/t/trash directory.t7412-submodule-absorbgitdirs/.git/modules/sub1/modules/nested${SQ}
> + EOF
> + test_cmp expect.err actual.errI'm afraid I'll need to change my name & OS for that to pass for me :)
This reminded me to send in: https://lore.kernel.org/git/patch-1.1-34b54fdd9bb-20221109T020347Z-avarab@gmail.com/ ; which fixes this message, & adds more tests for this output.