Re: [PATCH v5 4/7] submodule: add extension to encode gitdir paths

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 19, 2025 at 11:10:27PM +0200, Adrian Ratiu wrote:
> Add a submoduleEncoding extension which fixes filesystem collisions by
> encoding gitdir paths. At a high level, this implements a mechanism to
> encode -> validate -> retry until a working gitdir path is found.
> 
> Credit goes to Junio for coming up with this design: encoding is only
> applied when necessary, e.g. uppercase characters are encoded only on
> case-folding filesystems and only if a real conflict is detected.
> 
> To make this work, we rely on the submodule.<name>.gitdir config as the
> single source of truth for gitidir paths: the config is always set when

s/gitidir/gitdir/

> diff --git a/Documentation/config/extensions.adoc b/Documentation/config/extensions.adoc
> index 532456644b..4861d01894 100644
> --- a/Documentation/config/extensions.adoc
> +++ b/Documentation/config/extensions.adoc
> @@ -73,6 +73,12 @@ relativeWorktrees:::
>  	repaired with either the `--relative-paths` option or with the
>  	`worktree.useRelativePaths` config set to `true`.
>  
> +submoduleEncoding:::
> +	If enabled, submodule gitdir paths are encoded to avoid filesystem
> +	conflicts due to nested gitdirs, case insensitivity or other issues.
> +	When enabled, the submodule.<name>.gitdir config is always set for
> +	all submodules and is the single point of authority for gitdir paths.
> +
>  worktreeConfig:::
>  	If enabled, then worktrees will load config settings from the
>  	`$GIT_DIR/config.worktree` file in addition to the

I think the fact that the submodule gitdir paths are encoded now is
secondary to this repository extension. The more important fact is that
this changes the source of truth where the submodule gitdir path is
actually derived from: before it was derived on the fly, whereas now it
is persisted in the gitconfig.

It follows that because the source of truth is now a persistent entry in
the configuration, other implementations can read it without having to
understand how exactly the value was computed in the first place. So an
implementation may arbitrarily change the algorithm it uses to derive
that path from now on, and it doesn't necessarily have to encode
anything.

So I'd propose to rename the extension and rephrase its description
accordingly. It could for example be called something along the lines of
"submodulePathConfig".

> diff --git a/submodule.c b/submodule.c
> index 8ef028f26b..07cb4694cf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2559,33 +2591,74 @@ int submodule_to_gitdir(struct repository *repo,
>  	return ret;
>  }
>  
> +static int validate_and_set_submodule_gitdir(struct strbuf *gitdir_path,
> +					     const char *submodule_name)
> +{
> +	char *key;
> +
> +	if (validate_submodule_encoded_git_dir(gitdir_path->buf, submodule_name))
> +		return -1;
> +
> +	key = xstrfmt("submodule.%s.gitdir", submodule_name);
> +	repo_config_set_gently(the_repository, key, gitdir_path->buf);
> +	FREE_AND_NULL(key);

I think a simple call to `free()` should be sufficient here. There is no
risk of it being used afterwards.

> +	return 0;
> +}
> +
>  void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,
>  			      const char *submodule_name)
>  {
> +	const char *gitdir;
> +	char *key;
> +
> +	repo_git_path_append(r, buf, "modules/");
> +	strbuf_addstr(buf, submodule_name);
> +
> +	/* If extensions.submoduleEncoding is disabled, use the plain path set above */
> +	if (!r->repository_format_submodule_encoding) {
> +		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);
> +	if (!repo_config_get_string_tmp(r, key, &gitdir)) {
> +		strbuf_reset(buf);
> +		strbuf_addstr(buf, gitdir);
> +		FREE_AND_NULL(key);
> +
> +		/* validate because users might have modified the config */
> +		if (validate_submodule_encoded_git_dir(buf->buf, submodule_name))
> +			die(_("Invalid 'submodule.%s.gitdir' config: '%s' please check "
> +			      "if it is unique or conflicts with another module"),

Nit: error messages start with a lower-case character.

> +			    submodule_name, gitdir);
> +
> +		return;
> +	}
> +	FREE_AND_NULL(key);
> +
>  	/*
> -	 * 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.
> +	 * The gitdir config does not exist, even though the extension is enabled.
> +	 * Therefore we are in one of the following cases:
>  	 */
> +
> +	/* Case 1: legacy migration of valid plain submodule names */
> +	if (!validate_and_set_submodule_gitdir(buf, submodule_name))
> +		return;
> +
> +	/* Case 2: Try URI-safe (RFC3986) encoding first, this fixes nested gitdirs */
> +	strbuf_reset(buf);
>  	repo_git_path_append(r, buf, "modules/");
> -	strbuf_addstr(buf, submodule_name);
> +	strbuf_addstr_urlencode(buf, submodule_name, is_rfc3986_unreserved);
> +	if (!validate_and_set_submodule_gitdir(buf, submodule_name))
> +		return;
>  
> -	if (validate_submodule_git_dir(buf->buf, submodule_name) < 0)
> -		die(_("refusing to create/use '%s' in another submodule's "
> -		      "git dir"), buf->buf);
> +	/* Case 3: Nothing worked: error out */
> +	die(_("Cannot construct a valid gitdir path for submodule '%s': "
> +	      "please set a unique git config for 'submodule.%s.gitdir'."),
> +	    submodule_name, submodule_name);

It feels somewhat fragile to me that we unconditionally handle these
cases and try to find old submodule directories. If the extension is
enabled I'd expect that the submodule configuration is the _only_ source
of truth.

May I propose that we instead always error out in case the submodule
configuration does not exist? In the best case we'd then give the user a
nice error message that tells them how to run the migration manually.

(Coming back from reading subsequent patches) Maybe what's putting me
off is that this function is seemingly used for two things:

  1. To derive the submodule path in case we know it should already
     exist.

  2. To compute the submodule path so we can end up writing it into the
     "submodule.*.gitdir" variable.

I think we should tell these two cases apart. In the first case I expect
that we never fall back to a computed name, but bail out in case the
configuration key does not exist. And in the second case it of course
makes sense to compute the actual path that we want to store in the
configuration.

Thanks!

Patrick