From: Junio C Hamano Date: Mon, 06 Oct 2025 16:47:56 GMT Subject: Re: [PATCH v3 2/5] submodule: add gitdir path config override Message-ID: In-Reply-To: <20251006112518.3764240-3-adrian.ratiu@collabora.com> Adrian Ratiu 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 > Signed-off-by: Adrian Ratiu > --- > 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..active:: > submodule.active config option. See linkgit:gitsubmodules[7] for > details. > > +submodule..gitdir:: > + This option sets the gitdir path for submodule , 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 ")); > + > + 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 > '