From: Adrian Ratiu Date: Tue, 16 Dec 2025 09:45:17 GMT Subject: Re: [PATCH v6 04/10] submodule: introduce extensions.submodulePathConfig Message-ID: <874ipqhiaa.fsf@collabora.com> In-Reply-To: On Tue, 16 Dec 2025, Patrick Steinhardt wrote: > On Sat, Dec 13, 2025 at 10:08:10AM +0200, Adrian Ratiu wrote: >> diff --git a/Documentation/config/submodule.adoc b/Documentation/config/submodule.adoc >> index 0672d99117..4cf7424cda 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 . It only works when >> + `extensions.submodulePathConfig` is enabled, otherwise it does nothing. > > This reads a tiny bit awkward. How about: "This configuration is only > respected when..."? Ack, will fix on the next reroll. >> diff --git a/submodule.c b/submodule.c >> index f645372a18..85ca7ea0fb 100644 >> --- a/submodule.c >> +++ b/submodule.c >> @@ -2570,30 +2570,39 @@ 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); >> + const char *gitdir; >> + char *key; >> + int ret; >> + >> + /* If extensions.submodulePathConfig is disabled, continue to use the plain path */ >> + if (!r->repository_format_submodule_path_cfg) { >> + repo_git_path_append(r, buf, "modules/%s", submodule_name); >> + if (validate_submodule_git_dir(buf->buf, submodule_name) < 0) >> + die(_("refusing to create/use '%s' in another submodule's " >> + "git dir"), buf->buf); >> + >> + return; /* plain gitdir is valid for use */ >> + } >> + >> + /* Extension is enabled: use the gitdir config if it exists */ >> + key = xstrfmt("submodule.%s.gitdir", submodule_name); >> + ret = repo_config_get_string_tmp(r, key, &gitdir); >> + FREE_AND_NULL(key); >> + >> + if (!ret) { >> + strbuf_addstr(buf, gitdir); >> + >> + /* 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, gitdir); >> + >> + return; /* gitdir from config is valid for use */ >> + } >> >> - if (validate_submodule_git_dir(buf->buf, submodule_name) < 0) >> - die(_("refusing to create/use '%s' in another submodule's " >> - "git dir"), buf->buf); >> + 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'"), >> + submodule_name, submodule_name, submodule_name, submodule_name); > > I think the logic would flow a bit more naturally if we didn't have all > the early returns. Something like the following untested and uncompiled > code: > > void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r, > const char *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; > > /* Otherwise, if the extension is enabled, we use the gitdir config. */ > key = xstrfmt("submodule.%s.gitdir", submodule_name); > > if (repo_config_get_string_tmp(r, key, &gitdir)) { > 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'"), > submodule_name, submodule_name, submodule_name, submodule_name); > } > > strbuf_addstr(buf, gitdir); > FREE_AND_NULL(key); > } > > 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, gitdir); > } > } > > I think it would make sense to also hint at the extension in the error > message here so that users know _why_ we expect the key to be set. Ack, will fix in the next reroll.