From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 21 Oct 2025 08:05:52 GMT
Subject: Re: [PATCH v3 2/5] submodule: add gitdir path config override
Message-ID: <aPc-4FHCpHYgQ9hs@pks.im>
In-Reply-To: <20251006112518.3764240-3-adrian.ratiu@collabora.com>

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.

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

> +		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?

Patrick