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

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Tue, 16 Dec 2025, Patrick Steinhardt <ps@pks.im> 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.<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..."?

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

Ack, will fix in the next reroll.