From: Patrick Steinhardt Date: Tue, 06 Jan 2026 07:25:44 GMT Subject: Re: [PATCH v7 04/11] submodule: introduce extensions.submodulePathConfig Message-ID: In-Reply-To: <20251220101528.1227487-5-adrian.ratiu@collabora.com> On Sat, Dec 20, 2025 at 12:15:21PM +0200, Adrian Ratiu wrote: > diff --git a/Documentation/config/submodule.adoc b/Documentation/config/submodule.adoc > index 0672d99117..9c260a69f6 100644 > --- a/Documentation/config/submodule.adoc > +++ b/Documentation/config/submodule.adoc > @@ -52,6 +52,13 @@ submodule..active:: > submodule.active config option. See linkgit:gitsubmodules[7] for > details. > > +submodule..gitdir:: > + This sets the gitdir path for submodule . This configuration is > + respected when `extensions.submodulePathConfig` is enabled, otherwise it > + has no effect. When enabled, this config becomes the single source of > + truth for submodule gitdir paths and git will error if it is missing. Tiny nit: s/git/Git/ > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 3bc139ff9c..f8cae345a5 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -435,6 +435,48 @@ struct init_cb { > }; > #define INIT_CB_INIT { 0 } > > +static int validate_and_set_submodule_gitdir(struct strbuf *gitdir_path, > + const char *submodule_name) > +{ > + const char *value; > + char *key; > + > + if (validate_submodule_git_dir(gitdir_path->buf, submodule_name)) > + return -1; > + > + key = xstrfmt("submodule.%s.gitdir", submodule_name); Tiny nit: extra space before `key`. > diff --git a/submodule.c b/submodule.c > index f645372a18..e3692009cd 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2570,30 +2571,35 @@ int submodule_to_gitdir(struct repository *repo, > void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r, > const char *submodule_name) > { > - /* > - * NEEDSWORK: The current way of mapping a submodule's name to > - * its location in .git/modules/ has problems with some naming > - * schemes. For example, if a submodule is named "foo" and > - * another is named "foo/bar" (whether present in the same > - * superproject commit or not - the problem will arise if both > - * superproject commits have been checked out at any point in > - * time), or if two submodule names only have different cases in > - * a case-insensitive filesystem. > - * > - * There are several solutions, including encoding the path in > - * some way, introducing a submodule..gitdir config in > - * .git/config (not .gitmodules) that allows overriding what the > - * gitdir of a submodule would be (and teach Git, upon noticing > - * a clash, to automatically determine a non-clashing name and > - * to write such a config), or introducing a > - * submodule..gitdir config in .gitmodules that repo > - * administrators can explicitly set. Nothing has been decided, > - * so for now, just append the name at the end of the path. > - */ > - repo_git_path_append(r, buf, "modules/"); > - strbuf_addstr(buf, submodule_name); > + if (!r->repository_format_submodule_path_cfg) { > + /* > + * If extensions.submodulePathConfig is disabled, > + * continue to use the plain path. > + */ > + repo_git_path_append(r, buf, "modules/%s", submodule_name); > + } else { > + const char *gitdir; > + char *key; > + int ret; > + > + /* Otherwise the extension is enabled, so use the gitdir config. */ > + key = xstrfmt("submodule.%s.gitdir", submodule_name); > + ret = repo_config_get_string_tmp(r, key, &gitdir); > + FREE_AND_NULL(key); > + > + if (ret) > + die(_("the 'submodule.%s.gitdir' config does not exist for module '%s'. " > + "Please ensure it is set, for example by running something like: " > + "'git config submodule.%s.gitdir .git/modules/%s'. For details " > + "see the extensions.submodulePathConfig documentation."), > + submodule_name, submodule_name, submodule_name, submodule_name); > + > + strbuf_addstr(buf, gitdir); > + } > > - if (validate_submodule_git_dir(buf->buf, submodule_name) < 0) > - die(_("refusing to create/use '%s' in another submodule's " > - "git dir"), buf->buf); > + /* validate because users might have modified the config */ > + if (validate_submodule_git_dir(buf->buf, submodule_name)) > + die(_("invalid 'submodule.%s.gitdir' config: '%s' please check " > + "if it is unique or conflicts with another module"), > + submodule_name, buf->buf); > } Nit: this error message may be misleading, as it is also used in the case where the submodule path was derived from its name. I think the original message should be retained, maybe followed by a call to `advice()` that users may wish to enable the extension to fix this in case it's not enabled already. > diff --git a/t/lib-verify-submodule-gitdir-path.sh b/t/lib-verify-submodule-gitdir-path.sh > new file mode 100644 > index 0000000000..62794df976 > --- /dev/null > +++ b/t/lib-verify-submodule-gitdir-path.sh > @@ -0,0 +1,24 @@ > +# Helper to verify if repo $1 contains a submodule named $2 with gitdir path $3 > + > +# This does not check filesystem existence. That is done in submodule.c via the > +# submodule_name_to_gitdir() API which this helper ends up calling. The gitdirs > +# might or might not exist (e.g. when adding a new submodule), so this only > +# checks the expected configuration path, which might be overridden by the user. > + > +verify_submodule_gitdir_path() { Nit: there should be a space between function name and `()`. > diff --git a/t/t7425-submodule-gitdir-path-extension.sh b/t/t7425-submodule-gitdir-path-extension.sh > new file mode 100755 > index 0000000000..5d52a289f8 > --- /dev/null > +++ b/t/t7425-submodule-gitdir-path-extension.sh > @@ -0,0 +1,138 @@ > +#!/bin/sh > + > +test_description='submodulePathConfig extension works as expected' I think I didn't spot any test that verifies the actual config values that get written when the repository extension is enabled. Specifially, what I think we ought to test there is that the generated submodule path is relative to the repository and not an absolute path. > + > +. ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-verify-submodule-gitdir-path.sh > + > +test_expect_success 'setup: allow file protocol' ' > + git config --global protocol.file.allow always > +' > + > +test_expect_success 'create repo with mixed extension submodules' ' > + git init -b main legacy-sub && > + test_commit -C legacy-sub legacy-initial && > + legacy_rev=$(git -C legacy-sub rev-parse HEAD) && > + > + git init -b main new-sub && > + test_commit -C new-sub new-initial && > + new_rev=$(git -C new-sub rev-parse HEAD) && > + > + git init -b main main && > + ( > + cd main && > + git submodule add ../legacy-sub legacy && > + test_commit legacy-sub && > + > + # trigger the "die_path_inside_submodule" check > + test_must_fail git submodule add ../new-sub "legacy/nested" && > + > + git config core.repositoryformatversion 1 && > + git config extensions.submodulePathConfig true && > + > + git submodule add ../new-sub "New Sub" && > + test_commit new && > + > + # retrigger the "die_path_inside_submodule" check with encoding > + test_must_fail git submodule add ../new-sub "New Sub/nested2" > + ) > +' > + > +test_expect_success 'verify new submodule gitdir config' ' > + git -C main config submodule."New Sub".gitdir > actual && > + echo ".git/modules/New Sub" > expect && Nit: we don't typically have a space between ">" and the target file. Also true in other test cases. Patrick