From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Tue, 21 Oct 2025 11:57:44 GMT
Subject: Re: [PATCH v3 2/5] submodule: add gitdir path config override
Message-ID: <875xc8qxfr.fsf@collabora.com>
In-Reply-To: <aPc-4FHCpHYgQ9hs@pks.im>

On Tue, 21 Oct 2025, Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Oct 06, 2025 at 02:25:15PM +0300, Adrian Ratiu wrote: 
>> diff --git a/submodule.c b/submodule.c index 
>> 35c55155f7..7a2d7cd592 100644 --- a/submodule.c +++ 
>> b/submodule.c @@ -2604,6 +2604,18 @@ void 
>> submodule_name_to_gitdir(struct strbuf *buf, struct repository 
>> *r, 
>>  	 * administrators can explicitly set. Nothing has been 
>>  decided, * so for now, just append the name at the end of the 
>>  path.  */ 
>> +	char *gitdir_path, *key; + +	/* Allow config 
>> override. */ +	key = xstrfmt("submodule.%s.gitdirpath", 
>> submodule_name); +	if (!repo_config_get_string(r, key, 
>> &gitdir_path)) { +		strbuf_addstr(buf, gitdir_path); + 
>> free(key); +		free(gitdir_path); +		return; + 
>> } +	free(key); + 
>>  	repo_git_path_append(r, buf, "modules/"); 
>>  strbuf_addstr(buf, submodule_name); } 
> 
> You can use `repo_config_get_string_tmp()` to avoid having to 
> manage the `gitdir_path` lifetime. 

Oh, sweet, I wasn't aware of that. Will do, thanks!
 
>> diff --git a/t/lib-verify-submodule-gitdir-path.sh 
>> b/t/lib-verify-submodule-gitdir-path.sh new file mode 100644 
>> index 0000000000..3a83f2d975 --- /dev/null +++ 
>> b/t/lib-verify-submodule-gitdir-path.sh @@ -0,0 +1,20 @@ +# 
>> Helper to verify if repo $1 contains a submodule named $2 with 
>> gitdir path $3 + +# This does not check filesystem 
>> existence. That is done in submodule.c via the +# 
>> submodule_name_to_gitdir() API which this helper ends up 
>> calling. The gitdirs +# might or might not exist (e.g. when 
>> adding a new submodule), so this only +# checks the expected 
>> configuration path, which might be overridden by the user.  + 
>> +verify_submodule_gitdir_path() { +	repo="$1" && +	name="$2" 
>> && +	path="$3" && +	( +		cd "$repo" && + 
>> cat >expect <<-EOF && +			$(git rev-parse 
>> --git-common-dir)/$path +		EOF 
> 
> Style nit: we typically don't indent the heredoc body.

Ack, will fix in v4. 
 
>> +		git submodule--helper gitdir "$name" >actual && + 
>> test_cmp expect actual +	) +} diff --git 
>> a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 
>> fd3e7e355e..11c84a7bdf 100755 --- a/t/t7400-submodule-basic.sh 
>> +++ b/t/t7400-submodule-basic.sh @@ -13,6 +13,7 @@ 
>> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main 
>>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME  . ./test-lib.sh 
>> +. "$TEST_DIRECTORY"/lib-verify-submodule-gitdir-path.sh 
>>   test_expect_success 'setup - enable local submodules' ' git 
>>  config --global protocol.file.allow always 
>> @@ -1505,4 +1506,12 @@ test_expect_success 'submodule add fails 
>> when name is reused' ' 
>>  	) '  
>> +test_expect_success 'submodule helper gitdir config overrides' 
>> ' +	verify_submodule_gitdir_path test-submodule child 
>> modules/child && +	test_config -C test-submodule 
>> submodule.child.gitdirpath ".git/modules/custom-child" && + 
>> verify_submodule_gitdir_path test-submodule child 
>> modules/custom-child && +	test_unconfig -C test-submodule 
>> submodule.child.gitdirpath && + 
>> verify_submodule_gitdir_path test-submodule child modules/child 
>> +' + 
>>  test_done 
> 
> I feel like it's a bit curious that we recognize the 
> configuration even though "extensions.submodulePath" hasn't been 
> introduced yet. I would expect that we ignore the config key if 
> that extension is not set, as the extension otherwise seems to 
> not be doing its job, does it? 

Good point.

In v3 the config wasn't as tied to the extension because it was 
just an optional override, however since we'll make it the 
centerpiece of the design, this 100% makes sense.

I will reorder the commits & rework this logic in v4 as you and 
Junio suggested.

Many thanks,
Adrian