Re: [PATCH v6 04/10] submodule: introduce extensions.submodulePathConfig
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 16, 2025, 09:09 UTC
- Message-ID
- <aUEh14H242nm0NcE@pks.im>
- In-Reply-To
- <20251213080817.347922-5-adrian.ratiu@collabora.com>
On Sat, Dec 13, 2025 at 10:08:10AM +0200, Adrian Ratiu wrote:
Show 19 quoted lines
> 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.<name>.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.<name>.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.<name>.gitdir .git/modules/<name>". > + > worktreeConfig::: > If enabled, then worktrees will load config settings from the > `$GIT_DIR/config.worktree` file in addition to the
Yup, makes sense.
Show 11 quoted lines
> 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.<name>.active:: > submodule.active config option. See linkgit:gitsubmodules[7] for > details. > > +submodule.<name>.gitdir:: > + This sets the gitdir path for submodule <name>. 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..."?
Show 20 quoted lines
> 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.
Show 23 quoted lines
> +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.
Show 68 quoted lines
> 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.<name>.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.<name>.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