submodule: introduce extensions.submodulePathConfig

The idea of this extension is to abstract away the submodule gitdir
path implementation: everyone is expected to use the config and not
worry about how the path is computed internally, either in git or
other implementations.

With this extension enabled, the submodule.<name>.gitdir repo config
becomes the single source of truth for all submodule gitdir paths.

The submodule.<name>.gitdir config is added automatically for all new
submodules when this extension is enabled.

Git will throw an error if the extension is enabled and a config is
missing, advising users how to migrate. Migration is manual for now.

E.g. to add a missing config entry for an existing "foo" module:
git config submodule.foo.gitdir .git/modules/foo

Suggested-by: Junio C Hamano <gitster@pobox.com>
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/extensions.adoc b/Documentation/config/extensions.adoc
index 532456644b770e7f9a8d15b5eaac9eef143a5315..e15b93f2fb9edd68b5df3b4c41c46a14341e1718 100644 (file)
--- a/Documentation/config/extensions.adoc
+++ b/Documentation/config/extensions.adoc
@@ -73,6 +73,29 @@ relativeWorktrees:::
        repaired with either the `--relative-paths` option or with the
        `worktree.useRelativePaths` config set to `true`.
 
+submodulePathConfig:::
+       This extension is for the minority of users who:
++
+--
+* Encounter errors like        `refusing to create ... in another submodule's git dir`
+  due to a number of reasons, like case-insensitive filesystem conflicts when
+  creating modules named `foo` and `Foo`.
+* Require more flexible submodule layouts, for example due to nested names like
+  `foo`, `foo/bar` and `foo/baz` not supported by the default gitdir mechanism
+  which uses `.git/modules/<plain-name>` locations, causing further conflicts.
+--
++
+When `extensions.submodulePathConfig` is enabled, the `submodule.<name>.gitdir`
+config becomes the single source of truth for all submodule gitdir paths and is
+automatically set for all new submodules both during clone and init operations.
++
+Git will error out if a module does not have a corresponding
+`submodule.<name>.gitdir` set.
++
+Existing (pre-extension) submodules need to be migrated by adding the missing
+config entries. This is done manually for now, e.g. for each submodule:
+`git config submodule.<name>.gitdir .git/modules/<name>`.
+
 worktreeConfig:::
        If enabled, then worktrees will load config settings from the
        `$GIT_DIR/config.worktree` file in addition to the

diff --git a/Documentation/config/submodule.adoc b/Documentation/config/submodule.adoc
index 0672d9911724d184b7b49ee0e50de3e992a0eac5..9c260a69f603612b0d3caa7d0cc5a3525cad36e4 100644 (file)
--- 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>. This configuration is
+       respected when `extensions.submodulePathConfig` is enabled, otherwise it
+       has no effect. When enabled, this config becomes the single source of
+       truth for submodule gitdir paths and git will error if it is missing.
+       See linkgit:git-config[1] for details.
+
 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 7ea82d7fa243e4b6e34512d5b41f5e28184752d2..0d6f5eb27bf170f36747a654f6db273adf88237a 100644 (file)
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -435,6 +435,48 @@ struct init_cb {
 };
 #define INIT_CB_INIT { 0 }
 
