{"thread":{"id":"55906","subject":"[RFC PATCH 0/4] cache parent project's gitdir in submodules","startedAt":"2021-06-11T22:54:48Z","lastAt":"2021-10-14T19:51:23Z","messageCount":51,"participants":["Emily Shaffer","Jacob Keller","Junio C Hamano","Jonathan Tan","Derrick Stolee","Matheus Tavares","Matheus Tavares Bernardino","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"427154","messageId":"20210611225428.1208973-1-emilyshaffer@google.com","threadId":"55906","inReplyTo":null,"subject":"[RFC PATCH 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-11T22:54:24Z","receivedAt":"2021-06-11T22:54:48Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"It's necessary for a superproject to know which submodules it contains.\nHowever, historically submodules do not know they are anything but a\nnormal single-repo Git project (or a superproject, in nested-submodules\ncases). This decision does help prevent us from having to support\ndivergent behaviors in submodule projects vs. superprojects, which makes\nsure Git is (somewhat) less confusing for the reader, and helps simplify\nour code.\n\nOne could imagine, though, some conveniences we could gain from\nsubmodules learning added behavior (though not necessarily *different*\nbehavior) to provide more context about the state of the project as a\nwhole, and to make large submodule-based projects easier to work with.\nOne example is a series[1] I sent some time ago, adding a config to be\nshared between the superproject and all its submodules. The RFC[2] I\nsent around the same time mentions a few other examples, such as \"git\nstatus\" run in the submodule noticing whether the superproject has a\ncommit referencing the submodule's current HEAD.\n\nIt's expensive and non-definitive to try and guess whether or not the\ncurrent repo is a submodule. submodule.c:get_superproject_working_tree()\ndoes so by essentially running 'git -C .. ls-files -- ',\ninvoking an additional process. get_superproject_working_tree() is not\ncalled often, so that's mostly fine. However, [1] attempted to include\nan additional config located in the superproject's gitdir by running\n'git -C .. rev-parse --git-dir' during startup - a little expensive in\nthe best case, because it's an extra process, but extremely expensive in\nthe case when the current repo is *not* a submodule, because we hunt all\nthe way up the filesystem looking for a '.git'. Adding that cost to\nevery startup is infeasible.\n\nTo that end, in this series I propose caching a path to the\nsuperproject's gitdir - by having the superproject write that relative\npath to the submodule's config on creation or update. The goal here is\n*not* to say \"If I am a submodule, I must have\nsubmodule.superprojectGitDir set\" - but instead to say \"If I have\nsubmodule.superprojectGitDir set, then I must be a submodule.\" That is,\nI expect we will find edge cases where a submodule was introduced in\nsome interesting way that bypassed any of the patches below, and\ntherefore doesn't have the superproject's gitdir cached.\n\nThe combination of these two rules:\n - Anything relying on submodule.superprojectGitDir must be nice to\n have, but not essential, because\n - It's possible for a submodule to be valid without having\n submodule.superprojectGitDir set\nmakes me feel more comfortable with the idea of submodules learning\nadditional behavior based on this config. I feel pretty unconfident in\nour ability to ensure that *every* submodule has this config set.\n\nThe series covers a few paths for introducing that config, which I'm\nhoping covers most cases.\n - \"git submodule update\" (which seems to be part of the \"git submodule\n init\" flow)\n - \"git submodule absorbgitdir\" to convert a \"git init\"'d repo into a\n submodule\n\nNotably, we can only really set this config when 'the_repository' is the\nsuperproject - that appears to be the only time when we know the gitdirs\nof both the superproject and the submodule.\n\nI'm expecting folks may have a lot to say about this, so I look forward\nto discussion :)\n\n - Emily\n\n1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com\n2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com\n\nEmily Shaffer (4):\n t7400-submodule-basic: modernize inspect() helper\n introduce submodule.superprojectGitDir cache\n submodule: cache superproject gitdir during absorbgitdirs\n submodule: cache superproject gitdir during 'update'\n\n builtin/submodule--helper.c | 4 +++\n git-submodule.sh | 9 ++++++\n submodule.c | 10 ++++++\n t/t7400-submodule-basic.sh | 49 ++++++++++++++----------------\n t/t7406-submodule-update.sh | 10 ++++++\n t/t7412-submodule-absorbgitdirs.sh | 1 +\n 6 files changed, 57 insertions(+), 26 deletions(-)\n\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427155","messageId":"20210611225428.1208973-2-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"[RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-11T22:54:25Z","receivedAt":"2021-06-11T22:54:49Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Since the inspect() helper in the submodule-basic test suite was\nwritten, 'git -C ' was added. By using -C, we no longer need a\nreference to the base directory for the test. This simplifies callsites,\nand will make the addition of other arguments in later patches more\nreadable.\n\nSigned-off-by: Emily Shaffer \n---\n t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------\n 1 file changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..f5dc051a6e 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '\n # generates, which will expand symbolic links.\n submodurl=$(pwd -P)\n \n-listbranches() {\n-\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n-}\n-\n inspect() {\n \tdir=$1 &&\n-\tdotdot=\"${2:-..}\" &&\n \n-\t(\n-\t\tcd \"$dir\" &&\n-\t\tlistbranches >\"$dotdot/heads\" &&\n-\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n-\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff-files --exit-code &&\n-\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n-\t)\n+\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$dir\" update-index --refresh &&\n+\tgit -C \"$dir\" diff-files --exit-code &&\n+\tgit -C \"$dir\" clean -n -d -x >untracked\n }\n \n+\n test_expect_success 'submodule add' '\n \techo \"refs/heads/main\" >expect &&\n \n@@ -146,7 +139,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod ../.. &&\n+\tinspect addtest/submod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch ../.. &&\n+\tinspect addtest/submod-branch &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotslashdotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz ../../.. &&\n+\tinspect addtest/slashslashsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod ../.. &&\n+\tinspect addtest/realsubmod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 ../.. &&\n+\tinspect addtest/realsubmod2 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 ../.. &&\n+\tinspect addtest/realsubmod3 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427156","messageId":"20210611225428.1208973-3-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"[RFC PATCH 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-11T22:54:26Z","receivedAt":"2021-06-11T22:54:53Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach submodules a reference to their superproject's gitdir. This allows\nus to A) know that we're running from a submodule, and B) have a\nshortcut to the superproject's vitals, for example, configs.\n\nBy using a relative path instead of an absolute path, we can move the\nsuperproject directory around on the filesystem without breaking the\nsubmodule's cache.\n\nSince this cached value is only introduced during new submodule creation\nvia `git submodule add`, though, there is more work to do to allow the\ncache to be created at other times.\n\nSigned-off-by: Emily Shaffer \n---\n builtin/submodule--helper.c | 4 ++++\n t/t7400-submodule-basic.sh | 40 ++++++++++++++++++++-----------------\n 2 files changed, 26 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9d505a6329..3d6fd54c9b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1909,6 +1909,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n \t\t\t\t\t error_strategy);\n \n+\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(absolute_path(get_git_dir()),\n+\t\t\t\t\t path, &sb));\n+\n \tfree(sm_alternate);\n \tfree(error_strategy);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex f5dc051a6e..e45f42588f 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '\n submodurl=$(pwd -P)\n \n inspect() {\n-\tdir=$1 &&\n-\n-\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n-\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n-\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n-\tgit -C \"$dir\" update-index --refresh &&\n-\tgit -C \"$dir\" diff-files --exit-code &&\n-\tgit -C \"$dir\" clean -n -d -x >untracked\n+\tsub_dir=$1 &&\n+\tsuper_dir=$2 &&\n+\n+\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$sub_dir\" update-index --refresh &&\n+\tgit -C \"$sub_dir\" diff-files --exit-code &&\n+\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n+\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n+\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n+\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n }\n \n \n@@ -139,7 +143,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod &&\n+\tinspect addtest/submod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch &&\n+\tinspect addtest/submod-branch addtest &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz &&\n+\tinspect addtest/dotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz &&\n+\tinspect addtest/dotslashdotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz &&\n+\tinspect addtest/slashslashsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod &&\n+\tinspect addtest/realsubmod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 &&\n+\tinspect addtest/realsubmod2 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 &&\n+\tinspect addtest/realsubmod3 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' '\n \tgit submodule update -q >update.out &&\n \ttest_must_be_empty update.out &&\n \n-\tinspect init &&\n+\tinspect init . &&\n \ttest_cmp expect head-sha1\n '\n \n@@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' '\n \techo \"$rev1\" >expect &&\n \n \tgit submodule update init &&\n-\tinspect init &&\n+\tinspect init . &&\n \n \ttest_cmp expect head-sha1\n '\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427157","messageId":"20210611225428.1208973-4-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"[RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-11T22:54:27Z","receivedAt":"2021-06-11T22:55:59Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Already during 'git submodule add' we cache a pointer to the\nsuperproject's gitdir. However, this doesn't help brand-new\nsubmodules created with 'git init' and later absorbed with 'git\nsubmodule absorbgitdir'. Let's start adding that pointer during 'git\nsubmodule absorbgitdir' too.\n\nSigned-off-by: Emily Shaffer \n---\n submodule.c | 10 ++++++++++\n t/t7412-submodule-absorbgitdirs.sh | 1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9767ba9893..09dfc4ee38 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2064,6 +2064,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tchar *new_git_dir;\n \tconst struct submodule *sub;\n+\tstruct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;\n \n \tif (submodule_uses_worktrees(path))\n \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n@@ -2095,6 +2096,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \n \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n \n+\t/* cache pointer to superproject's gitdir */\n+\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n+\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n+\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(get_super_prefix_or_empty(),\n+\t\t\t\t\t path, &sb));\n+\n+\tstrbuf_release(&config_path);\n+\tstrbuf_release(&sb);\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1cfa150768..70fc282937 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -29,6 +29,7 @@ test_expect_success 'absorb the git dir' '\n \ttest -d .git/modules/sub1 &&\n \tgit status >actual.1 &&\n \tgit -C sub1 rev-parse HEAD >actual.2 &&\n+\ttest . -ef \"$(git -C sub1 config submodule.superprojectGitDir)\" &&\n \ttest_cmp expect.1 actual.1 &&\n \ttest_cmp expect.2 actual.2\n '\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427158","messageId":"20210611225428.1208973-5-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"[RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-11T22:54:28Z","receivedAt":"2021-06-11T22:55:59Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"A cached path to the superproject's gitdir might be added during 'git\nsubmodule add', but in some cases - like submodules which were created\nbefore 'git submodule add' learned to cache that info - it might be\nuseful to update the cache. Let's do it during 'git submodule update',\nwhen we already have a handle to the superproject while calling\noperations on the submodules.\n\nSigned-off-by: Emily Shaffer \n---\n git-submodule.sh | 9 +++++++++\n t/t7406-submodule-update.sh | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex eb90f18229..ddda751cfa 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -648,6 +648,15 @@ cmd_update()\n \t\t\tfi\n \t\tfi\n \n+\t\t# Cache a pointer to the superproject's gitdir. This may have\n+\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n+\t\t# if applicable, otherwise to local.\n+\n+\t\tsp_gitdir=\"$(git rev-parse --absolute-git-dir)\"\n+\t\trelative_gitdir=\"$(realpath --relative-to \"$sm_path\" \"$sp_gitdir\")\"\n+\t\tgit -C \"$sm_path\" config --worktree \\\n+\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n+\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex ff3ba5422e..96023cbb6a 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1037,4 +1037,14 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase'\n \t)\n '\n \n+test_expect_success 'submodule update adds superproject gitdir to older repos' '\n+\t(cd super &&\n+\t git -C submodule config --unset submodule.superprojectGitdir &&\n+\t git submodule update &&\n+\t echo \"../.git\" >expect &&\n+\t git -C submodule config submodule.superprojectGitdir >actual &&\n+\t test_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427194","messageId":"CA+P7+xp5NNb6zz1LkQrv_A3Ny41WE=-h2-9VswyL_qzwFdyifw@mail.gmail.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2021-06-12T20:12:58Z","receivedAt":"2021-06-12T20:14:27Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jun 11, 2021 at 3:54 PM Emily Shaffer wrote:\n>\n> It's necessary for a superproject to know which submodules it contains.\n> However, historically submodules do not know they are anything but a\n> normal single-repo Git project (or a superproject, in nested-submodules\n> cases). This decision does help prevent us from having to support\n> divergent behaviors in submodule projects vs. superprojects, which makes\n> sure Git is (somewhat) less confusing for the reader, and helps simplify\n> our code.\n>\n> One could imagine, though, some conveniences we could gain from\n> submodules learning added behavior (though not necessarily *different*\n> behavior) to provide more context about the state of the project as a\n> whole, and to make large submodule-based projects easier to work with.\n> One example is a series[1] I sent some time ago, adding a config to be\n> shared between the superproject and all its submodules. The RFC[2] I\n> sent around the same time mentions a few other examples, such as \"git\n> status\" run in the submodule noticing whether the superproject has a\n> commit referencing the submodule's current HEAD.\n>\n> It's expensive and non-definitive to try and guess whether or not the\n> current repo is a submodule. submodule.c:get_superproject_working_tree()\n> does so by essentially running 'git -C .. ls-files -- ',\n> invoking an additional process. get_superproject_working_tree() is not\n> called often, so that's mostly fine. However, [1] attempted to include\n> an additional config located in the superproject's gitdir by running\n> 'git -C .. rev-parse --git-dir' during startup - a little expensive in\n> the best case, because it's an extra process, but extremely expensive in\n> the case when the current repo is *not* a submodule, because we hunt all\n> the way up the filesystem looking for a '.git'. Adding that cost to\n> every startup is infeasible.\n>\n\nIt also adds cost for no benefit to the normal case where it's not a\nsubmodule. The cost added for non-submodules ought to be as near-zero\nas possible.\n\n> To that end, in this series I propose caching a path to the\n> superproject's gitdir - by having the superproject write that relative\n> path to the submodule's config on creation or update. The goal here is\n> *not* to say \"If I am a submodule, I must have\n> submodule.superprojectGitDir set\" - but instead to say \"If I have\n> submodule.superprojectGitDir set, then I must be a submodule.\" That is,\n> I expect we will find edge cases where a submodule was introduced in\n> some interesting way that bypassed any of the patches below, and\n> therefore doesn't have the superproject's gitdir cached.\n>\n> The combination of these two rules:\n> - Anything relying on submodule.superprojectGitDir must be nice to\n> have, but not essential, because\n> - It's possible for a submodule to be valid without having\n> submodule.superprojectGitDir set\n> makes me feel more comfortable with the idea of submodules learning\n> additional behavior based on this config. I feel pretty unconfident in\n> our ability to ensure that *every* submodule has this config set.\n>\n\nI think this is a good direction. I do think having some information\nabout being a submodule could be very useful for tools such as git\nstatus, and making it more of a \"if we know for sure, we get some\nsmall benefit, but if we don't know then it's no harm\" is a good\ndirection.\n\n> The series covers a few paths for introducing that config, which I'm\n> hoping covers most cases.\n> - \"git submodule update\" (which seems to be part of the \"git submodule\n> init\" flow)\n> - \"git submodule absorbgitdir\" to convert a \"git init\"'d repo into a\n> submodule\n>\n> Notably, we can only really set this config when 'the_repository' is the\n> superproject - that appears to be the only time when we know the gitdirs\n> of both the superproject and the submodule.\n\nWe could also presumably add a new command for setting this?\n\n>\n> I'm expecting folks may have a lot to say about this, so I look forward\n> to discussion :)\n>\n> - Emily\n>\n> 1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com\n> 2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com\n>\n> Emily Shaffer (4):\n> t7400-submodule-basic: modernize inspect() helper\n> introduce submodule.superprojectGitDir cache\n> submodule: cache superproject gitdir during absorbgitdirs\n> submodule: cache superproject gitdir during 'update'\n>\n> builtin/submodule--helper.c | 4 +++\n> git-submodule.sh | 9 ++++++\n> submodule.c | 10 ++++++\n> t/t7400-submodule-basic.sh | 49 ++++++++++++++----------------\n> t/t7406-submodule-update.sh | 10 ++++++\n> t/t7412-submodule-absorbgitdirs.sh | 1 +\n> 6 files changed, 57 insertions(+), 26 deletions(-)\n>\n> --\n> 2.32.0.272.g935e593368-goog\n>\n"},{"id":"427270","messageId":"xmqq5yyhyrmr.fsf@gitster.g","threadId":"55906","inReplyTo":"20210611225428.1208973-2-emilyshaffer@google.com","subject":"Re: [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-14T04:52:44Z","receivedAt":"2021-06-14T04:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Since the inspect() helper in the submodule-basic test suite was\n> written, 'git -C ' was added. By using -C, we no longer need a\n> reference to the base directory for the test. This simplifies callsites,\n> and will make the addition of other arguments in later patches more\n> readable.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n> t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------\n> 1 file changed, 15 insertions(+), 22 deletions(-)\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index a924fdb7a6..f5dc051a6e 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '\n> # generates, which will expand symbolic links.\n> submodurl=$(pwd -P)\n> \n> -listbranches() {\n> -\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n> -}\n> -\n> inspect() {\n> \tdir=$1 &&\n> -\tdotdot=\"${2:-..}\" &&\n> \n> -\t(\n> -\t\tcd \"$dir\" &&\n> -\t\tlistbranches >\"$dotdot/heads\" &&\n> -\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n> -\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n> -\t\tgit update-index --refresh &&\n> -\t\tgit diff-files --exit-code &&\n> -\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n> -\t)\n> +\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> +\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> +\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> +\tgit -C \"$dir\" update-index --refresh &&\n> +\tgit -C \"$dir\" diff-files --exit-code &&\n> +\tgit -C \"$dir\" clean -n -d -x >untracked\n> }\n\nQuite straight-forward.\n\n> -\tinspect addtest/submod ../.. &&\n> +\tinspect addtest/submod &&\n\nAnd specifically the losing ../.. is pleasing to the eye, especially\nbecause the old \"dotdot\" thing was mechanically computable from \"dir\".\n\nNicely done.\n"},{"id":"427271","messageId":"xmqqzgvtxcas.fsf@gitster.g","threadId":"55906","inReplyTo":"20210611225428.1208973-3-emilyshaffer@google.com","subject":"Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-14T05:09:15Z","receivedAt":"2021-06-14T05:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Teach submodules a reference to their superproject's gitdir. This allows\n> us to A) know that we're running from a submodule, and B) have a\n> shortcut to the superproject's vitals, for example, configs.\n>\n> By using a relative path instead of an absolute path, we can move the\n> superproject directory around on the filesystem without breaking the\n> submodule's cache.\n\nAs the function this new thing is added assumes the modern layout of\nhaving submodule \"repository\" in .git/modules/* of the repository of\nthe superproject, it is rather easy to move the whole thing together,\nso recording it as relative path is all the more important.\n\nCan a submodule repository be bound to two or more superproject at\nthe same time? \"We assume no, and we will forbid such a layout, and\nthat is why we can afford to make submodules aware of their\nsuperprojects\" is a totally acceptable answer, but it would make it\neasier to follow the reasoning behind a design change like this\nseries does if such an assumption is recorded somewhere.\n\n> +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(absolute_path(get_git_dir()),\n> +\t\t\t\t\t path, &sb));\n> +\n\nOK, so even when the superproject is used as a submodule of somebody\nelse, we could get to the top of its working tree, because (1) the\nsubmodule we are currently working in can find out where the gitdir\nof the superproject is, and (2) in that gitdir, which is very likely\ndifferent from the \".git/\" subdirectory of the working tree of the\nsuperproject (instead, it would be a directory in \".git/modules/\" of\nits superproject), we could find the core.worktree configuration to\nreach the working tree of the superproject.\n\nOK, makes sense.\n\nThanks.\n"},{"id":"427272","messageId":"xmqqim2hx93w.fsf@gitster.g","threadId":"55906","inReplyTo":"20210611225428.1208973-4-emilyshaffer@google.com","subject":"Re: [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-14T06:18:11Z","receivedAt":"2021-06-14T06:18:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> +\ttest . -ef \"$(git -C sub1 config submodule.superprojectGitDir)\" &&\n\nUnfortunately \"test f1 -ef f2\", \"test f1 -nt f2\", etc. are not part\nof POSIX.\n\nWhen in doubt, check\n\n https://pubs.opengroup.org/onlinepubs/9699919799/utilities/toc.html\n\nThanks.\n"},{"id":"427273","messageId":"xmqqeed5x8wq.fsf@gitster.g","threadId":"55906","inReplyTo":"20210611225428.1208973-5-emilyshaffer@google.com","subject":"Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-14T06:22:29Z","receivedAt":"2021-06-14T06:22:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> A cached path to the superproject's gitdir might be added during 'git\n> submodule add', but in some cases - like submodules which were created\n> before 'git submodule add' learned to cache that info - it might be\n> useful to update the cache. Let's do it during 'git submodule update',\n> when we already have a handle to the superproject while calling\n> operations on the submodules.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n> git-submodule.sh | 9 +++++++++\n> t/t7406-submodule-update.sh | 10 ++++++++++\n> 2 files changed, 19 insertions(+)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index eb90f18229..ddda751cfa 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -648,6 +648,15 @@ cmd_update()\n> \t\t\tfi\n> \t\tfi\n> \n> +\t\t# Cache a pointer to the superproject's gitdir. This may have\n> +\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n> +\t\t# if applicable, otherwise to local.\n> +\n> +\t\tsp_gitdir=\"$(git rev-parse --absolute-git-dir)\"\n> +\t\trelative_gitdir=\"$(realpath --relative-to \"$sm_path\" \"$sp_gitdir\")\"\n\nrealpath may not exist on the target system. Discussions on the\npatch [*1*] may be of interest.\n\nIt might be a good idea to push to your github repository to trigger\nCI on macOS (I am guessing that you do not test locally on macs from\nthe two issues we saw in this series).\n\nThanks.\n\n\n[Reference]\n\n*1*\nhttps://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/\n"},{"id":"427275","messageId":"xmqqo8c8zz3a.fsf@gitster.g","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-14T07:26:17Z","receivedAt":"2021-06-14T07:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> It's necessary for a superproject to know which submodules it contains.\n> However, historically submodules do not know they are anything but a\n> normal single-repo Git project (or a superproject, in nested-submodules\n> cases). This decision does help prevent us from having to support\n> divergent behaviors in submodule projects vs. superprojects, which makes\n> sure Git is (somewhat) less confusing for the reader, and helps simplify\n> our code.\n\nhttps://github.com/git/git/actions/runs/934803365 is the CI run of\n'seen' that passes. With this and another topic merged to 'seen',\nas can be seen in https://github.com/git/git/actions/runs/934680687,\na handful of tests fail.\n\nThanks.\n\n"},{"id":"427530","messageId":"YMkZOxlSRnWlYRKR@google.com","threadId":"55906","inReplyTo":"xmqqo8c8zz3a.fsf@gitster.g","subject":"Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-15T21:18:51Z","receivedAt":"2021-06-15T21:19:02Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Jun 14, 2021 at 04:26:17PM +0900, Junio C Hamano wrote:\n> \n> Emily Shaffer writes:\n> \n> > It's necessary for a superproject to know which submodules it contains.\n> > However, historically submodules do not know they are anything but a\n> > normal single-repo Git project (or a superproject, in nested-submodules\n> > cases). This decision does help prevent us from having to support\n> > divergent behaviors in submodule projects vs. superprojects, which makes\n> > sure Git is (somewhat) less confusing for the reader, and helps simplify\n> > our code.\n> \n> https://github.com/git/git/actions/runs/934803365 is the CI run of\n> 'seen' that passes. With this and another topic merged to 'seen',\n> as can be seen in https://github.com/git/git/actions/runs/934680687,\n> a handful of tests fail.\n\nThanks, I will investigate.\n"},{"id":"427533","messageId":"YMkbUYmefgp6erxD@google.com","threadId":"55906","inReplyTo":"xmqqeed5x8wq.fsf@gitster.g","subject":"Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-15T21:27:45Z","receivedAt":"2021-06-15T21:27:55Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Jun 14, 2021 at 03:22:29PM +0900, Junio C Hamano wrote:\n> \n> Emily Shaffer writes:\n> \n> > A cached path to the superproject's gitdir might be added during 'git\n> > submodule add', but in some cases - like submodules which were created\n> > before 'git submodule add' learned to cache that info - it might be\n> > useful to update the cache. Let's do it during 'git submodule update',\n> > when we already have a handle to the superproject while calling\n> > operations on the submodules.\n> >\n> > Signed-off-by: Emily Shaffer \n> > ---\n> > git-submodule.sh | 9 +++++++++\n> > t/t7406-submodule-update.sh | 10 ++++++++++\n> > 2 files changed, 19 insertions(+)\n> >\n> > diff --git a/git-submodule.sh b/git-submodule.sh\n> > index eb90f18229..ddda751cfa 100755\n> > --- a/git-submodule.sh\n> > +++ b/git-submodule.sh\n> > @@ -648,6 +648,15 @@ cmd_update()\n> > \t\t\tfi\n> > \t\tfi\n> > \n> > +\t\t# Cache a pointer to the superproject's gitdir. This may have\n> > +\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n> > +\t\t# if applicable, otherwise to local.\n> > +\n> > +\t\tsp_gitdir=\"$(git rev-parse --absolute-git-dir)\"\n> > +\t\trelative_gitdir=\"$(realpath --relative-to \"$sm_path\" \"$sp_gitdir\")\"\n> \n> realpath may not exist on the target system. Discussions on the\n> patch [*1*] may be of interest.\n> \n> It might be a good idea to push to your github repository to trigger\n> CI on macOS (I am guessing that you do not test locally on macs from\n> the two issues we saw in this series).\n\nI typically do, sorry for forgetting to do so this time.\n\n> \n> Thanks.\n> \n> \n> [Reference]\n> \n> *1*\n> https://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/\n"},{"id":"427537","messageId":"YMkjF+TN05lM4IJV@google.com","threadId":"55906","inReplyTo":"xmqqzgvtxcas.fsf@gitster.g","subject":"Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-15T22:00:55Z","receivedAt":"2021-06-15T22:01:05Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Mon, Jun 14, 2021 at 02:09:15PM +0900, Junio C Hamano wrote:\n> \n> Emily Shaffer writes:\n> \n> > Teach submodules a reference to their superproject's gitdir. This allows\n> > us to A) know that we're running from a submodule, and B) have a\n> > shortcut to the superproject's vitals, for example, configs.\n> >\n> > By using a relative path instead of an absolute path, we can move the\n> > superproject directory around on the filesystem without breaking the\n> > submodule's cache.\n> \n> As the function this new thing is added assumes the modern layout of\n> having submodule \"repository\" in .git/modules/* of the repository of\n> the superproject, it is rather easy to move the whole thing together,\n> so recording it as relative path is all the more important.\n> \n> Can a submodule repository be bound to two or more superproject at\n> the same time? \"We assume no, and we will forbid such a layout, and\n> that is why we can afford to make submodules aware of their\n> superprojects\" is a totally acceptable answer, but it would make it\n> easier to follow the reasoning behind a design change like this\n> series does if such an assumption is recorded somewhere.\n\nThanks for pointing out the use case - I can see this happening in some\ncase like adding a source code dependency from a few projects at once,\nso it doesn't sound too far-fetched. But no, I don't think this method\nof caching can support it; I think forbidding it and saying \"if you want\nthis to work, use a worktree\" is pretty reasonable.\n\nWill find a nice place to write it down.\n\n - Emily\n"},{"id":"427547","messageId":"20210616004508.87186-1-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210611225428.1208973-1-emilyshaffer@google.com","subject":"[PATCH v2 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-16T00:45:04Z","receivedAt":"2021-06-16T00:45:20Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"The reception for this series seemed pretty good in v1, so I'm dropping\nthe RFC.\n\nTested: https://github.com/nasamuffin/git/actions/runs/941100646\n\nSinec v1, mostly platform-friendliness fixes. Also added documentation\nfor the new config option - wordsmithing help is always welcome.\n\n - Emily\n\nEmily Shaffer (4):\n t7400-submodule-basic: modernize inspect() helper\n introduce submodule.superprojectGitDir cache\n submodule: cache superproject gitdir during absorbgitdirs\n submodule: cache superproject gitdir during 'update'\n\n Documentation/config/submodule.txt | 12 ++++++++\n builtin/submodule--helper.c | 4 +++\n git-submodule.sh | 10 ++++++\n submodule.c | 10 ++++++\n t/t7400-submodule-basic.sh | 49 ++++++++++++++----------------\n t/t7406-submodule-update.sh | 10 ++++++\n t/t7412-submodule-absorbgitdirs.sh | 9 +++++-\n 7 files changed, 77 insertions(+), 27 deletions(-)\n\nRange-diff against v1:\n1: d6284438fb = 1: a6718eea80 t7400-submodule-basic: modernize inspect() helper\n2: 56470e2eab ! 2: 4cebe7bcb5 introduce submodule.superprojectGitDir cache\n @@ Commit message\n \n Signed-off-by: Emily Shaffer \n \n + ## Documentation/config/submodule.txt ##\n +@@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::\n + \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n + \tor `info`, and if there is an error with the computed alternate, the\n + \tclone proceeds as if no alternate was specified.\n ++\n ++submodule.superprojectGitDir::\n ++\tThe relative path from the submodule's worktree to the superproject's\n ++\tgitdir. This config should only be present in projects which are\n ++\tsubmodules, but is not guaranteed to be present in every submodule. It\n ++\tis set automatically during submodule creation.\n +++\n ++\tIn situations where more than one superproject references the same\n ++\tsubmodule worktree, the value of this config and the behavior of\n ++\toperations which use it are undefined. To reference a single project\n ++\tfrom multiple superprojects, it is better to create a worktree of the\n ++\tsubmodule for each superproject.\n +\n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n3: 42f954f523 ! 3: df97a9c2bb submodule: cache superproject gitdir during absorbgitdirs\n @@ submodule.c: static void relocate_single_git_dir_into_superproject(const char *p\n \n ## t/t7412-submodule-absorbgitdirs.sh ##\n @@ t/t7412-submodule-absorbgitdirs.sh: test_expect_success 'absorb the git dir' '\n - \ttest -d .git/modules/sub1 &&\n \tgit status >actual.1 &&\n \tgit -C sub1 rev-parse HEAD >actual.2 &&\n -+\ttest . -ef \"$(git -C sub1 config submodule.superprojectGitDir)\" &&\n \ttest_cmp expect.1 actual.1 &&\n - \ttest_cmp expect.2 actual.2\n +-\ttest_cmp expect.2 actual.2\n ++\ttest_cmp expect.2 actual.2 &&\n ++\n ++\t# make sure the submodule cached the superproject gitdir correctly\n ++\ttest-tool path-utils real_path . >expect &&\n ++\ttest-tool path-utils real_path \\\n ++\t\t\"$(git -C sub1 config submodule.superprojectGitDir)\" >actual &&\n ++\n ++\ttest_cmp expect actual\n '\n + \n + test_expect_success 'absorbing does not fail for deinitialized submodules' '\n4: 4f55ab42c7 ! 4: a3f3be58ad submodule: cache superproject gitdir during 'update'\n @@ git-submodule.sh: cmd_update()\n +\t\t# Cache a pointer to the superproject's gitdir. This may have\n +\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n +\t\t# if applicable, otherwise to local.\n ++\t\trelative_gitdir=\"$(git rev-parse --path-format=relative \\\n ++\t\t\t\t\t\t --prefix \"${sm_path}\" \\\n ++\t\t\t\t\t\t --git-dir)\"\n +\n -+\t\tsp_gitdir=\"$(git rev-parse --absolute-git-dir)\"\n -+\t\trelative_gitdir=\"$(realpath --relative-to \"$sm_path\" \"$sp_gitdir\")\"\n +\t\tgit -C \"$sm_path\" config --worktree \\\n +\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n +\n @@ git-submodule.sh: cmd_update()\n \t\t\t(\n \n ## t/t7406-submodule-update.sh ##\n -@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to merge/rebase'\n +@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to fetch with a s\n \t)\n '\n \n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427548","messageId":"20210616004508.87186-2-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-1-emilyshaffer@google.com","subject":"[PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-16T00:45:05Z","receivedAt":"2021-06-16T00:45:21Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Since the inspect() helper in the submodule-basic test suite was\nwritten, 'git -C ' was added. By using -C, we no longer need a\nreference to the base directory for the test. This simplifies callsites,\nand will make the addition of other arguments in later patches more\nreadable.\n\nSigned-off-by: Emily Shaffer \n---\n t/t7400-submodule-basic.sh | 37 +++++++++++++++----------------------\n 1 file changed, 15 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..f5dc051a6e 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '\n # generates, which will expand symbolic links.\n submodurl=$(pwd -P)\n \n-listbranches() {\n-\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n-}\n-\n inspect() {\n \tdir=$1 &&\n-\tdotdot=\"${2:-..}\" &&\n \n-\t(\n-\t\tcd \"$dir\" &&\n-\t\tlistbranches >\"$dotdot/heads\" &&\n-\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n-\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff-files --exit-code &&\n-\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n-\t)\n+\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$dir\" update-index --refresh &&\n+\tgit -C \"$dir\" diff-files --exit-code &&\n+\tgit -C \"$dir\" clean -n -d -x >untracked\n }\n \n+\n test_expect_success 'submodule add' '\n \techo \"refs/heads/main\" >expect &&\n \n@@ -146,7 +139,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod ../.. &&\n+\tinspect addtest/submod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch ../.. &&\n+\tinspect addtest/submod-branch &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotslashdotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz ../../.. &&\n+\tinspect addtest/slashslashsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod ../.. &&\n+\tinspect addtest/realsubmod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 ../.. &&\n+\tinspect addtest/realsubmod2 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 ../.. &&\n+\tinspect addtest/realsubmod3 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427549","messageId":"20210616004508.87186-3-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-1-emilyshaffer@google.com","subject":"[PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-16T00:45:06Z","receivedAt":"2021-06-16T00:45:22Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach submodules a reference to their superproject's gitdir. This allows\nus to A) know that we're running from a submodule, and B) have a\nshortcut to the superproject's vitals, for example, configs.\n\nBy using a relative path instead of an absolute path, we can move the\nsuperproject directory around on the filesystem without breaking the\nsubmodule's cache.\n\nSince this cached value is only introduced during new submodule creation\nvia `git submodule add`, though, there is more work to do to allow the\ncache to be created at other times.\n\nSigned-off-by: Emily Shaffer \n---\n Documentation/config/submodule.txt | 12 +++++++++\n builtin/submodule--helper.c | 4 +++\n t/t7400-submodule-basic.sh | 40 ++++++++++++++++--------------\n 3 files changed, 38 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\nindex d7a63c8c12..7c459cc19e 100644\n--- a/Documentation/config/submodule.txt\n+++ b/Documentation/config/submodule.txt\n@@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::\n \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n \tor `info`, and if there is an error with the computed alternate, the\n \tclone proceeds as if no alternate was specified.\n+\n+submodule.superprojectGitDir::\n+\tThe relative path from the submodule's worktree to the superproject's\n+\tgitdir. This config should only be present in projects which are\n+\tsubmodules, but is not guaranteed to be present in every submodule. It\n+\tis set automatically during submodule creation.\n++\n+\tIn situations where more than one superproject references the same\n+\tsubmodule worktree, the value of this config and the behavior of\n+\toperations which use it are undefined. To reference a single project\n+\tfrom multiple superprojects, it is better to create a worktree of the\n+\tsubmodule for each superproject.\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..d60fcd2c7d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n \t\t\t\t\t error_strategy);\n \n+\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(absolute_path(get_git_dir()),\n+\t\t\t\t\t path, &sb));\n+\n \tfree(sm_alternate);\n \tfree(error_strategy);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex f5dc051a6e..e45f42588f 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '\n submodurl=$(pwd -P)\n \n inspect() {\n-\tdir=$1 &&\n-\n-\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n-\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n-\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n-\tgit -C \"$dir\" update-index --refresh &&\n-\tgit -C \"$dir\" diff-files --exit-code &&\n-\tgit -C \"$dir\" clean -n -d -x >untracked\n+\tsub_dir=$1 &&\n+\tsuper_dir=$2 &&\n+\n+\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$sub_dir\" update-index --refresh &&\n+\tgit -C \"$sub_dir\" diff-files --exit-code &&\n+\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n+\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n+\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n+\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n }\n \n \n@@ -139,7 +143,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod &&\n+\tinspect addtest/submod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch &&\n+\tinspect addtest/submod-branch addtest &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz &&\n+\tinspect addtest/dotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz &&\n+\tinspect addtest/dotslashdotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz &&\n+\tinspect addtest/slashslashsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod &&\n+\tinspect addtest/realsubmod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 &&\n+\tinspect addtest/realsubmod2 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 &&\n+\tinspect addtest/realsubmod3 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' '\n \tgit submodule update -q >update.out &&\n \ttest_must_be_empty update.out &&\n \n-\tinspect init &&\n+\tinspect init . &&\n \ttest_cmp expect head-sha1\n '\n \n@@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' '\n \techo \"$rev1\" >expect &&\n \n \tgit submodule update init &&\n-\tinspect init &&\n+\tinspect init . &&\n \n \ttest_cmp expect head-sha1\n '\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427550","messageId":"20210616004508.87186-4-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-1-emilyshaffer@google.com","subject":"[PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-16T00:45:07Z","receivedAt":"2021-06-16T00:45:25Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Already during 'git submodule add' we cache a pointer to the\nsuperproject's gitdir. However, this doesn't help brand-new\nsubmodules created with 'git init' and later absorbed with 'git\nsubmodule absorbgitdir'. Let's start adding that pointer during 'git\nsubmodule absorbgitdir' too.\n\nSigned-off-by: Emily Shaffer \n---\n submodule.c | 10 ++++++++++\n t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0b1d9c1dde..4b314bf09c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tchar *new_git_dir;\n \tconst struct submodule *sub;\n+\tstruct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;\n \n \tif (submodule_uses_worktrees(path))\n \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n@@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \n \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n \n+\t/* cache pointer to superproject's gitdir */\n+\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n+\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n+\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(get_super_prefix_or_empty(),\n+\t\t\t\t\t path, &sb));\n+\n+\tstrbuf_release(&config_path);\n+\tstrbuf_release(&sb);\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1cfa150768..e2d78e01df 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' '\n \tgit status >actual.1 &&\n \tgit -C sub1 rev-parse HEAD >actual.2 &&\n \ttest_cmp expect.1 actual.1 &&\n-\ttest_cmp expect.2 actual.2\n+\ttest_cmp expect.2 actual.2 &&\n+\n+\t# make sure the submodule cached the superproject gitdir correctly\n+\ttest-tool path-utils real_path . >expect &&\n+\ttest-tool path-utils real_path \\\n+\t\t\"$(git -C sub1 config submodule.superprojectGitDir)\" >actual &&\n+\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'absorbing does not fail for deinitialized submodules' '\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427551","messageId":"20210616004508.87186-5-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-1-emilyshaffer@google.com","subject":"[PATCH v2 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-16T00:45:08Z","receivedAt":"2021-06-16T00:45:30Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"A cached path to the superproject's gitdir might be added during 'git\nsubmodule add', but in some cases - like submodules which were created\nbefore 'git submodule add' learned to cache that info - it might be\nuseful to update the cache. Let's do it during 'git submodule update',\nwhen we already have a handle to the superproject while calling\noperations on the submodules.\n\nSigned-off-by: Emily Shaffer \n---\n git-submodule.sh | 10 ++++++++++\n t/t7406-submodule-update.sh | 10 ++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..f98dcc16ae 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -648,6 +648,16 @@ cmd_update()\n \t\t\tfi\n \t\tfi\n \n+\t\t# Cache a pointer to the superproject's gitdir. This may have\n+\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n+\t\t# if applicable, otherwise to local.\n+\t\trelative_gitdir=\"$(git rev-parse --path-format=relative \\\n+\t\t\t\t\t\t --prefix \"${sm_path}\" \\\n+\t\t\t\t\t\t --git-dir)\"\n+\n+\t\tgit -C \"$sm_path\" config --worktree \\\n+\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n+\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f4f61fe554..c39821ba8e 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s\n \t)\n '\n \n+test_expect_success 'submodule update adds superproject gitdir to older repos' '\n+\t(cd super &&\n+\t git -C submodule config --unset submodule.superprojectGitdir &&\n+\t git submodule update &&\n+\t echo \"../.git\" >expect &&\n+\t git -C submodule config submodule.superprojectGitdir >actual &&\n+\t test_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.32.0.272.g935e593368-goog\n\n"},{"id":"427574","messageId":"xmqqsg1iqv5n.fsf@gitster.g","threadId":"55906","inReplyTo":"20210616004508.87186-3-emilyshaffer@google.com","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T04:40:36Z","receivedAt":"2021-06-16T04:40:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Teach submodules a reference to their superproject's gitdir. This allows\n> us to A) know that we're running from a submodule, and B) have a\n> shortcut to the superproject's vitals, for example, configs.\n>\n> By using a relative path instead of an absolute path, we can move the\n> superproject directory around on the filesystem without breaking the\n> submodule's cache.\n>\n> Since this cached value is only introduced during new submodule creation\n> via `git submodule add`, though, there is more work to do to allow the\n> cache to be created at other times.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n> Documentation/config/submodule.txt | 12 +++++++++\n> builtin/submodule--helper.c | 4 +++\n> t/t7400-submodule-basic.sh | 40 ++++++++++++++++--------------\n> 3 files changed, 38 insertions(+), 18 deletions(-)\n>\n> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> index d7a63c8c12..7c459cc19e 100644\n> --- a/Documentation/config/submodule.txt\n> +++ b/Documentation/config/submodule.txt\n> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::\n> \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n> \tor `info`, and if there is an error with the computed alternate, the\n> \tclone proceeds as if no alternate was specified.\n> +\n> +submodule.superprojectGitDir::\n> +\tThe relative path from the submodule's worktree to the superproject's\n> +\tgitdir. This config should only be present in projects which are\n> +\tsubmodules, but is not guaranteed to be present in every submodule. It\n> +\tis set automatically during submodule creation.\n> ++\n> +\tIn situations where more than one superproject references the same\n> +\tsubmodule worktree, the value of this config and the behavior of\n> +\toperations which use it are undefined. To reference a single project\n> +\tfrom multiple superprojects, it is better to create a worktree of the\n> +\tsubmodule for each superproject.\n\nYou'd need to dedent the second paragraph that follows a lone '+'\nsign to typeset this correctly.\n\nThe new paragraph suggests separate worktrees for the same submodule\nrepository, but for that to work correctly,\n\n - \"git clone [--recurse-submodules]\" that clones the second\n superproject that shares the same submodule with a superproject\n that we already locally has to support a way for users to tell\n where to grab that existing submodule from and arrange a new\n worktree, instead of creating another instance of the submodule\n repository by cloning it afresh.\n\n - The \"submodule.superprojectGitDir\" needs to be set to\n per-worktree half of the repo-local configuration file.\n\nBecause I usually do not pay much attention to the submodule part of\nthe toolset, I may well be mistaken, but I suspect that the former\ndoes not exist yet. If I recall correctly, the latter was a NEEDSWORK\nitem in the previous round of this patchset?\n\nAs I said, I think it is OK for now to stop at declaring that you\ncannot simply do it without hinting something that may not fully\nwork.\n\nThanks.\n"},{"id":"427575","messageId":"xmqqk0muqv0n.fsf@gitster.g","threadId":"55906","inReplyTo":"xmqqsg1iqv5n.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-16T04:43:36Z","receivedAt":"2021-06-16T04:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As I said, I think it is OK for now to stop at declaring that you\n> cannot simply do it without hinting something that may not fully\n> work.\n\nI'll add the following to the tip of the topic for now.\n\n\n Documentation/config/submodule.txt | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt\nindex 7c459cc19e..58ba63a75e 100644\n--- c/Documentation/config/submodule.txt\n+++ w/Documentation/config/submodule.txt\n@@ -97,8 +97,5 @@ submodule.superprojectGitDir::\n \tsubmodules, but is not guaranteed to be present in every submodule. It\n \tis set automatically during submodule creation.\n +\n-\tIn situations where more than one superproject references the same\n-\tsubmodule worktree, the value of this config and the behavior of\n-\toperations which use it are undefined. To reference a single project\n-\tfrom multiple superprojects, it is better to create a worktree of the\n-\tsubmodule for each superproject.\n+Because of this configuration variable, it is forbidden to use the\n+same submodule worktree shared by multiple superprojects.\n"},{"id":"427800","messageId":"YMviMFy4yCFXFiEv@google.com","threadId":"55906","inReplyTo":"xmqqsg1iqv5n.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-18T00:00:48Z","receivedAt":"2021-06-18T00:00:58Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 16, 2021 at 01:40:36PM +0900, Junio C Hamano wrote:\n> \n> Emily Shaffer writes:\n> > +submodule.superprojectGitDir::\n> > +\tThe relative path from the submodule's worktree to the superproject's\n> > +\tgitdir. This config should only be present in projects which are\n> > +\tsubmodules, but is not guaranteed to be present in every submodule. It\n> > +\tis set automatically during submodule creation.\n> > ++\n> > +\tIn situations where more than one superproject references the same\n> > +\tsubmodule worktree, the value of this config and the behavior of\n> > +\toperations which use it are undefined. To reference a single project\n> > +\tfrom multiple superprojects, it is better to create a worktree of the\n> > +\tsubmodule for each superproject.\n> \n> You'd need to dedent the second paragraph that follows a lone '+'\n> sign to typeset this correctly.\n\nOk.\n\n> \n> The new paragraph suggests separate worktrees for the same submodule\n> repository, but for that to work correctly,\n> \n> - \"git clone [--recurse-submodules]\" that clones the second\n> superproject that shares the same submodule with a superproject\n> that we already locally has to support a way for users to tell\n> where to grab that existing submodule from and arrange a new\n> worktree, instead of creating another instance of the submodule\n> repository by cloning it afresh.\n> \n> - The \"submodule.superprojectGitDir\" needs to be set to\n> per-worktree half of the repo-local configuration file.\n> \n> Because I usually do not pay much attention to the submodule part of\n> the toolset, I may well be mistaken, but I suspect that the former\n> does not exist yet. If I recall correctly, the latter was a NEEDSWORK\n> item in the previous round of this patchset?\n> \n> As I said, I think it is OK for now to stop at declaring that you\n> cannot simply do it without hinting something that may not fully\n> work.\n\nYeah, that is all correct. Ok, I will drop the broken suggestion.\n\n - Emily\n"},{"id":"427801","messageId":"YMvixSHYdEJB2qTY@google.com","threadId":"55906","inReplyTo":"xmqqk0muqv0n.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-18T00:03:17Z","receivedAt":"2021-06-18T00:03:26Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Wed, Jun 16, 2021 at 01:43:36PM +0900, Junio C Hamano wrote:\n> \n> Junio C Hamano writes:\n> \n> > As I said, I think it is OK for now to stop at declaring that you\n> > cannot simply do it without hinting something that may not fully\n> > work.\n> \n> I'll add the following to the tip of the topic for now.\n\nJust saw this - yes, it looks fine to me. I'll squash that locally in\ncase anybody has more comments and wants a reroll.\n\n> \n> \n> Documentation/config/submodule.txt | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt\n> index 7c459cc19e..58ba63a75e 100644\n> --- c/Documentation/config/submodule.txt\n> +++ w/Documentation/config/submodule.txt\n> @@ -97,8 +97,5 @@ submodule.superprojectGitDir::\n> \tsubmodules, but is not guaranteed to be present in every submodule. It\n> \tis set automatically during submodule creation.\n> +\n> -\tIn situations where more than one superproject references the same\n> -\tsubmodule worktree, the value of this config and the behavior of\n> -\toperations which use it are undefined. To reference a single project\n> -\tfrom multiple superprojects, it is better to create a worktree of the\n> -\tsubmodule for each superproject.\n> +Because of this configuration variable, it is forbidden to use the\n> +same submodule worktree shared by multiple superprojects.\n"},{"id":"431319","messageId":"20210727171225.2457893-1-jonathantanmy@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-2-emilyshaffer@google.com","subject":"Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-27T17:12:25Z","receivedAt":"2021-07-27T17:12:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index a924fdb7a6..f5dc051a6e 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '\n> # generates, which will expand symbolic links.\n> submodurl=$(pwd -P)\n> \n> -listbranches() {\n> -\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n> -}\n> -\n> inspect() {\n> \tdir=$1 &&\n> -\tdotdot=\"${2:-..}\" &&\n> \n> -\t(\n> -\t\tcd \"$dir\" &&\n> -\t\tlistbranches >\"$dotdot/heads\" &&\n> -\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n> -\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n> -\t\tgit update-index --refresh &&\n> -\t\tgit diff-files --exit-code &&\n> -\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n> -\t)\n> +\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> +\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> +\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> +\tgit -C \"$dir\" update-index --refresh &&\n> +\tgit -C \"$dir\" diff-files --exit-code &&\n> +\tgit -C \"$dir\" clean -n -d -x >untracked\n> }\n> \n> +\n\nStray extra line.\n\nOther than that, this is definitely a good simplification.\n"},{"id":"431328","messageId":"20210727174650.2462099-1-jonathantanmy@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-3-emilyshaffer@google.com","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-27T17:46:50Z","receivedAt":"2021-07-27T17:46:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Teach submodules a reference to their superproject's gitdir. This allows\n> us to A) know that we're running from a submodule, and B) have a\n> shortcut to the superproject's vitals, for example, configs.\n\nThe first sentence is probably better \"Introduce a new config variable\nstoring a submodule's reference to its superproject's gitdir, and teach\n'git submodule add' to write it\".\n\nAlso, I think there should be more detail about the planned use both\nhere in the commit message and in the config documentation. Is the plan\njust to use it for best-effort explanatory messages? (Using it as a true\ncache is probably too performance-intensive, I would think, since in its\nabsence, we have no idea whether the repo is a submodule and would\nalways have to search to the root of the filesystem.) If it is just for\nbest-effort explanatory messages, maybe write:\n\n If present, commands like \"git status\" in this submodule may print\n additional information about this submodule's status with respect to\n its superproject.\n\nThis would further reinforce that this variable being missing is OK.\n\n> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> index d7a63c8c12..7c459cc19e 100644\n> --- a/Documentation/config/submodule.txt\n> +++ b/Documentation/config/submodule.txt\n> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::\n> \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n> \tor `info`, and if there is an error with the computed alternate, the\n> \tclone proceeds as if no alternate was specified.\n> +\n> +submodule.superprojectGitDir::\n> +\tThe relative path from the submodule's worktree to the superproject's\n> +\tgitdir. This config should only be present in projects which are\n> +\tsubmodules, but is not guaranteed to be present in every submodule. It\n> +\tis set automatically during submodule creation.\n> ++\n> +\tIn situations where more than one superproject references the same\n> +\tsubmodule worktree, the value of this config and the behavior of\n> +\toperations which use it are undefined. To reference a single project\n> +\tfrom multiple superprojects, it is better to create a worktree of the\n> +\tsubmodule for each superproject.\n\nSo my suggestion would be:\n\n The relative path from this repository's worktree to its\n superproject's gitdir. When Git is run in a repository, it usually\n makes no difference whether this repository is standalone or a\n submodule, but if this configuration variable is present, commands\n like \"git status\" in this submodule may print additional information\n about this submodule's status with respect to its superproject.\n\n(I believe Junio has commented on the second paragraph - I don't have\nadditional comments on that.)\n"},{"id":"431330","messageId":"20210727175151.2462983-1-jonathantanmy@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-5-emilyshaffer@google.com","subject":"Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-27T17:51:51Z","receivedAt":"2021-07-27T17:51:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> A cached path to the superproject's gitdir might be added during 'git\n\n[snip]\n\n> +\t\t# Cache a pointer to the superproject's gitdir. This may have\n\nPatches 3 and 4 look good to me except that for me, a cache is something\nthat lets us avoid an expensive computation. But as far as I know, we're\nnot planning to perform the expensive computation in the absence of a\ncache. Maybe the word \"record\" would fit better.\n\nThis is nit-picky, though, and if others think that the word \"cache\"\nfits here, that's fine by me too.\n"},{"id":"433138","messageId":"YR6Y91D2Yt5K5vaS@google.com","threadId":"55906","inReplyTo":"20210727171225.2457893-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T17:46:31Z","receivedAt":"2021-08-19T17:46:45Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 27, 2021 at 10:12:25AM -0700, Jonathan Tan wrote:\n> \n> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> > index a924fdb7a6..f5dc051a6e 100755\n> > --- a/t/t7400-submodule-basic.sh\n> > +++ b/t/t7400-submodule-basic.sh\n> > @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' '\n> > # generates, which will expand symbolic links.\n> > submodurl=$(pwd -P)\n> > \n> > -listbranches() {\n> > -\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n> > -}\n> > -\n> > inspect() {\n> > \tdir=$1 &&\n> > -\tdotdot=\"${2:-..}\" &&\n> > \n> > -\t(\n> > -\t\tcd \"$dir\" &&\n> > -\t\tlistbranches >\"$dotdot/heads\" &&\n> > -\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n> > -\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n> > -\t\tgit update-index --refresh &&\n> > -\t\tgit diff-files --exit-code &&\n> > -\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n> > -\t)\n> > +\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> > +\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> > +\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> > +\tgit -C \"$dir\" update-index --refresh &&\n> > +\tgit -C \"$dir\" diff-files --exit-code &&\n> > +\tgit -C \"$dir\" clean -n -d -x >untracked\n> > }\n> > \n> > +\n> \n> Stray extra line.\n> \n> Other than that, this is definitely a good simplification.\n\nAck, fixed locally. Thanks.\n\n - Emily\n"},{"id":"433139","messageId":"YR6agG3pwNJpCakh@google.com","threadId":"55906","inReplyTo":"20210727174650.2462099-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T17:53:04Z","receivedAt":"2021-08-19T17:53:15Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 27, 2021 at 10:46:50AM -0700, Jonathan Tan wrote:\n> \n> > Teach submodules a reference to their superproject's gitdir. This allows\n> > us to A) know that we're running from a submodule, and B) have a\n> > shortcut to the superproject's vitals, for example, configs.\n> \n> The first sentence is probably better \"Introduce a new config variable\n> storing a submodule's reference to its superproject's gitdir, and teach\n> 'git submodule add' to write it\".\n> \n> Also, I think there should be more detail about the planned use both\n> here in the commit message and in the config documentation. Is the plan\n> just to use it for best-effort explanatory messages? (Using it as a true\n> cache is probably too performance-intensive, I would think, since in its\n> absence, we have no idea whether the repo is a submodule and would\n> always have to search to the root of the filesystem.) If it is just for\n> best-effort explanatory messages, maybe write:\n> \n> If present, commands like \"git status\" in this submodule may print\n> additional information about this submodule's status with respect to\n> its superproject.\n> \n> This would further reinforce that this variable being missing is OK.\n\nOk, I'll expand the commit message. The first use case for this extra\npointer is for a shared config between superproject and submodule, which\nI've sent a series for already; I'll mention that in the commit message\ntoo.\n\n> \n> > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> > index d7a63c8c12..7c459cc19e 100644\n> > --- a/Documentation/config/submodule.txt\n> > +++ b/Documentation/config/submodule.txt\n> > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::\n> > \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n> > \tor `info`, and if there is an error with the computed alternate, the\n> > \tclone proceeds as if no alternate was specified.\n> > +\n> > +submodule.superprojectGitDir::\n> > +\tThe relative path from the submodule's worktree to the superproject's\n> > +\tgitdir. This config should only be present in projects which are\n> > +\tsubmodules, but is not guaranteed to be present in every submodule. It\n> > +\tis set automatically during submodule creation.\n> > ++\n> > +\tIn situations where more than one superproject references the same\n> > +\tsubmodule worktree, the value of this config and the behavior of\n> > +\toperations which use it are undefined. To reference a single project\n> > +\tfrom multiple superprojects, it is better to create a worktree of the\n> > +\tsubmodule for each superproject.\n> \n> So my suggestion would be:\n> \n> The relative path from this repository's worktree to its\n> superproject's gitdir. When Git is run in a repository, it usually\n> makes no difference whether this repository is standalone or a\n> submodule, but if this configuration variable is present, commands\n> like \"git status\" in this submodule may print additional information\n> about this submodule's status with respect to its superproject.\n> \n> (I believe Junio has commented on the second paragraph - I don't have\n> additional comments on that.)\n\nThe spirit of this suggestion seems to be \"describe a possible side\neffect of this config\", so I'll do that, although the wording may not be\nexactly the same. Thanks.\n\n - Emily\n"},{"id":"433141","messageId":"YR6cqbGiPRXZ9GxU@google.com","threadId":"55906","inReplyTo":"20210727175151.2462983-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T18:02:17Z","receivedAt":"2021-08-19T18:02:28Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jul 27, 2021 at 10:51:51AM -0700, Jonathan Tan wrote:\n> \n> > A cached path to the superproject's gitdir might be added during 'git\n> \n> [snip]\n> \n> > +\t\t# Cache a pointer to the superproject's gitdir. This may have\n> \n> Patches 3 and 4 look good to me except that for me, a cache is something\n> that lets us avoid an expensive computation. But as far as I know, we're\n> not planning to perform the expensive computation in the absence of a\n> cache. Maybe the word \"record\" would fit better.\n> \n> This is nit-picky, though, and if others think that the word \"cache\"\n> fits here, that's fine by me too.\n\nThanks. I switched the commit messages around to use 'record' and 'hint'\nwhen appropriate instead. I'll send a reroll with those nits as soon as\nit passes CI.\n\n - Emily\n"},{"id":"433158","messageId":"20210819200953.2105230-1-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210616004508.87186-1-emilyshaffer@google.com","subject":"[PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T20:09:49Z","receivedAt":"2021-08-19T20:10:04Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Since v2, mostly documentation changes and a handful of small nits from\nJunio and Jonathan Tan. Thanks for the reviews, both.\n\nCI run here: https://github.com/nasamuffin/git/actions/runs/1147984974\n\n - Emily\n\nEmily Shaffer (4):\n t7400-submodule-basic: modernize inspect() helper\n introduce submodule.superprojectGitDir record\n submodule: record superproject gitdir during absorbgitdirs\n submodule: record superproject gitdir during 'update'\n\n Documentation/config/submodule.txt | 15 ++++++++++\n builtin/submodule--helper.c | 4 +++\n git-submodule.sh | 10 +++++++\n submodule.c | 10 +++++++\n t/t7400-submodule-basic.sh | 48 ++++++++++++++----------------\n t/t7406-submodule-update.sh | 10 +++++++\n t/t7412-submodule-absorbgitdirs.sh | 9 +++++-\n 7 files changed, 79 insertions(+), 27 deletions(-)\n\nRange-diff against v2:\n1: a6718eea80 ! 1: f1236dc9d7 t7400-submodule-basic: modernize inspect() helper\n @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo\n +\tgit -C \"$dir\" clean -n -d -x >untracked\n }\n \n -+\n test_expect_success 'submodule add' '\n - \techo \"refs/heads/main\" >expect &&\n - \n @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' '\n \t) &&\n \n2: 4cebe7bcb5 ! 2: 2caf9eb8ee introduce submodule.superprojectGitDir cache\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - introduce submodule.superprojectGitDir cache\n + introduce submodule.superprojectGitDir record\n \n Teach submodules a reference to their superproject's gitdir. This allows\n us to A) know that we're running from a submodule, and B) have a\n @@ Commit message\n superproject directory around on the filesystem without breaking the\n submodule's cache.\n \n - Since this cached value is only introduced during new submodule creation\n + Since this hint value is only introduced during new submodule creation\n via `git submodule add`, though, there is more work to do to allow the\n - cache to be created at other times.\n + record to be created at other times.\n +\n + If the new config is present, we can do some optional value-added\n + behavior, like letting \"git status\" print additional info about the\n + submodule's status in relation to its superproject, or like letting the\n + superproject and submodule share an additional config file separate from\n + either one's local config.\n \n Signed-off-by: Emily Shaffer \n + Helped-by: Junio C Hamano \n \n ## Documentation/config/submodule.txt ##\n @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::\n @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy::\n \tclone proceeds as if no alternate was specified.\n +\n +submodule.superprojectGitDir::\n -+\tThe relative path from the submodule's worktree to the superproject's\n -+\tgitdir. This config should only be present in projects which are\n -+\tsubmodules, but is not guaranteed to be present in every submodule. It\n -+\tis set automatically during submodule creation.\n ++\tThe relative path from the submodule's worktree to its superproject's\n ++\tgitdir. When Git is run in a repository, it usually makes no difference\n ++\twhether this repository is standalone or a submodule, but if this\n ++\tconfiguration variable is present, additional behavior may be possible,\n ++\tsuch as \"git status\" printing additional information about this\n ++\tsubmodule's status with respect to its superproject. This config should\n ++\tonly be present in projects which are submodules, but is not guaranteed\n ++\tto be present in every submodule, so only optional value-added behavior\n ++\tshould be linked to it. It is set automatically during\n ++\tsubmodule creation.\n ++\n -+\tIn situations where more than one superproject references the same\n -+\tsubmodule worktree, the value of this config and the behavior of\n -+\toperations which use it are undefined. To reference a single project\n -+\tfrom multiple superprojects, it is better to create a worktree of the\n -+\tsubmodule for each superproject.\n ++\tBecause of this configuration variable, it is forbidden to use the\n ++\tsame submodule worktree shared by multiple superprojects.\n \n ## builtin/submodule--helper.c ##\n @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)\n @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo\n +\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n }\n \n - \n + test_expect_success 'submodule add' '\n @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' '\n \t) &&\n \n3: df97a9c2bb ! 3: d278568a8e submodule: cache superproject gitdir during absorbgitdirs\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - submodule: cache superproject gitdir during absorbgitdirs\n + submodule: record superproject gitdir during absorbgitdirs\n \n - Already during 'git submodule add' we cache a pointer to the\n + Already during 'git submodule add' we record a pointer to the\n superproject's gitdir. However, this doesn't help brand-new\n submodules created with 'git init' and later absorbed with 'git\n submodule absorbgitdir'. Let's start adding that pointer during 'git\n4: a3f3be58ad ! 4: 6866c36620 submodule: cache superproject gitdir during 'update'\n @@ Metadata\n Author: Emily Shaffer \n \n ## Commit message ##\n - submodule: cache superproject gitdir during 'update'\n + submodule: record superproject gitdir during 'update'\n \n - A cached path to the superproject's gitdir might be added during 'git\n - submodule add', but in some cases - like submodules which were created\n - before 'git submodule add' learned to cache that info - it might be\n - useful to update the cache. Let's do it during 'git submodule update',\n - when we already have a handle to the superproject while calling\n + A recorded hint path to the superproject's gitdir might be added during\n + 'git submodule add', but in some cases - like submodules which were\n + created before 'git submodule add' learned to record that info - it might\n + be useful to update the hint. Let's do it during 'git submodule\n + update', when we already have a handle to the superproject while calling\n operations on the submodules.\n \n Signed-off-by: Emily Shaffer \n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433159","messageId":"20210819200953.2105230-2-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"[PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T20:09:50Z","receivedAt":"2021-08-19T20:10:05Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Since the inspect() helper in the submodule-basic test suite was\nwritten, 'git -C ' was added. By using -C, we no longer need a\nreference to the base directory for the test. This simplifies callsites,\nand will make the addition of other arguments in later patches more\nreadable.\n\nSigned-off-by: Emily Shaffer \n---\n t/t7400-submodule-basic.sh | 36 ++++++++++++++----------------------\n 1 file changed, 14 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..4bc6b6c886 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -107,23 +107,15 @@ test_expect_success 'setup - repository to add submodules to' '\n # generates, which will expand symbolic links.\n submodurl=$(pwd -P)\n \n-listbranches() {\n-\tgit for-each-ref --format='%(refname)' 'refs/heads/*'\n-}\n-\n inspect() {\n \tdir=$1 &&\n-\tdotdot=\"${2:-..}\" &&\n \n-\t(\n-\t\tcd \"$dir\" &&\n-\t\tlistbranches >\"$dotdot/heads\" &&\n-\t\t{ git symbolic-ref HEAD || :; } >\"$dotdot/head\" &&\n-\t\tgit rev-parse HEAD >\"$dotdot/head-sha1\" &&\n-\t\tgit update-index --refresh &&\n-\t\tgit diff-files --exit-code &&\n-\t\tgit clean -n -d -x >\"$dotdot/untracked\"\n-\t)\n+\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$dir\" update-index --refresh &&\n+\tgit -C \"$dir\" diff-files --exit-code &&\n+\tgit -C \"$dir\" clean -n -d -x >untracked\n }\n \n test_expect_success 'submodule add' '\n@@ -146,7 +138,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod ../.. &&\n+\tinspect addtest/submod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -237,7 +229,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch ../.. &&\n+\tinspect addtest/submod-branch &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -253,7 +245,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -269,7 +261,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz ../../.. &&\n+\tinspect addtest/dotslashdotsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -285,7 +277,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz ../../.. &&\n+\tinspect addtest/slashslashsubmod/frotz &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -301,7 +293,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod ../.. &&\n+\tinspect addtest/realsubmod &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -317,7 +309,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 ../.. &&\n+\tinspect addtest/realsubmod2 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -348,7 +340,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 ../.. &&\n+\tinspect addtest/realsubmod3 &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433160","messageId":"20210819200953.2105230-3-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"[PATCH v3 2/4] introduce submodule.superprojectGitDir record","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T20:09:51Z","receivedAt":"2021-08-19T20:10:12Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Teach submodules a reference to their superproject's gitdir. This allows\nus to A) know that we're running from a submodule, and B) have a\nshortcut to the superproject's vitals, for example, configs.\n\nBy using a relative path instead of an absolute path, we can move the\nsuperproject directory around on the filesystem without breaking the\nsubmodule's cache.\n\nSince this hint value is only introduced during new submodule creation\nvia `git submodule add`, though, there is more work to do to allow the\nrecord to be created at other times.\n\nIf the new config is present, we can do some optional value-added\nbehavior, like letting \"git status\" print additional info about the\nsubmodule's status in relation to its superproject, or like letting the\nsuperproject and submodule share an additional config file separate from\neither one's local config.\n\nSigned-off-by: Emily Shaffer \nHelped-by: Junio C Hamano \n---\n Documentation/config/submodule.txt | 15 +++++++++++\n builtin/submodule--helper.c | 4 +++\n t/t7400-submodule-basic.sh | 40 ++++++++++++++++--------------\n 3 files changed, 41 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\nindex d7a63c8c12..23e0a01d90 100644\n--- a/Documentation/config/submodule.txt\n+++ b/Documentation/config/submodule.txt\n@@ -90,3 +90,18 @@ submodule.alternateErrorStrategy::\n \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n \tor `info`, and if there is an error with the computed alternate, the\n \tclone proceeds as if no alternate was specified.\n+\n+submodule.superprojectGitDir::\n+\tThe relative path from the submodule's worktree to its superproject's\n+\tgitdir. When Git is run in a repository, it usually makes no difference\n+\twhether this repository is standalone or a submodule, but if this\n+\tconfiguration variable is present, additional behavior may be possible,\n+\tsuch as \"git status\" printing additional information about this\n+\tsubmodule's status with respect to its superproject. This config should\n+\tonly be present in projects which are submodules, but is not guaranteed\n+\tto be present in every submodule, so only optional value-added behavior\n+\tshould be linked to it. It is set automatically during\n+\tsubmodule creation.\n++\n+\tBecause of this configuration variable, it is forbidden to use the\n+\tsame submodule worktree shared by multiple superprojects.\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..d60fcd2c7d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n \t\t\t\t\t error_strategy);\n \n+\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(absolute_path(get_git_dir()),\n+\t\t\t\t\t path, &sb));\n+\n \tfree(sm_alternate);\n \tfree(error_strategy);\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 4bc6b6c886..e407329d81 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '\n submodurl=$(pwd -P)\n \n inspect() {\n-\tdir=$1 &&\n-\n-\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n-\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n-\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n-\tgit -C \"$dir\" update-index --refresh &&\n-\tgit -C \"$dir\" diff-files --exit-code &&\n-\tgit -C \"$dir\" clean -n -d -x >untracked\n+\tsub_dir=$1 &&\n+\tsuper_dir=$2 &&\n+\n+\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n+\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n+\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n+\tgit -C \"$sub_dir\" update-index --refresh &&\n+\tgit -C \"$sub_dir\" diff-files --exit-code &&\n+\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n+\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n+\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n+\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n }\n \n test_expect_success 'submodule add' '\n@@ -138,7 +142,7 @@ test_expect_success 'submodule add' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod &&\n+\tinspect addtest/submod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -229,7 +233,7 @@ test_expect_success 'submodule add --branch' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/submod-branch &&\n+\tinspect addtest/submod-branch addtest &&\n \ttest_cmp expect-heads heads &&\n \ttest_cmp expect-head head &&\n \ttest_must_be_empty untracked\n@@ -245,7 +249,7 @@ test_expect_success 'submodule add with ./ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotsubmod/frotz &&\n+\tinspect addtest/dotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -261,7 +265,7 @@ test_expect_success 'submodule add with /././ in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/dotslashdotsubmod/frotz &&\n+\tinspect addtest/dotslashdotsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -277,7 +281,7 @@ test_expect_success 'submodule add with // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/slashslashsubmod/frotz &&\n+\tinspect addtest/slashslashsubmod/frotz addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -293,7 +297,7 @@ test_expect_success 'submodule add with /.. in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod &&\n+\tinspect addtest/realsubmod addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -309,7 +313,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod2 &&\n+\tinspect addtest/realsubmod2 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -340,7 +344,7 @@ test_expect_success 'submodule add in subdirectory' '\n \t) &&\n \n \trm -f heads head untracked &&\n-\tinspect addtest/realsubmod3 &&\n+\tinspect addtest/realsubmod3 addtest &&\n \ttest_cmp expect heads &&\n \ttest_cmp expect head &&\n \ttest_must_be_empty untracked\n@@ -481,7 +485,7 @@ test_expect_success 'update should work when path is an empty dir' '\n \tgit submodule update -q >update.out &&\n \ttest_must_be_empty update.out &&\n \n-\tinspect init &&\n+\tinspect init . &&\n \ttest_cmp expect head-sha1\n '\n \n@@ -540,7 +544,7 @@ test_expect_success 'update should checkout rev1' '\n \techo \"$rev1\" >expect &&\n \n \tgit submodule update init &&\n-\tinspect init &&\n+\tinspect init . &&\n \n \ttest_cmp expect head-sha1\n '\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433161","messageId":"20210819200953.2105230-4-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"[PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T20:09:52Z","receivedAt":"2021-08-19T20:10:13Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Already during 'git submodule add' we record a pointer to the\nsuperproject's gitdir. However, this doesn't help brand-new\nsubmodules created with 'git init' and later absorbed with 'git\nsubmodule absorbgitdir'. Let's start adding that pointer during 'git\nsubmodule absorbgitdir' too.\n\nSigned-off-by: Emily Shaffer \n---\n submodule.c | 10 ++++++++++\n t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 0b1d9c1dde..4b314bf09c 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n \tchar *new_git_dir;\n \tconst struct submodule *sub;\n+\tstruct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;\n \n \tif (submodule_uses_worktrees(path))\n \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n@@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n \n \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n \n+\t/* cache pointer to superproject's gitdir */\n+\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n+\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n+\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n+\t\t\t relative_path(get_super_prefix_or_empty(),\n+\t\t\t\t\t path, &sb));\n+\n+\tstrbuf_release(&config_path);\n+\tstrbuf_release(&sb);\n \tfree(old_git_dir);\n \tfree(real_old_git_dir);\n \tfree(real_new_git_dir);\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex 1cfa150768..e2d78e01df 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' '\n \tgit status >actual.1 &&\n \tgit -C sub1 rev-parse HEAD >actual.2 &&\n \ttest_cmp expect.1 actual.1 &&\n-\ttest_cmp expect.2 actual.2\n+\ttest_cmp expect.2 actual.2 &&\n+\n+\t# make sure the submodule cached the superproject gitdir correctly\n+\ttest-tool path-utils real_path . >expect &&\n+\ttest-tool path-utils real_path \\\n+\t\t\"$(git -C sub1 config submodule.superprojectGitDir)\" >actual &&\n+\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'absorbing does not fail for deinitialized submodules' '\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433162","messageId":"20210819200953.2105230-5-emilyshaffer@google.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"[PATCH v3 4/4] submodule: record superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-08-19T20:09:53Z","receivedAt":"2021-08-19T20:10:14Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"A recorded hint path to the superproject's gitdir might be added during\n'git submodule add', but in some cases - like submodules which were\ncreated before 'git submodule add' learned to record that info - it might\nbe useful to update the hint. Let's do it during 'git submodule\nupdate', when we already have a handle to the superproject while calling\noperations on the submodules.\n\nSigned-off-by: Emily Shaffer \n---\n git-submodule.sh | 10 ++++++++++\n t/t7406-submodule-update.sh | 10 ++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..f98dcc16ae 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -648,6 +648,16 @@ cmd_update()\n \t\t\tfi\n \t\tfi\n \n+\t\t# Cache a pointer to the superproject's gitdir. This may have\n+\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n+\t\t# if applicable, otherwise to local.\n+\t\trelative_gitdir=\"$(git rev-parse --path-format=relative \\\n+\t\t\t\t\t\t --prefix \"${sm_path}\" \\\n+\t\t\t\t\t\t --git-dir)\"\n+\n+\t\tgit -C \"$sm_path\" config --worktree \\\n+\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n+\n \t\tif test -n \"$recursive\"\n \t\tthen\n \t\t\t(\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f4f61fe554..c39821ba8e 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s\n \t)\n '\n \n+test_expect_success 'submodule update adds superproject gitdir to older repos' '\n+\t(cd super &&\n+\t git -C submodule config --unset submodule.superprojectGitdir &&\n+\t git submodule update &&\n+\t echo \"../.git\" >expect &&\n+\t git -C submodule config submodule.superprojectGitdir >actual &&\n+\t test_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.33.0.rc2.250.ged5fa647cd-goog\n\n"},{"id":"433174","messageId":"xmqqwnohxfnh.fsf@gitster.g","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T21:56:18Z","receivedAt":"2021-08-19T21:56:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> Since v2, mostly documentation changes and a handful of small nits from\n> Junio and Jonathan Tan. Thanks for the reviews, both.\n\nWill queue, but ...\n\n> +submodule.superprojectGitDir::\n> -+\tThe relative path from the submodule's worktree to the superproject's\n> -+\tgitdir. This config should only be present in projects which are\n> -+\tsubmodules, but is not guaranteed to be present in every submodule. It\n> -+\tis set automatically during submodule creation.\n> ++\tThe relative path from the submodule's worktree to its superproject's\n> ++\tgitdir. When Git is run in a repository, it usually makes no difference\n> ++\twhether this repository is standalone or a submodule, but if this\n> ++\tconfiguration variable is present, additional behavior may be possible,\n> ++\tsuch as \"git status\" printing additional information about this\n> ++\tsubmodule's status with respect to its superproject. This config should\n> ++\tonly be present in projects which are submodules, but is not guaranteed\n> ++\tto be present in every submodule, so only optional value-added behavior\n> ++\tshould be linked to it. It is set automatically during\n> ++\tsubmodule creation.\n> ++\n> -+\tIn situations where more than one superproject references the same\n> -+\tsubmodule worktree, the value of this config and the behavior of\n> -+\toperations which use it are undefined. To reference a single project\n> -+\tfrom multiple superprojects, it is better to create a worktree of the\n> -+\tsubmodule for each superproject.\n> ++\tBecause of this configuration variable, it is forbidden to use the\n> ++\tsame submodule worktree shared by multiple superprojects.\n\n... I think this will regress the format from what I've queued in\n'seen' unless you dedent the \"Because of this...\" paragraph. It is\nunfortunate but AsciiDoc wants the follow-up paragraphs that follow\nthe single '+' line to start at the left edge without indentation.\n\n"},{"id":"433189","messageId":"9fecf160-9646-7e56-478c-aa5f8defa6a9@gmail.com","threadId":"55906","inReplyTo":"20210819200953.2105230-3-emilyshaffer@google.com","subject":"Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T00:38:19Z","receivedAt":"2021-08-20T00:38:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n...\n> +submodule.superprojectGitDir::\n> +\tThe relative path from the submodule's worktree to its superproject's\n> +\tgitdir. When Git is run in a repository, it usually makes no difference\n> +\twhether this repository is standalone or a submodule, but if this\n> +\tconfiguration variable is present, additional behavior may be possible,\n> +\tsuch as \"git status\" printing additional information about this\n> +\tsubmodule's status with respect to its superproject. This config should\n> +\tonly be present in projects which are submodules, but is not guaranteed\n> +\tto be present in every submodule, so only optional value-added behavior\n> +\tshould be linked to it. It is set automatically during\n> +\tsubmodule creation.\n> ++\n> +\tBecause of this configuration variable, it is forbidden to use the\n> +\tsame submodule worktree shared by multiple superprojects.\n\nnit: this paragraph linked with the \"+\" line should have no tabbing.\n\nAlso, could we use the same submodule worktree for multiple superprojects\n_before_ this configuration variable? That seems wild to me. Or, is that\nnot a new requirement?\n\nPerhaps you mean something like this instead:\n\n\tIt is forbidden to use the same submodule worktree for multiple\n\tsuperprojects, so this configuration variable stores the unique\n\tsuperproject and is not multi-valued.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d55f6262e9..d60fcd2c7d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> \t\t\t\t\t error_strategy);\n> \n> +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(absolute_path(get_git_dir()),\n> +\t\t\t\t\t path, &sb));\n> +\n\nI see that all new submodules will have this configuration set. But we will\nalso live in a world where some existing submodules do not have this variable\nset. I'll look elsewhere for compatibility checks.\n\n> inspect() {\n> -\tdir=$1 &&\n> -\n> -\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> -\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> -\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> -\tgit -C \"$dir\" update-index --refresh &&\n> -\tgit -C \"$dir\" diff-files --exit-code &&\n> -\tgit -C \"$dir\" clean -n -d -x >untracked\n> +\tsub_dir=$1 &&\n> +\tsuper_dir=$2 &&\n> +\n> +\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> +\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n> +\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n> +\tgit -C \"$sub_dir\" update-index --refresh &&\n> +\tgit -C \"$sub_dir\" diff-files --exit-code &&\n> +\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n> +\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n> +\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n> +\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n\nYou rewrote this test in the previous patch, and now every line is changed\nbecause you renamed 'dir' to 'sub_dir'. Could the previous patch use\n'sub_dir' from the start so this change only shows the new lines instead of\nmany edited lines?\n\n> }\n> \n> test_expect_success 'submodule add' '\n> @@ -138,7 +142,7 @@ test_expect_success 'submodule add' '\n> \t) &&\n> \n> \trm -f heads head untracked &&\n> -\tinspect addtest/submod &&\n> +\tinspect addtest/submod addtest &&\n\nSimilarly, I would not be upset to see these lines be changed just the\nonce, even if the second argument is ignored for a single commit. But\nthis nitpick is definitely less important since I could see taste\nswaying things either way.\n\nThanks,\n-Stolee\n"},{"id":"433194","messageId":"ecc6c43d-1415-125c-af53-31274a86f656@gmail.com","threadId":"55906","inReplyTo":"20210819200953.2105230-4-emilyshaffer@google.com","subject":"Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T00:50:27Z","receivedAt":"2021-08-20T00:50:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> +\t/* cache pointer to superproject's gitdir */\n> +\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n> +\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n\nRegarding this NEEDSWORK: you are specifying the config file that\ncovers every worktree of this submodule, which seems to make sense\nto me. The submodule shouldn't have worktrees to itself, right? It\nmight become a worktree if its superproject has a worktree.\n\nBut also: it's still correct to write to the local, non-worktree\nconfig file even if experimental.worktreeConfig is set. That\nconfig just means \"we need to read the worktree config as well\nas the local config\".\n\nOr, am I misunderstanding something?\n\n(I'm advocating for the removal of the NEEDSWORK, in case that\nwasn't clear.)\n\n> +\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(get_super_prefix_or_empty(),\n> +\t\t\t\t\t path, &sb));\n\nThanks,\n-Stolee\n\n"},{"id":"433195","messageId":"66111c98-8be3-d3e5-288f-cb5b59744cb5@gmail.com","threadId":"55906","inReplyTo":"20210819200953.2105230-5-emilyshaffer@google.com","subject":"Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T00:59:55Z","receivedAt":"2021-08-20T01:00:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> +\t\t# Cache a pointer to the superproject's gitdir. This may have\n> +\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n> +\t\t# if applicable, otherwise to local.\n> +\t\trelative_gitdir=\"$(git rev-parse --path-format=relative \\\n> +\t\t\t\t\t\t --prefix \"${sm_path}\" \\\n> +\t\t\t\t\t\t --git-dir)\"\n> +\n> +\t\tgit -C \"$sm_path\" config --worktree \\> +\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n\nOk, I see now why you care about the worktree config. The scenario you\nare covering is something like moving a submodule within a worktree and\nnot wanting to change the relative path of other copies of that submodule\nwithin other worktrees, yes?\n\nFor commands such as 'init' and 'add', we don't have the possibility of\ncolliding with other worktrees because the submodule is being created\nfor the first time, so the relative path should be safe to place in the\nnon-worktree config.\n\nI do struggle with the fact that these are inconsistent across the\ntwo commits. It makes me think that there should only be one way to\ndo it, and either the NEEDSWORK needs to be fixed now, or this line\nshouldn't include --worktree. Much of this can depend on the reason\nthe worktree config exists for a submodule. I expect you have more\ncontext than me, so could you help me understand?\n\nMoving to a different concern I am now realizing with this config:\nWhat happens if a submodule moves, and then a user runs 'git checkout'\nin the superproject to move it back? How do we make this config value\nupdate across those movements? Is there a possibility of integrating\nwith unpack_trees() to notice that a submodule has moved and hence we\nshould update this config value?\n\nThanks,\n-Stoolee\n"},{"id":"433196","messageId":"f5197594-550f-3bd8-2d22-c9e1b3e161fd@gmail.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T01:09:46Z","receivedAt":"2021-08-20T01:09:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> Since v2, mostly documentation changes and a handful of small nits from\n> Junio and Jonathan Tan. Thanks for the reviews, both.\n\nSorry to show up late with many questions (mostly because my understanding\nof submodules is not as strong as yours), but I do have some concerns that\nwe have not covered all the cases that could lead to the relative path\nneeding an update, such as a 'git checkout' across commits in the super-\nproject which moves a submodule.\n\nLeading more to my confusion is that I thought 'git submodule update'\nwas the way to move a submodule, but that does not appear to be the case.\nI used 'git mv' to move a submodule and that correctly updated the\n.gitmodules file, but did not run any 'git submodule' subcommands (based\non GIT_TRACE2_PERF=1).\n\nYou mention in an earlier cover letter that this config does not need to\nexist, but it seems to me that if it exists it needs to be correct for us\nto depend on it for future features. I'm not convinced that we can rely\non it as-written, but you might be able to convince me by pointing out\nwhy these submodule movements are safe.\n\nThanks,\n-Stolee\n"},{"id":"434699","messageId":"20210904172051.2858-1-matheus.bernardino@usp.br","threadId":"55906","inReplyTo":"20210819200953.2105230-3-emilyshaffer@google.com","subject":"Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-04T17:20:51Z","receivedAt":"2021-09-04T17:21:05Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Emily Shaffer wrote:\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d55f6262e9..d60fcd2c7d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> \t\t\t\t\t error_strategy);\n> \n> +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(absolute_path(get_git_dir()),\n> +\t\t\t\t\t path, &sb));\n\nThis will be executed when cloning a submodule with\n`git submodule add `. Do we also want to set\nsubmodule.superprojectGitdir when adding a repository that already exists in\nthe working tree as a submodule? I.e., something like:\n\ngit init super\ngit init super/sub\n[ make commits in super/sub ]\ngit -C super submodule add ./sub\n\nI don't know if this workflow is so commonly used, though... It may not be\nworth the additional work.\n\nAnother option, which I believe was suggested by Jonathan Nieder on the Review\nClub, is to change the code to absorb the gitdir when adding the local\nsubmodule. Then, the configuration would already be set by the\n`absorb_git_dir...()` function itself.\n\n> \tfree(sm_alternate);\n> \tfree(error_strategy);\n> \n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 4bc6b6c886..e407329d81 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '\n> submodurl=$(pwd -P)\n> \n> inspect() {\n> -\tdir=$1 &&\n> -\n> -\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> -\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> -\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> -\tgit -C \"$dir\" update-index --refresh &&\n> -\tgit -C \"$dir\" diff-files --exit-code &&\n> -\tgit -C \"$dir\" clean -n -d -x >untracked\n> +\tsub_dir=$1 &&\n> +\tsuper_dir=$2 &&\n> +\n> +\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> +\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n> +\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n> +\tgit -C \"$sub_dir\" update-index --refresh &&\n> +\tgit -C \"$sub_dir\" diff-files --exit-code &&\n> +\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n> +\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n> +\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n\nTo avoid the non-POSIX `-ef`, we could perhaps do something like: \n\n\tsuper_gitdir=\"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" &&\n\tcached_gitdir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n\ttest \"$cached_gitdir\" = \"$(test-tool path-utils relative_path \"$super_gitdir\" \"$PWD/$sub_dir\")\" &&\n\n(We need the \"$PWD/\" at the last command because `path.c:relative_path()`\nreturns the first argument as-is when one of the two paths given to it is\nabsolute and the other is not.)\n\nOne bonus of testing the cached path this way is that we also check that\nit is indeed being stored as a relative path :)\n"},{"id":"434700","messageId":"20210904172746.3031-1-matheus.bernardino@usp.br","threadId":"55906","inReplyTo":"20210819200953.2105230-4-emilyshaffer@google.com","subject":"Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-04T17:27:46Z","receivedAt":"2021-09-04T17:27:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Emily Shaffer wrote:\n>\n> Already during 'git submodule add' we record a pointer to the\n> superproject's gitdir. However, this doesn't help brand-new\n> submodules created with 'git init' and later absorbed with 'git\n> submodule absorbgitdir'. Let's start adding that pointer during 'git\n> submodule absorbgitdir' too.\n> \n> Signed-off-by: Emily Shaffer \n> ---\n> submodule.c | 10 ++++++++++\n> t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++-\n> 2 files changed, 18 insertions(+), 1 deletion(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 0b1d9c1dde..4b314bf09c 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n\nThis function has only one caller, `absorb_git_dir_into_superproject()`.\nPerhaps it would be better to set submodule.superprojectGitdir in the caller,\nright after the `else` block in which `relocate...()` is called. This way,\nthe config would also be set in the case of nested submodules where the inner\none already had its gitdir absorbed (which is the case handled by the code in\nthe `if` block).\n\n> \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n> \tchar *new_git_dir;\n> \tconst struct submodule *sub;\n> +\tstruct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;\n> \n> \tif (submodule_uses_worktrees(path))\n> \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n> @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n> \n> \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n> \n> +\t/* cache pointer to superproject's gitdir */\n> +\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n\ns/experimental/extensions/\n\nOn the Review Club, I mentioned we might want to save\nsubmodule.superprojectGitdir at the worktree config file. But Jonathan and Josh\ngave a better suggestion, which is to cache the superproject gitdir relative to\nthe submodule's gitdir, instead of its working tree.\n\nThis way, the worktree config wouldn't be an issue. And more importantly, this\nwould prevent `git mv` from making the cached path stale, as Stolee pointed out\nupthread.\n\n> +\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n> +\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(get_super_prefix_or_empty(),\n> +\t\t\t\t\t path, &sb));\n\n\nIn this code, `the_repository` corresponds to the superproject, right? I\nthink `get_super_prefix_or_empty()` should instead be\n`absolute_path(get_git_dir())`, like you did on the previous patch.\n\nAnd since the first argument to `relative_path()` will be an absolute path, I\nbelieve we also need to convert the second one to an absolute path. Otherwise,\n`relative_path()` would return the first argument as-is [1]. (I played around\nwith using `get_git_dir()` directly as the first argument, but it seems this\ncan sometimes already be absolute, in case of nested submodules.)\n\nIf we store the path as relative to the submodule's gitdir, it should be\nsimpler, as `real_new_git_dir` is already absolute:\n\t\n\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n\t\t\t relative_path(absolute_path(get_git_dir())\n\t\t\t\t\t real_new_git_dir, &sb));\n\n[1]: I'm not sure if this is intended or if it's a bug. I was expecting that,\nbefore comparing its two arguments, `relative_path()` would convert any\nrelative path given as argument to absolute, using the current working dir path.\nBut that's not the case.\n\n> +\tstrbuf_release(&config_path);\n> +\tstrbuf_release(&sb);\n> \tfree(old_git_dir);\n> \tfree(real_old_git_dir);\n> \tfree(real_new_git_dir);\n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index 1cfa150768..e2d78e01df 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' '\n> \tgit status >actual.1 &&\n> \tgit -C sub1 rev-parse HEAD >actual.2 &&\n> \ttest_cmp expect.1 actual.1 &&\n> -\ttest_cmp expect.2 actual.2\n> +\ttest_cmp expect.2 actual.2 &&\n> +\n> +\t# make sure the submodule cached the superproject gitdir correctly\n> +\ttest-tool path-utils real_path . >expect &&\n\nThis should be '.git' instead of '.', since the config caches the path to the\nsuperproject's gitdir. But ...\n\n> +\ttest-tool path-utils real_path \\\n> +\t\t\"$(git -C sub1 config submodule.superprojectGitDir)\" >actual &&\n\n... I think we could also avoid converting to an absolute path here, so that we\ncan test whether the setting is really caching a relative path. I.e., the test\ncould be:\n\n\tsuper_gitdir=\"$(git rev-parse --absolute-git-dir)\" &&\n\ttest-tool path-utils relative_path \"$super_gitdir\" \"$PWD/sub1\" >expect &&\n\tgit -C sub1 config submodule.superprojectGitDir >actual &&\n\ttest_cmp expect actual\n\n> +\n> +\ttest_cmp expect actual\n> '\n\nIt may also be interesting to test if the config is correctly set when\nabsorbing the gitdir of nested submodules:\n\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex e2d78e01df..c2e5e7dd1c 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -70,3 +70,8 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n \ttest_cmp expect.1 actual.1 &&\n-\ttest_cmp expect.2 actual.2\n+\ttest_cmp expect.2 actual.2 &&\n+\n+\tsub1_gitdir=\"$(git -C sub1 rev-parse --absolute-git-dir)\" &&\n+\ttest-tool path-utils relative_path \"$sub1_gitdir\" \"$PWD/sub1/nested\" >expect &&\n+\tgit -C sub1/nested config submodule.superprojectGitDir >actual &&\n+\ttest_cmp expect actual\n '\n--- >8 ---\n"},{"id":"434701","messageId":"CAHd-oW6Ko2s7uXZDbEBHpVhjNiXNcR74DriA-V=9cfOcN6bjFw@mail.gmail.com","threadId":"55906","inReplyTo":"20210819200953.2105230-1-emilyshaffer@google.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-04T17:50:30Z","receivedAt":"2021-09-04T17:50:46Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Emily\n\nOn Thu, Aug 19, 2021 at 5:10 PM Emily Shaffer wrote:\n>\n> Emily Shaffer (4):\n> t7400-submodule-basic: modernize inspect() helper\n> introduce submodule.superprojectGitDir record\n> submodule: record superproject gitdir during absorbgitdirs\n> submodule: record superproject gitdir during 'update'\n\nI left a few comments on patches 2 and 3. They are mostly about things\nwe have already discussed on the Review Club, but I got the chance to\nexperiment with the code a bit more after that, and I thought it could\nbe helpful to provide these inline suggestions. Hope it helps, and\nthanks for the series!\n\nThanks,\nMatheus\n"},{"id":"438675","messageId":"YWcquctPsrRm964k@google.com","threadId":"55906","inReplyTo":"f5197594-550f-3bd8-2d22-c9e1b3e161fd@gmail.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-13T18:51:37Z","receivedAt":"2021-10-13T18:51:46Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:\n> \n> On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> > Since v2, mostly documentation changes and a handful of small nits from\n> > Junio and Jonathan Tan. Thanks for the reviews, both.\n> \n> Sorry to show up late with many questions (mostly because my understanding\n> of submodules is not as strong as yours),\n\nWell, here I am apologizing for showing up even later with a reply ;)\n\n> but I do have some concerns that\n> we have not covered all the cases that could lead to the relative path\n> needing an update, such as a 'git checkout' across commits in the super-\n> project which moves a submodule.\n> \n> Leading more to my confusion is that I thought 'git submodule update'\n> was the way to move a submodule, but that does not appear to be the case.\n> I used 'git mv' to move a submodule and that correctly updated the\n> .gitmodules file, but did not run any 'git submodule' subcommands (based\n> on GIT_TRACE2_PERF=1).\n\nDuring a live review a few weeks ago it was pointed out, I forget by\nwho, that this whole series would make a lot more sense if it was\ntreated as the path from the submodule's gitdir to the superproject's\ngitdir. I think this would also fix your 'git mv' example here, as the\nsubmodule gitdir would not change.\n\n> \n> You mention in an earlier cover letter that this config does not need to\n> exist, but it seems to me that if it exists it needs to be correct for us\n> to depend on it for future features. I'm not convinced that we can rely\n> on it as-written, but you might be able to convince me by pointing out\n> why these submodule movements are safe.\n\nYeah, that is a good point, and I wonder how to be defensive about\nthis... Maybe it makes sense to BUG() if we don't find the\nsuperproject's gitdir from this config? Maybe it makes sense to warn\n(asking users to 'git submodule update') and erase the incorrect config\nline?\n\nBoth those approaches would require a wrapper around\n'git_config_get_string()' to cope with a failure, but it might be worth\nit.\n\nI'll try proposing such a wrapper in the reroll, unless I hear back\nsooner.\n\n - Emily\n"},{"id":"438682","messageId":"YWc1VJgWymoPado9@google.com","threadId":"55906","inReplyTo":"9fecf160-9646-7e56-478c-aa5f8defa6a9@gmail.com","subject":"Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-13T19:36:52Z","receivedAt":"2021-10-13T19:37:02Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Aug 19, 2021 at 08:38:19PM -0400, Derrick Stolee wrote:\n> \n> On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> ...\n> > +submodule.superprojectGitDir::\n> > +\tThe relative path from the submodule's worktree to its superproject's\n> > +\tgitdir. When Git is run in a repository, it usually makes no difference\n> > +\twhether this repository is standalone or a submodule, but if this\n> > +\tconfiguration variable is present, additional behavior may be possible,\n> > +\tsuch as \"git status\" printing additional information about this\n> > +\tsubmodule's status with respect to its superproject. This config should\n> > +\tonly be present in projects which are submodules, but is not guaranteed\n> > +\tto be present in every submodule, so only optional value-added behavior\n> > +\tshould be linked to it. It is set automatically during\n> > +\tsubmodule creation.\n> > ++\n> > +\tBecause of this configuration variable, it is forbidden to use the\n> > +\tsame submodule worktree shared by multiple superprojects.\n> \n> nit: this paragraph linked with the \"+\" line should have no tabbing.\n\nDone.\n\n> \n> Also, could we use the same submodule worktree for multiple superprojects\n> _before_ this configuration variable? That seems wild to me. Or, is that\n> not a new requirement?\n\nI guess it'd be possible to do something pretty evil with symlinks? I'm\nnot sure why you would want to, though.\n\nBut now that I think about it more, I'm not sure that it would work, at\nleast if we understand submodule to mean \"...and the gitdir lives in\n.git/modules/ of the superproject\".\n\nIf superA contains sub and superB contains a symlink to 'sub''s\nworktree in superA, then wouldn't superA and superB both be trying to\ncontain their own gitdirs for sub? And having multiple gitdirs for a\nworktree is an unacceptable state anyway.\n\nOr maybe the issue is more like: you have super, which contains sub, and\nyou have super-wt, which is a worktree of super; to avoid duplicating\nsub, you decided to use a symlink. So there's only one sub gitdir, and\nonly one super gitdir. It's a little awkward, but since submodule\nworktrees aren't currently supported, I can see the appeal. In this\nconfiguration, a path from submodule *worktree* to superproject gitdir,\nwhich is what v3 and earlier propose, would be broken in one\nsuperproject worktree or the other And having multiple gitdirs for a\nworktree is an unacceptable state anyway.\n\nOr maybe the issue is more like: you have super, which contains sub, and\nyou have super-wt, which is a worktree of super; to avoid duplicating\nsub, you decided to use a symlink. So there's only one sub gitdir, and\nonly one super gitdir. It's a little awkward, but since submodule\nworktrees aren't currently supported, I can see the appeal. In this\nconfiguration, a path from submodule *worktree* to superproject gitdir,\nwhich is what v3 and earlier propose, would be broken in one\nsuperproject worktree or the other. But as I'm proposing in v4, folks in\nthe review club pointed out to me that a pointer from gitdir to gitdir\nmakes more sense - and that would fix this concern, too, because sub and\nthe symlink of sub would both share a single gitdir, and that gitdir\nwould point to the single gitdir of super and super-wt.\n\nAll a long way to say: I think v4 will fix it by originating the\nrelative path from submodule gitdir, instead. And I will remove the\nextra paragraph - I think it is just adding confusion around a case that\nnobody would really want to use...\n\n> \n> Perhaps you mean something like this instead:\n> \n> \tIt is forbidden to use the same submodule worktree for multiple\n> \tsuperprojects, so this configuration variable stores the unique\n> \tsuperproject and is not multi-valued.\n> \n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index d55f6262e9..d60fcd2c7d 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> > \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> > \t\t\t\t\t error_strategy);\n> > \n> > +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> > +\t\t\t relative_path(absolute_path(get_git_dir()),\n> > +\t\t\t\t\t path, &sb));\n> > +\n> \n> I see that all new submodules will have this configuration set. But we will\n> also live in a world where some existing submodules do not have this variable\n> set. I'll look elsewhere for compatibility checks.\n\nYep, the series intended to add them piecemeal where possible, over the\ncourse of a handful of commits.\n\n> \n> > inspect() {\n> > -\tdir=$1 &&\n> > -\n> > -\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> > -\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> > -\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> > -\tgit -C \"$dir\" update-index --refresh &&\n> > -\tgit -C \"$dir\" diff-files --exit-code &&\n> > -\tgit -C \"$dir\" clean -n -d -x >untracked\n> > +\tsub_dir=$1 &&\n> > +\tsuper_dir=$2 &&\n> > +\n> > +\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> > +\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n> > +\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n> > +\tgit -C \"$sub_dir\" update-index --refresh &&\n> > +\tgit -C \"$sub_dir\" diff-files --exit-code &&\n> > +\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n> > +\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n> > +\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n> > +\tgit -C \"$sub_dir\" clean -n -d -x >untracked\n> \n> You rewrote this test in the previous patch, and now every line is changed\n> because you renamed 'dir' to 'sub_dir'. Could the previous patch use\n> 'sub_dir' from the start so this change only shows the new lines instead of\n> many edited lines?\n\nSure.\n\n> \n> > }\n> > \n> > test_expect_success 'submodule add' '\n> > @@ -138,7 +142,7 @@ test_expect_success 'submodule add' '\n> > \t) &&\n> > \n> > \trm -f heads head untracked &&\n> > -\tinspect addtest/submod &&\n> > +\tinspect addtest/submod addtest &&\n> \n> Similarly, I would not be upset to see these lines be changed just the\n> once, even if the second argument is ignored for a single commit. But\n> this nitpick is definitely less important since I could see taste\n> swaying things either way.\n\nI feel less interested in that nit; I think a mechanical \"strip the\nuseless arg\" change + a mechanical \"add an unrelated useful arg\" change\nis easier to review than doing both at once.\n\n - Emily\n"},{"id":"438683","messageId":"YWc13By8SnIw1mEV@google.com","threadId":"55906","inReplyTo":"20210904172051.2858-1-matheus.bernardino@usp.br","subject":"Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-13T19:39:08Z","receivedAt":"2021-10-13T19:39:15Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Sat, Sep 04, 2021 at 02:20:51PM -0300, Matheus Tavares wrote:\n> \n> Emily Shaffer wrote:\n> >\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index d55f6262e9..d60fcd2c7d 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> > \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> > \t\t\t\t\t error_strategy);\n> > \n> > +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> > +\t\t\t relative_path(absolute_path(get_git_dir()),\n> > +\t\t\t\t\t path, &sb));\n> \n> This will be executed when cloning a submodule with\n> `git submodule add `. Do we also want to set\n> submodule.superprojectGitdir when adding a repository that already exists in\n> the working tree as a submodule? I.e., something like:\n> \n> git init super\n> git init super/sub\n> [ make commits in super/sub ]\n> git -C super submodule add ./sub\n> \n> I don't know if this workflow is so commonly used, though... It may not be\n> worth the additional work.\n\nYeah, I think it is covered in the next patch with 'git submodule absorbgitdirs'.\n\n> \n> Another option, which I believe was suggested by Jonathan Nieder on the Review\n> Club, is to change the code to absorb the gitdir when adding the local\n> submodule. Then, the configuration would already be set by the\n> `absorb_git_dir...()` function itself.\n> \n> > \tfree(sm_alternate);\n> > \tfree(error_strategy);\n> > \n> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> > index 4bc6b6c886..e407329d81 100755\n> > --- a/t/t7400-submodule-basic.sh\n> > +++ b/t/t7400-submodule-basic.sh\n> > @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '\n> > submodurl=$(pwd -P)\n> > \n> > inspect() {\n> > -\tdir=$1 &&\n> > -\n> > -\tgit -C \"$dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> > -\t{ git -C \"$dir\" symbolic-ref HEAD || :; } >head &&\n> > -\tgit -C \"$dir\" rev-parse HEAD >head-sha1 &&\n> > -\tgit -C \"$dir\" update-index --refresh &&\n> > -\tgit -C \"$dir\" diff-files --exit-code &&\n> > -\tgit -C \"$dir\" clean -n -d -x >untracked\n> > +\tsub_dir=$1 &&\n> > +\tsuper_dir=$2 &&\n> > +\n> > +\tgit -C \"$sub_dir\" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&\n> > +\t{ git -C \"$sub_dir\" symbolic-ref HEAD || :; } >head &&\n> > +\tgit -C \"$sub_dir\" rev-parse HEAD >head-sha1 &&\n> > +\tgit -C \"$sub_dir\" update-index --refresh &&\n> > +\tgit -C \"$sub_dir\" diff-files --exit-code &&\n> > +\tcached_super_dir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n> > +\t[ \"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" \\\n> > +\t\t-ef \"$sub_dir/$cached_super_dir\" ] &&\n> \n> To avoid the non-POSIX `-ef`, we could perhaps do something like: \n> \n> \tsuper_gitdir=\"$(git -C \"$super_dir\" rev-parse --absolute-git-dir)\" &&\n> \tcached_gitdir=\"$(git -C \"$sub_dir\" config --get submodule.superprojectGitDir)\" &&\n> \ttest \"$cached_gitdir\" = \"$(test-tool path-utils relative_path \"$super_gitdir\" \"$PWD/$sub_dir\")\" &&\n> \n> (We need the \"$PWD/\" at the last command because `path.c:relative_path()`\n> returns the first argument as-is when one of the two paths given to it is\n> absolute and the other is not.)\n> \n> One bonus of testing the cached path this way is that we also check that\n> it is indeed being stored as a relative path :)\n\nYep, that is what I settled on. Thanks.\n\n - Emily\n"},{"id":"438684","messageId":"YWc2iJ7FQJYCnQ7w@google.com","threadId":"55906","inReplyTo":"ecc6c43d-1415-125c-af53-31274a86f656@gmail.com","subject":"Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-13T19:42:00Z","receivedAt":"2021-10-13T19:42:09Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Aug 19, 2021 at 08:50:27PM -0400, Derrick Stolee wrote:\n> \n> On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> > +\t/* cache pointer to superproject's gitdir */\n> > +\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n> > +\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n> \n> Regarding this NEEDSWORK: you are specifying the config file that\n> covers every worktree of this submodule, which seems to make sense\n> to me. The submodule shouldn't have worktrees to itself, right? It\n> might become a worktree if its superproject has a worktree.\n> \n> But also: it's still correct to write to the local, non-worktree\n> config file even if experimental.worktreeConfig is set. That\n> config just means \"we need to read the worktree config as well\n> as the local config\".\n> \n> Or, am I misunderstanding something?\n> \n> (I'm advocating for the removal of the NEEDSWORK, in case that\n> wasn't clear.)\n\nYeah, I think by pointing from submodule gitdir to superproject gitdir\nthis point goes away, anyways. Regardless of which worktree is in use,\nthe gitdir->gitdir pointer will still be valid. I'll delete the\nNEEDSWORK.\n\n> \n> > +\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n> > +\t\t\t relative_path(get_super_prefix_or_empty(),\n> > +\t\t\t\t\t path, &sb));\n> \n> Thanks,\n> -Stolee\n> \n"},{"id":"438766","messageId":"461f5fa8-42a4-b9a0-b223-7f97b764efd0@gmail.com","threadId":"55906","inReplyTo":"YWcquctPsrRm964k@google.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-14T17:12:11Z","receivedAt":"2021-10-14T17:12:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/13/2021 2:51 PM, Emily Shaffer wrote:\n> On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:\n>> but I do have some concerns that\n>> we have not covered all the cases that could lead to the relative path\n>> needing an update, such as a 'git checkout' across commits in the super-\n>> project which moves a submodule.\n>>\n>> Leading more to my confusion is that I thought 'git submodule update'\n>> was the way to move a submodule, but that does not appear to be the case.\n>> I used 'git mv' to move a submodule and that correctly updated the\n>> .gitmodules file, but did not run any 'git submodule' subcommands (based\n>> on GIT_TRACE2_PERF=1).\n> \n> During a live review a few weeks ago it was pointed out, I forget by\n> who, that this whole series would make a lot more sense if it was\n> treated as the path from the submodule's gitdir to the superproject's\n> gitdir. I think this would also fix your 'git mv' example here, as the\n> submodule gitdir would not change.\n\nI think that's a great way to deliver similar functionality without\nthe issues I was thinking about.\n\n>> You mention in an earlier cover letter that this config does not need to\n>> exist, but it seems to me that if it exists it needs to be correct for us\n>> to depend on it for future features. I'm not convinced that we can rely\n>> on it as-written, but you might be able to convince me by pointing out\n>> why these submodule movements are safe.\n> \n> Yeah, that is a good point, and I wonder how to be defensive about\n> this... Maybe it makes sense to BUG() if we don't find the\n> superproject's gitdir from this config? Maybe it makes sense to warn\n> (asking users to 'git submodule update') and erase the incorrect config\n> line?\n\nI think we can complain with something like a die() if the config points\nto data that doesn't make sense (not a .git directory), but a BUG() is\ntoo heavy-handed because it can just be a user modifying their config\nfile incorrectly.\n\nI'm happy to shut down a process because a user messed with config\nincorrectly. Since your proposed change allows operations like 'git mv'\nto move submodules without needing to change this config, I'm much\nhappier with the design. It becomes impossible for the config to get\nout of sync unless the user does something outside of a Git command.\n\nThanks,\n-Stolee\n"},{"id":"438776","messageId":"YWh5qvHdTyjoToYX@google.com","threadId":"55906","inReplyTo":"20210904172746.3031-1-matheus.bernardino@usp.br","subject":"Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-14T18:40:42Z","receivedAt":"2021-10-14T18:40:50Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Sat, Sep 04, 2021 at 02:27:46PM -0300, Matheus Tavares wrote:\n> \n> Emily Shaffer wrote:\n> >\n> > Already during 'git submodule add' we record a pointer to the\n> > superproject's gitdir. However, this doesn't help brand-new\n> > submodules created with 'git init' and later absorbed with 'git\n> > submodule absorbgitdir'. Let's start adding that pointer during 'git\n> > submodule absorbgitdir' too.\n> > \n> > Signed-off-by: Emily Shaffer \n> > ---\n> > submodule.c | 10 ++++++++++\n> > t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++-\n> > 2 files changed, 18 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/submodule.c b/submodule.c\n> > index 0b1d9c1dde..4b314bf09c 100644\n> > --- a/submodule.c\n> > +++ b/submodule.c\n> > @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n> \n> This function has only one caller, `absorb_git_dir_into_superproject()`.\n> Perhaps it would be better to set submodule.superprojectGitdir in the caller,\n> right after the `else` block in which `relocate...()` is called. This way,\n> the config would also be set in the case of nested submodules where the inner\n> one already had its gitdir absorbed (which is the case handled by the code in\n> the `if` block).\n\nSince relocate_single_git_dir() is the only place where the final\nsubmodule gitdir is resolved, I'll leave the code block where it is;\nneither side of the if/else in absorb_git_dir...() learns the final\n\"new\" gitdir, so I'd rather not re-derive it.\n\nAnyway, wouldn't the submodule-who-is-also-a-superproject have gone\nthrough this same codepath itself? I'll see if I can find a test to\nvalidate that.\n\n> \n> > \tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n> > \tchar *new_git_dir;\n> > \tconst struct submodule *sub;\n> > +\tstruct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT;\n> > \n> > \tif (submodule_uses_worktrees(path))\n> > \t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n> > @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path)\n> > \n> > \trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n> > \n> > +\t/* cache pointer to superproject's gitdir */\n> > +\t/* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */\n> \n> s/experimental/extensions/\n> \n> On the Review Club, I mentioned we might want to save\n> submodule.superprojectGitdir at the worktree config file. But Jonathan and Josh\n> gave a better suggestion, which is to cache the superproject gitdir relative to\n> the submodule's gitdir, instead of its working tree.\n> \n> This way, the worktree config wouldn't be an issue. And more importantly, this\n> would prevent `git mv` from making the cached path stale, as Stolee pointed out\n> upthread.\n\nYep, done. Thanks.\n\n> \n> > +\tstrbuf_addf(&config_path, \"%s/config\", real_new_git_dir);\n> > +\tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n> > +\t\t\t relative_path(get_super_prefix_or_empty(),\n> > +\t\t\t\t\t path, &sb));\n> \n> \n> In this code, `the_repository` corresponds to the superproject, right? I\n> think `get_super_prefix_or_empty()` should instead be\n> `absolute_path(get_git_dir())`, like you did on the previous patch.\n> \n> And since the first argument to `relative_path()` will be an absolute path, I\n> believe we also need to convert the second one to an absolute path. Otherwise,\n> `relative_path()` would return the first argument as-is [1]. (I played around\n> with using `get_git_dir()` directly as the first argument, but it seems this\n> can sometimes already be absolute, in case of nested submodules.)\n> \n> If we store the path as relative to the submodule's gitdir, it should be\n> simpler, as `real_new_git_dir` is already absolute:\n> \t\n> \tgit_config_set_in_file(config_path.buf, \"submodule.superprojectGitdir\",\n> \t\t\t relative_path(absolute_path(get_git_dir())\n> \t\t\t\t\t real_new_git_dir, &sb));\n> \n> [1]: I'm not sure if this is intended or if it's a bug. I was expecting that,\n> before comparing its two arguments, `relative_path()` would convert any\n> relative path given as argument to absolute, using the current working dir path.\n> But that's not the case.\n\nThanks, fixed.\n\n> \n> > +\tstrbuf_release(&config_path);\n> > +\tstrbuf_release(&sb);\n> > \tfree(old_git_dir);\n> > \tfree(real_old_git_dir);\n> > \tfree(real_new_git_dir);\n> > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> > index 1cfa150768..e2d78e01df 100755\n> > --- a/t/t7412-submodule-absorbgitdirs.sh\n> > +++ b/t/t7412-submodule-absorbgitdirs.sh\n> > @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' '\n> > \tgit status >actual.1 &&\n> > \tgit -C sub1 rev-parse HEAD >actual.2 &&\n> > \ttest_cmp expect.1 actual.1 &&\n> > -\ttest_cmp expect.2 actual.2\n> > +\ttest_cmp expect.2 actual.2 &&\n> > +\n> > +\t# make sure the submodule cached the superproject gitdir correctly\n> > +\ttest-tool path-utils real_path . >expect &&\n> \n> This should be '.git' instead of '.', since the config caches the path to the\n> superproject's gitdir. But ...\n> \n> > +\ttest-tool path-utils real_path \\\n> > +\t\t\"$(git -C sub1 config submodule.superprojectGitDir)\" >actual &&\n> \n> ... I think we could also avoid converting to an absolute path here, so that we\n> can test whether the setting is really caching a relative path. I.e., the test\n> could be:\n> \n> \tsuper_gitdir=\"$(git rev-parse --absolute-git-dir)\" &&\n> \ttest-tool path-utils relative_path \"$super_gitdir\" \"$PWD/sub1\" >expect &&\n> \tgit -C sub1 config submodule.superprojectGitDir >actual &&\n> \ttest_cmp expect actual\n\nSure.\n\n> \n> > +\n> > +\ttest_cmp expect actual\n> > '\n> \n> It may also be interesting to test if the config is correctly set when\n> absorbing the gitdir of nested submodules:\n> \n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index e2d78e01df..c2e5e7dd1c 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -70,3 +70,8 @@ test_expect_success 'absorb the git dir in a nested submodule' '\n> \ttest_cmp expect.1 actual.1 &&\n> -\ttest_cmp expect.2 actual.2\n> +\ttest_cmp expect.2 actual.2 &&\n> +\n> +\tsub1_gitdir=\"$(git -C sub1 rev-parse --absolute-git-dir)\" &&\n> +\ttest-tool path-utils relative_path \"$sub1_gitdir\" \"$PWD/sub1/nested\" >expect &&\n> +\tgit -C sub1/nested config submodule.superprojectGitDir >actual &&\n> +\ttest_cmp expect actual\n\nAh, thanks. I tried it out, even without the changes you mentioned\nabove, and this test passes. So I think as I expected, it works because\n'sub1' goes through relocate_single_git_dir() as well as 'sub1/nested'.\n\nThanks for the careful review.\n\n - Emily\n"},{"id":"438777","messageId":"YWh60MiWNKC8JseC@google.com","threadId":"55906","inReplyTo":"66111c98-8be3-d3e5-288f-cb5b59744cb5@gmail.com","subject":"Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update'","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-14T18:45:36Z","receivedAt":"2021-10-14T18:45:43Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Aug 19, 2021 at 08:59:55PM -0400, Derrick Stolee wrote:\n> \n> On 8/19/2021 4:09 PM, Emily Shaffer wrote:\n> > +\t\t# Cache a pointer to the superproject's gitdir. This may have\n> > +\t\t# changed, so rewrite it unconditionally. Writes it to worktree\n> > +\t\t# if applicable, otherwise to local.\n> > +\t\trelative_gitdir=\"$(git rev-parse --path-format=relative \\\n> > +\t\t\t\t\t\t --prefix \"${sm_path}\" \\\n> > +\t\t\t\t\t\t --git-dir)\"\n> > +\n> > +\t\tgit -C \"$sm_path\" config --worktree \\> +\t\t\tsubmodule.superprojectgitdir \"$relative_gitdir\"\n> \n> Ok, I see now why you care about the worktree config. The scenario you\n> are covering is something like moving a submodule within a worktree and\n> not wanting to change the relative path of other copies of that submodule\n> within other worktrees, yes?\n> \n> For commands such as 'init' and 'add', we don't have the possibility of\n> colliding with other worktrees because the submodule is being created\n> for the first time, so the relative path should be safe to place in the\n> non-worktree config.\n> \n> I do struggle with the fact that these are inconsistent across the\n> two commits. It makes me think that there should only be one way to\n> do it, and either the NEEDSWORK needs to be fixed now, or this line\n> shouldn't include --worktree. Much of this can depend on the reason\n> the worktree config exists for a submodule. I expect you have more\n> context than me, so could you help me understand?\n> \n> Moving to a different concern I am now realizing with this config:\n> What happens if a submodule moves, and then a user runs 'git checkout'\n> in the superproject to move it back? How do we make this config value\n> update across those movements? Is there a possibility of integrating\n> with unpack_trees() to notice that a submodule has moved and hence we\n> should update this config value?\n\nI think that switching from \"sub worktree to super gitdir\" to \"sub\ngitdir to super gitdir\" fixes this neatly - the gitdir-to-gitdir path\nwill be identical regardless of worktree, so we can set it in the main\nsubmodule config (lose the '--worktree' arg).\n\nThis also will fix the case you describe, moving around the submodule in\nthe superproject's tree from checkout to checkout, as the gitdir will\nnot move.\n\nThanks a bunch for the review and sorry it took me so long to reply. v4\nincoming.\n\n - Emily\n"},{"id":"438779","messageId":"YWh8ej9rWnxCBCKJ@google.com","threadId":"55906","inReplyTo":"461f5fa8-42a4-b9a0-b223-7f97b764efd0@gmail.com","subject":"Re: [PATCH v3 0/4] cache parent project's gitdir in submodules","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-10-14T18:52:42Z","receivedAt":"2021-10-14T18:52:51Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Oct 14, 2021 at 01:12:11PM -0400, Derrick Stolee wrote:\n> \n> On 10/13/2021 2:51 PM, Emily Shaffer wrote:\n> > On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote:\n> >> but I do have some concerns that\n> >> we have not covered all the cases that could lead to the relative path\n> >> needing an update, such as a 'git checkout' across commits in the super-\n> >> project which moves a submodule.\n> >>\n> >> Leading more to my confusion is that I thought 'git submodule update'\n> >> was the way to move a submodule, but that does not appear to be the case.\n> >> I used 'git mv' to move a submodule and that correctly updated the\n> >> .gitmodules file, but did not run any 'git submodule' subcommands (based\n> >> on GIT_TRACE2_PERF=1).\n> > \n> > During a live review a few weeks ago it was pointed out, I forget by\n> > who, that this whole series would make a lot more sense if it was\n> > treated as the path from the submodule's gitdir to the superproject's\n> > gitdir. I think this would also fix your 'git mv' example here, as the\n> > submodule gitdir would not change.\n> \n> I think that's a great way to deliver similar functionality without\n> the issues I was thinking about.\n> \n> >> You mention in an earlier cover letter that this config does not need to\n> >> exist, but it seems to me that if it exists it needs to be correct for us\n> >> to depend on it for future features. I'm not convinced that we can rely\n> >> on it as-written, but you might be able to convince me by pointing out\n> >> why these submodule movements are safe.\n> > \n> > Yeah, that is a good point, and I wonder how to be defensive about\n> > this... Maybe it makes sense to BUG() if we don't find the\n> > superproject's gitdir from this config? Maybe it makes sense to warn\n> > (asking users to 'git submodule update') and erase the incorrect config\n> > line?\n> \n> I think we can complain with something like a die() if the config points\n> to data that doesn't make sense (not a .git directory), but a BUG() is\n> too heavy-handed because it can just be a user modifying their config\n> file incorrectly.\n\nOk. Having re-thought since the other day, I think I will skip the\nwrapper for this reroll and instead propose it in a series that actually\nuses this pointer (so, the shared submodule-and-superproject config), if\nit looks like we'd want one. I expect doing that work in the context of\na caller will make the correct behavior a little more clear - in the\ncontext of this series I'm not totally sure what we'd want to do.\n\n> \n> I'm happy to shut down a process because a user messed with config\n> incorrectly. Since your proposed change allows operations like 'git mv'\n> to move submodules without needing to change this config, I'm much\n> happier with the design. It becomes impossible for the config to get\n> out of sync unless the user does something outside of a Git command.\n\nThanks for pulling the context back up here. I appreciate it. Like I\nmentioned in reply to your comment on v4, look for a reroll in the next\n30 minutes or so (assuming the CI passes ok).\n\n - Emily\n"},{"id":"438780","messageId":"87r1cnfkqx.fsf@evledraar.gmail.com","threadId":"55906","inReplyTo":"20210616004508.87186-3-emilyshaffer@google.com","subject":"Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T19:25:51Z","receivedAt":"2021-10-14T19:51:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jun 15 2021, Emily Shaffer wrote:\n\n> Teach submodules a reference to their superproject's gitdir. This allows\n> us to A) know that we're running from a submodule, and B) have a\n> shortcut to the superproject's vitals, for example, configs.\n>\n> By using a relative path instead of an absolute path, we can move the\n> superproject directory around on the filesystem without breaking the\n> submodule's cache.\n>\n> Since this cached value is only introduced during new submodule creation\n> via `git submodule add`, though, there is more work to do to allow the\n> cache to be created at other times.\n>\n> Signed-off-by: Emily Shaffer \n> ---\n> Documentation/config/submodule.txt | 12 +++++++++\n> builtin/submodule--helper.c | 4 +++\n> t/t7400-submodule-basic.sh | 40 ++++++++++++++++--------------\n> 3 files changed, 38 insertions(+), 18 deletions(-)\n>\n> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt\n> index d7a63c8c12..7c459cc19e 100644\n> --- a/Documentation/config/submodule.txt\n> +++ b/Documentation/config/submodule.txt\n> @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy::\n> \t`ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore`\n> \tor `info`, and if there is an error with the computed alternate, the\n> \tclone proceeds as if no alternate was specified.\n> +\n> +submodule.superprojectGitDir::\n> +\tThe relative path from the submodule's worktree to the superproject's\n> +\tgitdir. This config should only be present in projects which are\n> +\tsubmodules, but is not guaranteed to be present in every submodule. It\n> +\tis set automatically during submodule creation.\n> ++\n> +\tIn situations where more than one superproject references the same\n> +\tsubmodule worktree, the value of this config and the behavior of\n> +\toperations which use it are undefined. To reference a single project\n> +\tfrom multiple superprojects, it is better to create a worktree of the\n> +\tsubmodule for each superproject.\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d55f6262e9..d60fcd2c7d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n> \t\t\t\t\t error_strategy);\n> \n> +\tgit_config_set_in_file(p, \"submodule.superprojectGitdir\",\n> +\t\t\t relative_path(absolute_path(get_git_dir()),\n> +\t\t\t\t\t path, &sb));\n> +\n> \tfree(sm_alternate);\n> \tfree(error_strategy);\n> \n\nAm I correct that what this series really does is avoid needing to:\n\n1. Run the equivalent of $(git rev-parse --absolute-git-dir), let's call\n the result of that $X.\n2. Feed that to the equvialent of $(git -C $X/../ rev-parse\n --absolute-git-dir)\n3. Check if the relationship between the two is really that of a\n submodule, i.e. running \"git submodule status\", check if $X contains\n \".git/modules/\" etc.\n\nI see an earlier iteration of this series had such a shell-out, and that\nthis is the \"cache\":\nhttps://lore.kernel.org/git/20210423001539.4059524-5-emilyshaffer@google.com/;\nand your v1 cover letter seems to support the above summary:\nhttps://lore.kernel.org/git/20210611225428.1208973-1-emilyshaffer@google.com/\n\nI think it's fine to fine to add such a cache in principle if it's needed.\n\nBut I'm a bit iffy on a series that's structured in a way as to not\nstart by asserting that we should have given semantics without the\ncache, and then adds the cache later as an optional optimization.\n\nParticularly as the whole submodule business is moving to C, so isn't\nthis whole caching business something we can avoid doing, and instead\njust call a C function? The optimization part of this was not calling\n\"git rev-parse\" on every submodule invocation wasn't it, not avoiding a\nfew syscalls that deal with the FS.\n\nYour initial RFC had modifications to git-submodule.sh, in the interim\nit seems that's been moved sufficiently to C that we're modifying just\nthe submodule.c here.\n\nI'm not very familiar with setup_git_directory_gently_1(),\ndiscover_git_directory() etc, but wherever you are in a git worktree\nwe'll chdir() to the top-level when running built-ins.\n\nSo that gives us step #1 of the above. And #2 would be adding \"/../\" to\nthe end of that path and running those functions again? Perhaps with a\n#3 for \"is there a submodule relationship?\".\n\nEven if we still have some bits in shellscript etc, couldn't we then\nsetenv() that in some GIT_* variable, e.g. GIT_SUPERPROJECT_DIR?\n\nOr is the problem really that this isn't a cache, because we can't say\nwith absolute certainty that there is such a gitdir/submodule\nrelationship, except at the time of running \"git submodule\" in the\nformer for some reason?\n"}]}