Re: [PATCH v3 2/5] submodule: add gitdir path config override

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

[jc: brandon removed from CC list as the address would bounce]

> This adds the ability to override gitdir paths via config files
> (not .gitmodules) such that the encoding scheme (or plain text
> name if the encoding extension is disabled) can be changed via
> config entries.
>
> These entries are not added by default for all submodules: they
> should be used on an as-needed basis.
>
> A new test and a helper are added. The helper will also be used
> in further tests exercising gitdir encoding functionality.

What is the use case of this?  The only reasonable use case I can
see is to set this to all the existing submodules when you are
switching the extension on before adding a new submodule, in which
case the old ones will keep using unencoded names, while the new
ones will use encoded ones.  But is that a sensible thing to do?
How would we guarantee that existing submodules' vanilla names would
not collide with encoded submodules' names?  We haven't seen the
encoded names yet, but I think I saw some mention of URL encoding.

So if I had a submodule whose name is "%41%42%43", set this
configuration because I do not want it treated as URL-encoded, then
enable the extension, and then later add a separate submodule whose
name is "ABC", which may be encoded (remember use of "ABC" here is
only for illustration; replace it with something that do need
encoding if you want a more realistic example) to the same
"%41%42%43".

If that kind of situation is what this new configuration allows, I
do not quite see why it is a good idea to have such a thing.

> Based-on-patch-by: Brandon Williams <bmwill@google.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  Documentation/config/submodule.adoc   |  4 ++++
>  builtin/submodule--helper.c           | 17 +++++++++++++++++
>  submodule.c                           | 12 ++++++++++++
>  t/lib-verify-submodule-gitdir-path.sh | 20 ++++++++++++++++++++
>  t/t7400-submodule-basic.sh            |  9 +++++++++
>  t/t9902-completion.sh                 |  1 +
>  6 files changed, 63 insertions(+)
>  create mode 100644 t/lib-verify-submodule-gitdir-path.sh
>
> diff --git a/Documentation/config/submodule.adoc b/Documentation/config/submodule.adoc
> index 0672d99117..8f64adfbe3 100644
> --- a/Documentation/config/submodule.adoc
> +++ b/Documentation/config/submodule.adoc
> @@ -52,6 +52,10 @@ submodule.<name>.active::
>  	submodule.active config option. See linkgit:gitsubmodules[7] for
>  	details.
>  
> +submodule.<name>.gitdir::
> +	This option sets the gitdir path for submodule <name>, allowing users
> +	to override the default path or change the default path name encoding.
> +
>  submodule.active::
>  	A repeated field which contains a pathspec used to match against a
>  	submodule's path to determine if the submodule is of interest to git
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2873b2780e..abd20eee53 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1208,6 +1208,22 @@ static int module_summary(int argc, const char **argv, const char *prefix,
>  	return ret;
>  }
>  
> +static int module_gitdir(int argc, const char **argv, const char *prefix UNUSED,
> +			 struct repository *repo)
> +{
> +	struct strbuf gitdir = STRBUF_INIT;
> +
> +	if (argc != 2)
> +		usage(_("git submodule--helper gitdir <name>"));
> +
> +	submodule_name_to_gitdir(&gitdir, repo, argv[1]);
> +
> +	printf("%s\n", gitdir.buf);
> +
> +	strbuf_release(&gitdir);
> +	return 0;
> +}
> +
>  struct sync_cb {
>  	const char *prefix;
>  	const char *super_prefix;
> @@ -3591,6 +3607,7 @@ int cmd_submodule__helper(int argc,
>  		NULL
>  	};
>  	struct option options[] = {
> +		OPT_SUBCOMMAND("gitdir", &fn, module_gitdir),
>  		OPT_SUBCOMMAND("clone", &fn, module_clone),
>  		OPT_SUBCOMMAND("add", &fn, module_add),
>  		OPT_SUBCOMMAND("update", &fn, module_update),
> 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);
>  }
> 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
> +		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
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 964e1f1569..ffb9c8b522 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -3053,6 +3053,7 @@ test_expect_success 'git config set - variable name - __git_compute_second_level
>  	submodule.sub.fetchRecurseSubmodules Z
>  	submodule.sub.ignore Z
>  	submodule.sub.active Z
> +	submodule.sub.gitdir Z
>  	EOF
>  '