{"thread":{"id":"56506","subject":"[PATCH] submodule: extract path to submodule gitdir func","startedAt":"2021-09-15T18:59:28Z","lastAt":"2021-09-15T18:59:28Z","messageCount":1,"participants":["Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436036","messageId":"20210915185920.346756-1-jonathantanmy@google.com","threadId":"56506","inReplyTo":null,"subject":"[PATCH] submodule: extract path to submodule gitdir func","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-09-15T18:59:19Z","receivedAt":"2021-09-15T18:59:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"We currently store each submodule gitdir in \".git/modules/<name>\", but\nthis has problems with some submodule naming schemes, as described in a\ncomment in submodule_name_to_gitdir() in this patch.\n\nExtract the determination of the location of a submodule's gitdir into\nits own function submodule_name_to_gitdir(). For now, the problem\nremains unsolved, but this puts us in a better position for finding a\nsolution.\n\nThis was motivated, at $DAYJOB, by a part of Android's repo hierarchy\n[1]. In particular, there is a repo \"build\", and several repos of the\nform \"build/<name>\".\n\nThis is based on earlier work by Brandon Williams [2].\n\n[1] https://android.googlesource.com/platform/\n[2] https://lore.kernel.org/git/20180808223323.79989-2-bmwill@google.com/\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/submodule--helper.c | 10 ++++-\n dir.c                       |  2 +-\n repository.c                |  3 +-\n submodule.c                 | 77 ++++++++++++++++++++++++++-----------\n submodule.h                 |  7 ++++\n 5 files changed, 72 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4da9781b99..29ca0bedf6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1704,18 +1704,24 @@ static int add_possible_reference_from_superproject(\n \t * standard layout with .git/(modules/<name>)+/objects\n \t */\n \tif (strip_suffix(odb->path, \"/objects\", &len)) {\n+\t\tstruct repository alternate;\n \t\tchar *sm_alternate;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tstrbuf_add(&sb, odb->path, len);\n \n+\t\trepo_init(&alternate, sb.buf, NULL);\n+\n \t\t/*\n \t\t * We need to end the new path with '/' to mark it as a dir,\n \t\t * otherwise a submodule name containing '/' will be broken\n \t\t * as the last part of a missing submodule reference would\n \t\t * be taken as a file name.\n \t\t */\n-\t\tstrbuf_addf(&sb, \"/modules/%s/\", sas->submodule_name);\n+\t\tstrbuf_reset(&sb);\n+\t\tsubmodule_name_to_gitdir(&sb, &alternate, sas->submodule_name);\n+\t\tstrbuf_addch(&sb, '/');\n+\t\trepo_clear(&alternate);\n \n \t\tsm_alternate = compute_alternate_path(sb.buf, &err);\n \t\tif (sm_alternate) {\n@@ -1785,7 +1791,7 @@ static int clone_submodule(struct module_clone_data *clone_data)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), clone_data->name);\n+\tsubmodule_name_to_gitdir(&sb, the_repository, clone_data->name);\n \tsm_gitdir = absolute_pathdup(sb.buf);\n \tstrbuf_reset(&sb);\n \ndiff --git a/dir.c b/dir.c\nindex 03c4d21267..5fe5d3a42e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3633,7 +3633,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,\n \t\tstrbuf_reset(&sub_wt);\n \t\tstrbuf_reset(&sub_gd);\n \t\tstrbuf_addf(&sub_wt, \"%s/%s\", sub_worktree, sub->path);\n-\t\tstrbuf_addf(&sub_gd, \"%s/modules/%s\", sub_gitdir, sub->name);\n+\t\tsubmodule_name_to_gitdir(&sub_gd, &subrepo, sub->name);\n \n \t\tconnect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1);\n \t}\ndiff --git a/repository.c b/repository.c\nindex b2bf44c6fa..710a3b4bf8 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -213,8 +213,7 @@ int repo_submodule_init(struct repository *subrepo,\n \t\t * submodule would not have a worktree.\n \t\t */\n \t\tstrbuf_reset(&gitdir);\n-\t\tstrbuf_repo_git_path(&gitdir, superproject,\n-\t\t\t\t     \"modules/%s\", sub->name);\n+\t\tsubmodule_name_to_gitdir(&gitdir, superproject, sub->name);\n \n \t\tif (repo_init(subrepo, gitdir.buf, NULL)) {\n \t\t\tret = -1;\ndiff --git a/submodule.c b/submodule.c\nindex 8e611fe1db..9382ddf0cc 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1819,14 +1819,16 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n \n void submodule_unset_core_worktree(const struct submodule *sub)\n {\n-\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n-\t\t\t\t    get_git_dir(), sub->name);\n+\tstruct strbuf config_path = STRBUF_INIT;\n \n-\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n+\tsubmodule_name_to_gitdir(&config_path, the_repository, sub->name);\n+\tstrbuf_addstr(&config_path, \"/config\");\n+\n+\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL))\n \t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n \t\t\t  sub->path);\n \n-\tfree(config_path);\n+\tstrbuf_release(&config_path);\n }\n \n static const char *get_super_prefix_or_empty(void)\n@@ -1922,20 +1924,22 @@ int submodule_move_head(const char *path,\n \t\t\t\tabsorb_git_dir_into_superproject(path,\n \t\t\t\t\tABSORB_GITDIR_RECURSE_SUBMODULES);\n \t\t} else {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t    get_git_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 0);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\t sub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 0);\n+\t\t\tstrbuf_release(&gitdir);\n \n \t\t\t/* make sure the index is clean as well */\n \t\t\tsubmodule_reset_index(path);\n \t\t}\n \n \t\tif (old_head && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n-\t\t\tchar *gitdir = xstrfmt(\"%s/modules/%s\",\n-\t\t\t\t    get_git_dir(), sub->name);\n-\t\t\tconnect_work_tree_and_git_dir(path, gitdir, 1);\n-\t\t\tfree(gitdir);\n+\t\t\tstruct strbuf gitdir = STRBUF_INIT;\n+\t\t\tsubmodule_name_to_gitdir(&gitdir, the_repository,\n+\t\t\t\t\t\t sub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, gitdir.buf, 1);\n+\t\t\tstrbuf_release(&gitdir);\n \t\t}\n \t}\n \n@@ -2050,7 +2054,7 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)\n static void relocate_single_git_dir_into_superproject(const char *path)\n {\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n-\tchar *new_git_dir;\n+\tstruct strbuf new_gitdir = STRBUF_INIT;\n \tconst struct submodule *sub;\n \n \tif (submodule_uses_worktrees(path))\n@@ -2068,14 +2072,13 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tif (!sub)\n \t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n \n-\tnew_git_dir = git_pathdup(\"modules/%s\", sub->name);\n-\tif (validate_submodule_git_dir(new_git_dir, sub->name) < 0)\n+\tsubmodule_name_to_gitdir(&new_gitdir, the_repository, sub->name);\n+\tif (validate_submodule_git_dir(new_gitdir.buf, sub->name) < 0)\n \t\tdie(_(\"refusing to move '%s' into an existing git dir\"),\n \t\t    real_old_git_dir);\n-\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n-\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n-\treal_new_git_dir = real_pathdup(new_git_dir, 1);\n-\tfree(new_git_dir);\n+\tif (safe_create_leading_directories_const(new_gitdir.buf) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), new_gitdir.buf);\n+\treal_new_git_dir = real_pathdup(new_gitdir.buf, 1);\n \n \tfprintf(stderr, _(\"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\"),\n \t\tget_super_prefix_or_empty(), path,\n@@ -2086,6 +2089,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\n+\tstrbuf_release(&new_gitdir);\n }\n \n /*\n@@ -2105,6 +2109,7 @@ void absorb_git_dir_into_superproject(const char *path,\n \t/* Not populated? */\n \tif (!sub_git_dir) {\n \t\tconst struct submodule *sub;\n+\t\tstruct strbuf sub_gitdir = STRBUF_INIT;\n \n \t\tif (err_code == READ_GITFILE_ERR_STAT_FAILED) {\n \t\t\t/* unpopulated as expected */\n@@ -2126,8 +2131,9 @@ void absorb_git_dir_into_superproject(const char *path,\n \t\tsub = submodule_from_path(the_repository, null_oid(), path);\n \t\tif (!sub)\n \t\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n-\t\tconnect_work_tree_and_git_dir(path,\n-\t\t\tgit_path(\"modules/%s\", sub->name), 0);\n+\t\tsubmodule_name_to_gitdir(&sub_gitdir, the_repository, sub->name);\n+\t\tconnect_work_tree_and_git_dir(path, sub_gitdir.buf, 0);\n+\t\tstrbuf_release(&sub_gitdir);\n \t} else {\n \t\t/* Is it already absorbed into the superprojects git dir? */\n \t\tchar *real_sub_git_dir = real_pathdup(sub_git_dir, 1);\n@@ -2278,9 +2284,36 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)\n \t\t\tgoto cleanup;\n \t\t}\n \t\tstrbuf_reset(buf);\n-\t\tstrbuf_git_path(buf, \"%s/%s\", \"modules\", sub->name);\n+\t\tsubmodule_name_to_gitdir(buf, the_repository, sub->name);\n \t}\n \n cleanup:\n \treturn ret;\n }\n+\n+void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t      const char *submodule_name)\n+{\n+\t/*\n+\t * NEEDSWORK: The current way of mapping a submodule's name to\n+\t * its location in .git/modules/ has problems with some naming\n+\t * schemes. For example, if a submodule is named \"foo\" and\n+\t * another is named \"foo/bar\" (whether present in the same\n+\t * superproject commit or not - the problem will arise if both\n+\t * superproject commits have been checked out at any point in\n+\t * time), or if two submodule names only have different cases in\n+\t * a case-insensitive filesystem.\n+\t *\n+\t * There are several solutions, including encoding the path in\n+\t * some way, introducing a submodule.<name>.gitdir config in\n+\t * .git/config (not .gitmodules) that allows overriding what the\n+\t * gitdir of a submodule would be (and teach Git, upon noticing\n+\t * a clash, to automatically determine a non-clashing name and\n+\t * to write such a config), or introducing a\n+\t * submodule.<name>.gitdir config in .gitmodules that repo\n+\t * administrators can explicitly set. Nothing has been decided,\n+\t * so for now, just append the name at the end of the path.\n+\t */\n+\tstrbuf_repo_git_path(buf, r, \"modules/\");\n+\tstrbuf_addstr(buf, submodule_name);\n+}\ndiff --git a/submodule.h b/submodule.h\nindex 84640c49c1..c11e0807fc 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -124,6 +124,13 @@ int push_unpushed_submodules(struct repository *r,\n  */\n int submodule_to_gitdir(struct strbuf *buf, const char *submodule);\n \n+/*\n+ * Given a submodule name, create a path to where the submodule's gitdir lives\n+ * inside of the provided repository's 'modules' directory.\n+ */\n+void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r,\n+\t\t\t      const char *submodule_name);\n+\n /*\n  * Make sure that no submodule's git dir is nested in a sibling submodule's.\n  */\n-- \n2.33.0.309.g3052b89438-goog\n\n"}]}