Re: [PATCH v6 04/10] submodule: introduce extensions.submodulePathConfig

[Patrick Steinhardt <ps@pks.im>]

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.<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.

> 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..."?

> 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.<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