+static int validate_and_set_submodule_gitdir(struct strbuf *gitdir_path,
+                                            const char *submodule_name)
+{
+       const char *value;
+       char *key;
+
+       if (validate_submodule_git_dir(gitdir_path->buf, submodule_name))
+               return -1;
+
+        key = xstrfmt("submodule.%s.gitdir", submodule_name);
+
+       /* Nothing to do if the config already exists. */
+       if (!repo_config_get_string_tmp(the_repository, key, &value)) {
+               free(key);
+               return 0;
+       }
+
+       if (repo_config_set_gently(the_repository, key, gitdir_path->buf)) {
+               free(key);
+               return -1;
+       }
+
+       free(key);
+       return 0;
+}
+
+static void create_default_gitdir_config(const char *submodule_name)
+{
+       struct strbuf gitdir_path = STRBUF_INIT;
+
+       repo_git_path_append(the_repository, &gitdir_path, "modules/%s", submodule_name);
+       if (!validate_and_set_submodule_gitdir(&gitdir_path, submodule_name)) {
+               strbuf_release(&gitdir_path);
+               return;
+       }
+
+       die(_("failed to set a valid default config for 'submodule.%s.gitdir'. "
+             "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);
+}
+
 static void init_submodule(const char *path, const char *prefix,
                           const char *super_prefix,
                           unsigned int flags)
@@ -511,6 +553,10 @@ static void init_submodule(const char *path, const char *prefix,
                if (repo_config_set_gently(the_repository, sb.buf, upd))
                        die(_("Failed to register update mode for submodule path '%s'"), displaypath);
        }
+
+       if (the_repository->repository_format_submodule_path_cfg)
+               create_default_gitdir_config(sub->name);
+
        strbuf_release(&sb);
        free(displaypath);
        free(url);
@@ -1805,8 +1851,9 @@ static int clone_submodule(const struct module_clone_data *clone_data,
                char *head = xstrfmt("%s/HEAD", sm_gitdir);
                unlink(head);
                free(head);
-               die(_("refusing to create/use '%s' in another submodule's "
-                     "git dir"), sm_gitdir);
+               die(_("refusing to create/use '%s' in another submodule's git dir. "
+                     "Enabling extensions.submodulePathConfig should fix this."),
+                   sm_gitdir);
        }
 
        connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);
@@ -3578,6 +3625,9 @@ static int module_add(int argc, const char **argv, const char *prefix,
        add_data.progress = !!progress;
        add_data.dissociate = !!dissociate;
 
+       if (the_repository->repository_format_submodule_path_cfg)
+               create_default_gitdir_config(add_data.sm_name);
+
        if (add_submodule(&add_data))
                goto cleanup;
        configure_added_submodule(&add_data);

diff --git a/repository.c b/repository.c
index 6faf5c73981ebf6c520919a5475f0243f08cea87..35a06e6719a7b5b467e893b0b1149c25bfed7417 100644 (file)
--- a/repository.c
+++ b/repository.c
@@ -288,6 +288,7 @@ int repo_init(struct repository *repo,
        repo->repository_format_worktree_config = format.worktree_config;
        repo->repository_format_relative_worktrees = format.relative_worktrees;
        repo->repository_format_precious_objects = format.precious_objects;
+       repo->repository_format_submodule_path_cfg = format.submodule_path_cfg;
 
        /* take ownership of format.partial_clone */
        repo->repository_format_partial_clone = format.partial_clone;

diff --git a/repository.h b/repository.h
index 5808a5d610846a0e42233f66e56dcbcebbd3ecd0..aa907bd1e4d18c70abf53bdb87ffe4d38b33e39c 100644 (file)
--- a/repository.h
+++ b/repository.h
@@ -158,6 +158,7 @@ struct repository {
        int repository_format_worktree_config;
        int repository_format_relative_worktrees;
        int repository_format_precious_objects;
+       int repository_format_submodule_path_cfg;
 
        /* Indicate if a repository has a different 'commondir' from 'gitdir' */
        unsigned different_commondir:1;

diff --git a/setup.c b/setup.c
index 7086741e6c2d1f5c73e484b76e1a1272f7d0697d..207fa36e100b61e0a5f35d04f2e75d39ecf76457 100644 (file)
--- a/setup.c
+++ b/setup.c
@@ -687,6 +687,9 @@ static enum extension_result handle_extension(const char *var,
        } else if (!strcmp(ext, "relativeworktrees")) {
                data->relative_worktrees = git_config_bool(var, value);
                return EXTENSION_OK;
+       } else if (!strcmp(ext, "submodulepathconfig")) {
+               data->submodule_path_cfg = git_config_bool(var, value);
+               return EXTENSION_OK;
        }
        return EXTENSION_UNKNOWN;
 }
@@ -1865,6 +1868,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
                                repo_fmt.worktree_config;
                        the_repository->repository_format_relative_worktrees =
                                repo_fmt.relative_worktrees;
+                       the_repository->repository_format_submodule_path_cfg =
+                               repo_fmt.submodule_path_cfg;
                        /* take ownership of repo_fmt.partial_clone */
                        the_repository->repository_format_partial_clone =
                                repo_fmt.partial_clone;
@@ -1963,6 +1968,8 @@ void check_repository_format(struct repository_format *fmt)
                                    fmt->ref_storage_format);
        the_repository->repository_format_worktree_config =
                fmt->worktree_config;
+       the_repository->repository_format_submodule_path_cfg =
+               fmt->submodule_path_cfg;
        the_repository->repository_format_relative_worktrees =
                fmt->relative_worktrees;
        the_repository->repository_format_partial_clone =

diff --git a/setup.h b/setup.h
index 8522fa8575da7123d0a9dc57dbabf08677a58eaa..568bb9f1d12c3963ef1e502cf64fcded20591849 100644 (file)
--- a/setup.h
+++ b/setup.h
@@ -130,6 +130,7 @@ struct repository_format {
        char *partial_clone; /* value of extensions.partialclone */
        int worktree_config;
        int relative_worktrees;
+       int submodule_path_cfg;
        int is_bare;
        int hash_algo;
        int compat_hash_algo;

diff --git a/submodule.c b/submodule.c
index d937911fbcbd4744be6e655c3d0682fac131ec1a..2e644ec2da2cd84e6a8db291a97e8b3a80653105 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -2164,8 +2164,9 @@ int submodule_move_head(const char *path, const char *super_prefix,
                                if (validate_submodule_git_dir(git_dir,
                                                               sub->name) < 0)
                                        die(_("refusing to create/use '%s' in "
-                                             "another submodule's git dir"),
-                                           git_dir);
+                                             "another submodule's git dir. "
+                                             "Enabling extensions.submodulePathConfig "
+                                             "should fix this."), git_dir);
                                free(git_dir);
                        }
                } else {
@@ -2576,30 +2577,35 @@ cleanup:
 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);
