From: Patrick Steinhardt Date: Tue, 16 Dec 2025 09:09:43 GMT Subject: Re: [PATCH v6 04/10] submodule: introduce extensions.submodulePathConfig Message-ID: In-Reply-To: <20251213080817.347922-5-adrian.ratiu@collabora.com> On Sat, Dec 13, 2025 at 10:08:10AM +0200, Adrian Ratiu wrote: > diff --git a/Documentation/config/extensions.adoc b/Documentation/config/extensions.adoc > index 532456644b..6ce1dcc98b 100644 > --- a/Documentation/config/extensions.adoc > +++ b/Documentation/config/extensions.adoc > @@ -73,6 +73,14 @@ relativeWorktrees::: > repaired with either the `--relative-paths` option or with the > `worktree.useRelativePaths` config set to `true`. > > +submodulePathConfig::: > + If enabled, the submodule..gitdir config is the single source of > + truth for submodule gitdir paths and is always set for new submodules. > + Git will error if a module does not have submodule..gitdir set. > + Existing pre-extension submodules need to be migrated by adding the > + missing config entries. This is done manually for now, e.g. for each > + submodule: "git config submodule..gitdir .git/modules/". > + > worktreeConfig::: > If enabled, then worktrees will load config settings from the > `$GIT_DIR/config.worktree` file in addition to the Yup, makes sense. > 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..."? > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 3bc139ff9c..699ac32004 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -435,6 +435,52 @@ 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); > + > + /* Nothing to do if the config already exists. */ Nit: additional leading space. > +static void create_default_gitdir_config(const char *submodule_name) > +{ > + struct strbuf gitdir_path = STRBUF_INIT; > + > + /* The config is set only when extensions.submodulePathConfig is enabled */ > + if (!the_repository->repository_format_submodule_path_cfg) > + return; > + > + repo_git_path_append(the_repository, &gitdir_path, "modules/%s", submodule_name); > + if (!validate_and_set_submodule_gitdir(&gitdir_path, submodule_name)) { > + strbuf_release(&gitdir_path); > + return; > + } > + > + die(_("failed to set a valid default config for 'submodule.%s.gitdir'. " > + "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); > +} > + > static void init_submodule(const char *path, const char *prefix, > const char *super_prefix, > unsigned int flags) Okay, here we populate the configuration if and only if the repository extension is enabled. Makes sense. > 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. Patrick