+       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;
+               int ret;
+
+               /* Otherwise the extension is enabled, so use the gitdir config. */
+               key = xstrfmt("submodule.%s.gitdir", submodule_name);
+               ret = repo_config_get_string_tmp(r, key, &gitdir);
+               FREE_AND_NULL(key);
+
+               if (ret)
+                       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'. For details "
+                             "see the extensions.submodulePathConfig documentation."),
+                           submodule_name, submodule_name, submodule_name, submodule_name);
+
+               strbuf_addstr(buf, gitdir);
+       }
 
-       if (validate_submodule_git_dir(buf->buf, submodule_name) < 0)
-               die(_("refusing to create/use '%s' in another submodule's "
-                     "git dir"), buf->buf);
+       /* 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, buf->buf);
 }

diff --git a/t/lib-verify-submodule-gitdir-path.sh b/t/lib-verify-submodule-gitdir-path.sh
new file mode 100644 (file)
index 0000000..62794df
--- /dev/null
+++ b/t/lib-verify-submodule-gitdir-path.sh
@@ -0,0 +1,24 @@
+# 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" &&
+               # Compute expected absolute path
+               expected="$(git rev-parse --git-common-dir)/$path" &&
+               expected="$(test-tool path-utils real_path "$expected")" &&
+               # Compute actual absolute path
+               actual="$(git submodule--helper gitdir "$name")" &&
+               actual="$(test-tool path-utils real_path "$actual")" &&
+               echo "$expected" >expect &&
+               echo "$actual" >actual &&
+               test_cmp expect actual
+       )
+}

diff --git a/t/meson.build b/t/meson.build
index a5531df415ffe2cda8ff81fd688e9cb1e6717ab4..2c565beb8d22e81313dfb2525cb03f10c229e799 100644 (file)
--- a/t/meson.build
+++ b/t/meson.build
@@ -884,6 +884,7 @@ integration_tests = [
   't7422-submodule-output.sh',
   't7423-submodule-symlinks.sh',
   't7424-submodule-mixed-ref-formats.sh',
+  't7425-submodule-gitdir-path-extension.sh',
   't7450-bad-git-dotfiles.sh',
   't7500-commit-template-squash-signoff.sh',
   't7501-commit-basic-functionality.sh',

diff --git a/t/t7425-submodule-gitdir-path-extension.sh b/t/t7425-submodule-gitdir-path-extension.sh
new file mode 100755 (executable)
index 0000000..5d52a28
--- /dev/null
+++ b/t/t7425-submodule-gitdir-path-extension.sh
@@ -0,0 +1,138 @@
+#!/bin/sh
+
+test_description='submodulePathConfig extension works as expected'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-verify-submodule-gitdir-path.sh
+
+test_expect_success 'setup: allow file protocol' '
+       git config --global protocol.file.allow always
+'
+
+test_expect_success 'create repo with mixed extension submodules' '
+       git init -b main legacy-sub &&
+       test_commit -C legacy-sub legacy-initial &&
+       legacy_rev=$(git -C legacy-sub rev-parse HEAD) &&
+
+       git init -b main new-sub &&
+       test_commit -C new-sub new-initial &&
+       new_rev=$(git -C new-sub rev-parse HEAD) &&
+
+       git init -b main main &&
+       (
+               cd main &&
+               git submodule add ../legacy-sub legacy &&
+               test_commit legacy-sub &&
+
+               # trigger the "die_path_inside_submodule" check
+               test_must_fail git submodule add ../new-sub "legacy/nested" &&
+
+               git config core.repositoryformatversion 1 &&
+               git config extensions.submodulePathConfig true &&
+
+               git submodule add ../new-sub "New Sub" &&
+               test_commit new &&
+
+               # retrigger the "die_path_inside_submodule" check with encoding
+               test_must_fail git submodule add ../new-sub "New Sub/nested2"
+       )
+'
+
+test_expect_success 'verify new submodule gitdir config' '
+       git -C main config submodule."New Sub".gitdir > actual &&
+       echo ".git/modules/New Sub" > expect &&
+       test_cmp expect actual &&
+       verify_submodule_gitdir_path main "New Sub" "modules/New Sub"
+'
+
+test_expect_success 'manual add and verify legacy submodule gitdir config' '
+       # the legacy module should not contain a gitdir config, because it
+       # was added before the extension was enabled. Add and test it.
+       test_must_fail git -C main config submodule.legacy.gitdir &&
+       git -C main config submodule.legacy.gitdir .git/modules/legacy &&
+       git -C main config submodule.legacy.gitdir > actual &&
+       echo ".git/modules/legacy" > expect &&
+       test_cmp expect actual &&
+       verify_submodule_gitdir_path main "legacy" "modules/legacy"
+'
+
+test_expect_success 'clone from repo with both legacy and new-style submodules' '
+       git clone --recurse-submodules main cloned-non-extension &&
+       (
+               cd cloned-non-extension &&
+
+               test_path_is_dir .git/modules/legacy &&
+               test_path_is_dir .git/modules/"New Sub" &&
+
+               test_must_fail git config submodule.legacy.gitdir &&
+               test_must_fail git config submodule."New Sub".gitdir &&
+
+               git submodule status >list &&
+               test_grep "$legacy_rev legacy" list &&
+               test_grep "$new_rev New Sub" list
+       ) &&
+
+       git clone -c extensions.submodulePathConfig=true --recurse-submodules main cloned-extension &&
+       (
+               cd cloned-extension &&
+
+               test_path_is_dir .git/modules/legacy &&
+               test_path_is_dir ".git/modules/New Sub" &&
+
+               git config submodule.legacy.gitdir &&
+               git config submodule."New Sub".gitdir &&
+
+               git submodule status >list &&
+               test_grep "$legacy_rev legacy" list &&
+               test_grep "$new_rev New Sub" list
+       )
+'
+
+test_expect_success 'commit and push changes to encoded submodules' '
+       git -C legacy-sub config receive.denyCurrentBranch updateInstead &&
+       git -C new-sub config receive.denyCurrentBranch updateInstead &&
+       git -C main config receive.denyCurrentBranch updateInstead &&
+       (
+               cd cloned-extension &&
+
+               git -C legacy switch --track -C main origin/main  &&
+               test_commit -C legacy second-commit &&
+               git -C legacy push &&
+
+               git -C "New Sub" switch --track -C main origin/main &&
+               test_commit -C "New Sub" second-commit &&
+               git -C "New Sub" push &&
+
+               # Stage and commit submodule changes in superproject
+               git switch --track -C main origin/main  &&
+               git add legacy "New Sub" &&
+               git commit -m "update submodules" &&
+
+               # push superproject commit to main repo
+               git push
+       ) &&
+
+       # update expected legacy & new submodule checksums
+       legacy_rev=$(git -C legacy-sub rev-parse HEAD) &&
+       new_rev=$(git -C new-sub rev-parse HEAD)
+'
+
+test_expect_success 'fetch mixed submodule changes and verify updates' '
+       (
+               cd main &&
+
+               # only update submodules because superproject was
+               # pushed into at the end of last test
+               git submodule update --init --recursive &&
+
+               test_path_is_dir .git/modules/legacy &&
+               test_path_is_dir ".git/modules/New Sub" &&
+
+               # Verify both submodules are at the expected commits
+               git submodule status >list &&
+               test_grep "$legacy_rev legacy" list &&
+               test_grep "$new_rev New Sub" list
+       )
+'
+
+test_done

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 964e1f156932c6c13b506185f508b877b2a2ca00..ffb9c8b522e269fa2e19bcdbdb62da4c75cf6d6b 100755 (executable)
--- 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
 '