* [RFC PATCH 0/4] cache parent project's gitdir in submodules @ 2021-06-11 22:54 Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer ` (6 more replies) 0 siblings, 7 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-11 22:54 UTC (permalink / raw) To: git Cc: Emily Shaffer, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar It's necessary for a superproject to know which submodules it contains. However, historically submodules do not know they are anything but a normal single-repo Git project (or a superproject, in nested-submodules cases). This decision does help prevent us from having to support divergent behaviors in submodule projects vs. superprojects, which makes sure Git is (somewhat) less confusing for the reader, and helps simplify our code. One could imagine, though, some conveniences we could gain from submodules learning added behavior (though not necessarily *different* behavior) to provide more context about the state of the project as a whole, and to make large submodule-based projects easier to work with. One example is a series[1] I sent some time ago, adding a config to be shared between the superproject and all its submodules. The RFC[2] I sent around the same time mentions a few other examples, such as "git status" run in the submodule noticing whether the superproject has a commit referencing the submodule's current HEAD. It's expensive and non-definitive to try and guess whether or not the current repo is a submodule. submodule.c:get_superproject_working_tree() does so by essentially running 'git -C .. ls-files -- <own-path>', invoking an additional process. get_superproject_working_tree() is not called often, so that's mostly fine. However, [1] attempted to include an additional config located in the superproject's gitdir by running 'git -C .. rev-parse --git-dir' during startup - a little expensive in the best case, because it's an extra process, but extremely expensive in the case when the current repo is *not* a submodule, because we hunt all the way up the filesystem looking for a '.git'. Adding that cost to every startup is infeasible. To that end, in this series I propose caching a path to the superproject's gitdir - by having the superproject write that relative path to the submodule's config on creation or update. The goal here is *not* to say "If I am a submodule, I must have submodule.superprojectGitDir set" - but instead to say "If I have submodule.superprojectGitDir set, then I must be a submodule." That is, I expect we will find edge cases where a submodule was introduced in some interesting way that bypassed any of the patches below, and therefore doesn't have the superproject's gitdir cached. The combination of these two rules: - Anything relying on submodule.superprojectGitDir must be nice to have, but not essential, because - It's possible for a submodule to be valid without having submodule.superprojectGitDir set makes me feel more comfortable with the idea of submodules learning additional behavior based on this config. I feel pretty unconfident in our ability to ensure that *every* submodule has this config set. The series covers a few paths for introducing that config, which I'm hoping covers most cases. - "git submodule update" (which seems to be part of the "git submodule init" flow) - "git submodule absorbgitdir" to convert a "git init"'d repo into a submodule Notably, we can only really set this config when 'the_repository' is the superproject - that appears to be the only time when we know the gitdirs of both the superproject and the submodule. I'm expecting folks may have a lot to say about this, so I look forward to discussion :) - Emily 1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com 2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com Emily Shaffer (4): t7400-submodule-basic: modernize inspect() helper introduce submodule.superprojectGitDir cache submodule: cache superproject gitdir during absorbgitdirs submodule: cache superproject gitdir during 'update' builtin/submodule--helper.c | 4 +++ git-submodule.sh | 9 ++++++ submodule.c | 10 ++++++ t/t7400-submodule-basic.sh | 49 ++++++++++++++---------------- t/t7406-submodule-update.sh | 10 ++++++ t/t7412-submodule-absorbgitdirs.sh | 1 + 6 files changed, 57 insertions(+), 26 deletions(-) -- 2.32.0.272.g935e593368-goog ^ permalink raw reply [flat|nested] 51+ messages in thread
* [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer @ 2021-06-11 22:54 ` Emily Shaffer 2021-06-14 4:52 ` Junio C Hamano 2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer ` (5 subsequent siblings) 6 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-11 22:54 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Since the inspect() helper in the submodule-basic test suite was written, 'git -C <dir>' was added. By using -C, we no longer need a reference to the base directory for the test. This simplifies callsites, and will make the addition of other arguments in later patches more readable. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t7400-submodule-basic.sh | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index a924fdb7a6..f5dc051a6e 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' ' # generates, which will expand symbolic links. submodurl=$(pwd -P) -listbranches() { - git for-each-ref --format='%(refname)' 'refs/heads/*' -} - inspect() { dir=$1 && - dotdot="${2:-..}" && - ( - cd "$dir" && - listbranches >"$dotdot/heads" && - { git symbolic-ref HEAD || :; } >"$dotdot/head" && - git rev-parse HEAD >"$dotdot/head-sha1" && - git update-index --refresh && - git diff-files --exit-code && - git clean -n -d -x >"$dotdot/untracked" - ) + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$dir" symbolic-ref HEAD || :; } >head && + git -C "$dir" rev-parse HEAD >head-sha1 && + git -C "$dir" update-index --refresh && + git -C "$dir" diff-files --exit-code && + git -C "$dir" clean -n -d -x >untracked } + test_expect_success 'submodule add' ' echo "refs/heads/main" >expect && @@ -146,7 +139,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod ../.. && + inspect addtest/submod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch ../.. && + inspect addtest/submod-branch && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz ../../.. && + inspect addtest/dotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz ../../.. && + inspect addtest/dotslashdotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz ../../.. && + inspect addtest/slashslashsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod ../.. && + inspect addtest/realsubmod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 ../.. && + inspect addtest/realsubmod2 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 ../.. && + inspect addtest/realsubmod3 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper 2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer @ 2021-06-14 4:52 ` Junio C Hamano 0 siblings, 0 replies; 51+ messages in thread From: Junio C Hamano @ 2021-06-14 4:52 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> writes: > Since the inspect() helper in the submodule-basic test suite was > written, 'git -C <dir>' was added. By using -C, we no longer need a > reference to the base directory for the test. This simplifies callsites, > and will make the addition of other arguments in later patches more > readable. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > t/t7400-submodule-basic.sh | 37 +++++++++++++++---------------------- > 1 file changed, 15 insertions(+), 22 deletions(-) > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index a924fdb7a6..f5dc051a6e 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' ' > # generates, which will expand symbolic links. > submodurl=$(pwd -P) > > -listbranches() { > - git for-each-ref --format='%(refname)' 'refs/heads/*' > -} > - > inspect() { > dir=$1 && > - dotdot="${2:-..}" && > > - ( > - cd "$dir" && > - listbranches >"$dotdot/heads" && > - { git symbolic-ref HEAD || :; } >"$dotdot/head" && > - git rev-parse HEAD >"$dotdot/head-sha1" && > - git update-index --refresh && > - git diff-files --exit-code && > - git clean -n -d -x >"$dotdot/untracked" > - ) > + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > + { git -C "$dir" symbolic-ref HEAD || :; } >head && > + git -C "$dir" rev-parse HEAD >head-sha1 && > + git -C "$dir" update-index --refresh && > + git -C "$dir" diff-files --exit-code && > + git -C "$dir" clean -n -d -x >untracked > } Quite straight-forward. > - inspect addtest/submod ../.. && > + inspect addtest/submod && And specifically the losing ../.. is pleasing to the eye, especially because the old "dotdot" thing was mechanically computable from "dir". Nicely done. ^ permalink raw reply [flat|nested] 51+ messages in thread
* [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer @ 2021-06-11 22:54 ` Emily Shaffer 2021-06-14 5:09 ` Junio C Hamano 2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer ` (4 subsequent siblings) 6 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-11 22:54 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Teach submodules a reference to their superproject's gitdir. This allows us to A) know that we're running from a submodule, and B) have a shortcut to the superproject's vitals, for example, configs. By using a relative path instead of an absolute path, we can move the superproject directory around on the filesystem without breaking the submodule's cache. Since this cached value is only introduced during new submodule creation via `git submodule add`, though, there is more work to do to allow the cache to be created at other times. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- builtin/submodule--helper.c | 4 ++++ t/t7400-submodule-basic.sh | 40 ++++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 9d505a6329..3d6fd54c9b 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1909,6 +1909,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) git_config_set_in_file(p, "submodule.alternateErrorStrategy", error_strategy); + git_config_set_in_file(p, "submodule.superprojectGitdir", + relative_path(absolute_path(get_git_dir()), + path, &sb)); + free(sm_alternate); free(error_strategy); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index f5dc051a6e..e45f42588f 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' ' submodurl=$(pwd -P) inspect() { - dir=$1 && - - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && - { git -C "$dir" symbolic-ref HEAD || :; } >head && - git -C "$dir" rev-parse HEAD >head-sha1 && - git -C "$dir" update-index --refresh && - git -C "$dir" diff-files --exit-code && - git -C "$dir" clean -n -d -x >untracked + sub_dir=$1 && + super_dir=$2 && + + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && + git -C "$sub_dir" rev-parse HEAD >head-sha1 && + git -C "$sub_dir" update-index --refresh && + git -C "$sub_dir" diff-files --exit-code && + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ + -ef "$sub_dir/$cached_super_dir" ] && + git -C "$sub_dir" clean -n -d -x >untracked } @@ -139,7 +143,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod && + inspect addtest/submod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch && + inspect addtest/submod-branch addtest && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz && + inspect addtest/dotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz && + inspect addtest/dotslashdotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz && + inspect addtest/slashslashsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod && + inspect addtest/realsubmod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 && + inspect addtest/realsubmod2 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 && + inspect addtest/realsubmod3 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' ' git submodule update -q >update.out && test_must_be_empty update.out && - inspect init && + inspect init . && test_cmp expect head-sha1 ' @@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' ' echo "$rev1" >expect && git submodule update init && - inspect init && + inspect init . && test_cmp expect head-sha1 ' -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache 2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer @ 2021-06-14 5:09 ` Junio C Hamano 2021-06-15 22:00 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Junio C Hamano @ 2021-06-14 5:09 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> writes: > Teach submodules a reference to their superproject's gitdir. This allows > us to A) know that we're running from a submodule, and B) have a > shortcut to the superproject's vitals, for example, configs. > > By using a relative path instead of an absolute path, we can move the > superproject directory around on the filesystem without breaking the > submodule's cache. As the function this new thing is added assumes the modern layout of having submodule "repository" in .git/modules/* of the repository of the superproject, it is rather easy to move the whole thing together, so recording it as relative path is all the more important. Can a submodule repository be bound to two or more superproject at the same time? "We assume no, and we will forbid such a layout, and that is why we can afford to make submodules aware of their superprojects" is a totally acceptable answer, but it would make it easier to follow the reasoning behind a design change like this series does if such an assumption is recorded somewhere. > + git_config_set_in_file(p, "submodule.superprojectGitdir", > + relative_path(absolute_path(get_git_dir()), > + path, &sb)); > + OK, so even when the superproject is used as a submodule of somebody else, we could get to the top of its working tree, because (1) the submodule we are currently working in can find out where the gitdir of the superproject is, and (2) in that gitdir, which is very likely different from the ".git/" subdirectory of the working tree of the superproject (instead, it would be a directory in ".git/modules/" of its superproject), we could find the core.worktree configuration to reach the working tree of the superproject. OK, makes sense. Thanks. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache 2021-06-14 5:09 ` Junio C Hamano @ 2021-06-15 22:00 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-15 22:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Jun 14, 2021 at 02:09:15PM +0900, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > Teach submodules a reference to their superproject's gitdir. This allows > > us to A) know that we're running from a submodule, and B) have a > > shortcut to the superproject's vitals, for example, configs. > > > > By using a relative path instead of an absolute path, we can move the > > superproject directory around on the filesystem without breaking the > > submodule's cache. > > As the function this new thing is added assumes the modern layout of > having submodule "repository" in .git/modules/* of the repository of > the superproject, it is rather easy to move the whole thing together, > so recording it as relative path is all the more important. > > Can a submodule repository be bound to two or more superproject at > the same time? "We assume no, and we will forbid such a layout, and > that is why we can afford to make submodules aware of their > superprojects" is a totally acceptable answer, but it would make it > easier to follow the reasoning behind a design change like this > series does if such an assumption is recorded somewhere. Thanks for pointing out the use case - I can see this happening in some case like adding a source code dependency from a few projects at once, so it doesn't sound too far-fetched. But no, I don't think this method of caching can support it; I think forbidding it and saying "if you want this to work, use a worktree" is pretty reasonable. Will find a nice place to write it down. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer @ 2021-06-11 22:54 ` Emily Shaffer 2021-06-14 6:18 ` Junio C Hamano 2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer ` (3 subsequent siblings) 6 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-11 22:54 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Already during 'git submodule add' we cache a pointer to the superproject's gitdir. However, this doesn't help brand-new submodules created with 'git init' and later absorbed with 'git submodule absorbgitdir'. Let's start adding that pointer during 'git submodule absorbgitdir' too. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- submodule.c | 10 ++++++++++ t/t7412-submodule-absorbgitdirs.sh | 1 + 2 files changed, 11 insertions(+) diff --git a/submodule.c b/submodule.c index 9767ba9893..09dfc4ee38 100644 --- a/submodule.c +++ b/submodule.c @@ -2064,6 +2064,7 @@ static void relocate_single_git_dir_into_superproject(const char *path) char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; char *new_git_dir; const struct submodule *sub; + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; if (submodule_uses_worktrees(path)) die(_("relocate_gitdir for submodule '%s' with " @@ -2095,6 +2096,15 @@ static void relocate_single_git_dir_into_superproject(const char *path) relocate_gitdir(path, real_old_git_dir, real_new_git_dir); + /* cache pointer to superproject's gitdir */ + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ + strbuf_addf(&config_path, "%s/config", real_new_git_dir); + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", + relative_path(get_super_prefix_or_empty(), + path, &sb)); + + strbuf_release(&config_path); + strbuf_release(&sb); free(old_git_dir); free(real_old_git_dir); free(real_new_git_dir); diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 1cfa150768..70fc282937 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -29,6 +29,7 @@ test_expect_success 'absorb the git dir' ' test -d .git/modules/sub1 && git status >actual.1 && git -C sub1 rev-parse HEAD >actual.2 && + test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" && test_cmp expect.1 actual.1 && test_cmp expect.2 actual.2 ' -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs 2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer @ 2021-06-14 6:18 ` Junio C Hamano 0 siblings, 0 replies; 51+ messages in thread From: Junio C Hamano @ 2021-06-14 6:18 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> writes: > + test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" && Unfortunately "test f1 -ef f2", "test f1 -nt f2", etc. are not part of POSIX. When in doubt, check https://pubs.opengroup.org/onlinepubs/9699919799/utilities/toc.html Thanks. ^ permalink raw reply [flat|nested] 51+ messages in thread
* [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (2 preceding siblings ...) 2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer @ 2021-06-11 22:54 ` Emily Shaffer 2021-06-14 6:22 ` Junio C Hamano 2021-06-12 20:12 ` [RFC PATCH 0/4] cache parent project's gitdir in submodules Jacob Keller ` (2 subsequent siblings) 6 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-11 22:54 UTC (permalink / raw) To: git; +Cc: Emily Shaffer A cached path to the superproject's gitdir might be added during 'git submodule add', but in some cases - like submodules which were created before 'git submodule add' learned to cache that info - it might be useful to update the cache. Let's do it during 'git submodule update', when we already have a handle to the superproject while calling operations on the submodules. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- git-submodule.sh | 9 +++++++++ t/t7406-submodule-update.sh | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index eb90f18229..ddda751cfa 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -648,6 +648,15 @@ cmd_update() fi fi + # Cache a pointer to the superproject's gitdir. This may have + # changed, so rewrite it unconditionally. Writes it to worktree + # if applicable, otherwise to local. + + sp_gitdir="$(git rev-parse --absolute-git-dir)" + relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")" + git -C "$sm_path" config --worktree \ + submodule.superprojectgitdir "$relative_gitdir" + if test -n "$recursive" then ( diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index ff3ba5422e..96023cbb6a 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1037,4 +1037,14 @@ test_expect_success 'submodule update --quiet passes quietness to merge/rebase' ) ' +test_expect_success 'submodule update adds superproject gitdir to older repos' ' + (cd super && + git -C submodule config --unset submodule.superprojectGitdir && + git submodule update && + echo "../.git" >expect && + git -C submodule config submodule.superprojectGitdir >actual && + test_cmp expect actual + ) +' + test_done -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' 2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer @ 2021-06-14 6:22 ` Junio C Hamano 2021-06-15 21:27 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Junio C Hamano @ 2021-06-14 6:22 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> writes: > A cached path to the superproject's gitdir might be added during 'git > submodule add', but in some cases - like submodules which were created > before 'git submodule add' learned to cache that info - it might be > useful to update the cache. Let's do it during 'git submodule update', > when we already have a handle to the superproject while calling > operations on the submodules. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > git-submodule.sh | 9 +++++++++ > t/t7406-submodule-update.sh | 10 ++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/git-submodule.sh b/git-submodule.sh > index eb90f18229..ddda751cfa 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -648,6 +648,15 @@ cmd_update() > fi > fi > > + # Cache a pointer to the superproject's gitdir. This may have > + # changed, so rewrite it unconditionally. Writes it to worktree > + # if applicable, otherwise to local. > + > + sp_gitdir="$(git rev-parse --absolute-git-dir)" > + relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")" realpath may not exist on the target system. Discussions on the patch [*1*] may be of interest. It might be a good idea to push to your github repository to trigger CI on macOS (I am guessing that you do not test locally on macs from the two issues we saw in this series). Thanks. [Reference] *1* https://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/ ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' 2021-06-14 6:22 ` Junio C Hamano @ 2021-06-15 21:27 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-15 21:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Jun 14, 2021 at 03:22:29PM +0900, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > A cached path to the superproject's gitdir might be added during 'git > > submodule add', but in some cases - like submodules which were created > > before 'git submodule add' learned to cache that info - it might be > > useful to update the cache. Let's do it during 'git submodule update', > > when we already have a handle to the superproject while calling > > operations on the submodules. > > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > git-submodule.sh | 9 +++++++++ > > t/t7406-submodule-update.sh | 10 ++++++++++ > > 2 files changed, 19 insertions(+) > > > > diff --git a/git-submodule.sh b/git-submodule.sh > > index eb90f18229..ddda751cfa 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -648,6 +648,15 @@ cmd_update() > > fi > > fi > > > > + # Cache a pointer to the superproject's gitdir. This may have > > + # changed, so rewrite it unconditionally. Writes it to worktree > > + # if applicable, otherwise to local. > > + > > + sp_gitdir="$(git rev-parse --absolute-git-dir)" > > + relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")" > > realpath may not exist on the target system. Discussions on the > patch [*1*] may be of interest. > > It might be a good idea to push to your github repository to trigger > CI on macOS (I am guessing that you do not test locally on macs from > the two issues we saw in this series). I typically do, sorry for forgetting to do so this time. > > Thanks. > > > [Reference] > > *1* > https://lore.kernel.org/git/20201206225349.3392790-3-sandals@crustytoothpaste.net/ ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (3 preceding siblings ...) 2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer @ 2021-06-12 20:12 ` Jacob Keller 2021-06-14 7:26 ` Junio C Hamano 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer 6 siblings, 0 replies; 51+ messages in thread From: Jacob Keller @ 2021-06-12 20:12 UTC (permalink / raw) To: Emily Shaffer Cc: Git mailing list, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Atharva Raykar On Fri, Jun 11, 2021 at 3:54 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > It's necessary for a superproject to know which submodules it contains. > However, historically submodules do not know they are anything but a > normal single-repo Git project (or a superproject, in nested-submodules > cases). This decision does help prevent us from having to support > divergent behaviors in submodule projects vs. superprojects, which makes > sure Git is (somewhat) less confusing for the reader, and helps simplify > our code. > > One could imagine, though, some conveniences we could gain from > submodules learning added behavior (though not necessarily *different* > behavior) to provide more context about the state of the project as a > whole, and to make large submodule-based projects easier to work with. > One example is a series[1] I sent some time ago, adding a config to be > shared between the superproject and all its submodules. The RFC[2] I > sent around the same time mentions a few other examples, such as "git > status" run in the submodule noticing whether the superproject has a > commit referencing the submodule's current HEAD. > > It's expensive and non-definitive to try and guess whether or not the > current repo is a submodule. submodule.c:get_superproject_working_tree() > does so by essentially running 'git -C .. ls-files -- <own-path>', > invoking an additional process. get_superproject_working_tree() is not > called often, so that's mostly fine. However, [1] attempted to include > an additional config located in the superproject's gitdir by running > 'git -C .. rev-parse --git-dir' during startup - a little expensive in > the best case, because it's an extra process, but extremely expensive in > the case when the current repo is *not* a submodule, because we hunt all > the way up the filesystem looking for a '.git'. Adding that cost to > every startup is infeasible. > It also adds cost for no benefit to the normal case where it's not a submodule. The cost added for non-submodules ought to be as near-zero as possible. > To that end, in this series I propose caching a path to the > superproject's gitdir - by having the superproject write that relative > path to the submodule's config on creation or update. The goal here is > *not* to say "If I am a submodule, I must have > submodule.superprojectGitDir set" - but instead to say "If I have > submodule.superprojectGitDir set, then I must be a submodule." That is, > I expect we will find edge cases where a submodule was introduced in > some interesting way that bypassed any of the patches below, and > therefore doesn't have the superproject's gitdir cached. > > The combination of these two rules: > - Anything relying on submodule.superprojectGitDir must be nice to > have, but not essential, because > - It's possible for a submodule to be valid without having > submodule.superprojectGitDir set > makes me feel more comfortable with the idea of submodules learning > additional behavior based on this config. I feel pretty unconfident in > our ability to ensure that *every* submodule has this config set. > I think this is a good direction. I do think having some information about being a submodule could be very useful for tools such as git status, and making it more of a "if we know for sure, we get some small benefit, but if we don't know then it's no harm" is a good direction. > The series covers a few paths for introducing that config, which I'm > hoping covers most cases. > - "git submodule update" (which seems to be part of the "git submodule > init" flow) > - "git submodule absorbgitdir" to convert a "git init"'d repo into a > submodule > > Notably, we can only really set this config when 'the_repository' is the > superproject - that appears to be the only time when we know the gitdirs > of both the superproject and the submodule. We could also presumably add a new command for setting this? > > I'm expecting folks may have a lot to say about this, so I look forward > to discussion :) > > - Emily > > 1: https://lore.kernel.org/git/20210423001539.4059524-1-emilyshaffer@google.com > 2: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com > > Emily Shaffer (4): > t7400-submodule-basic: modernize inspect() helper > introduce submodule.superprojectGitDir cache > submodule: cache superproject gitdir during absorbgitdirs > submodule: cache superproject gitdir during 'update' > > builtin/submodule--helper.c | 4 +++ > git-submodule.sh | 9 ++++++ > submodule.c | 10 ++++++ > t/t7400-submodule-basic.sh | 49 ++++++++++++++---------------- > t/t7406-submodule-update.sh | 10 ++++++ > t/t7412-submodule-absorbgitdirs.sh | 1 + > 6 files changed, 57 insertions(+), 26 deletions(-) > > -- > 2.32.0.272.g935e593368-goog > ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (4 preceding siblings ...) 2021-06-12 20:12 ` [RFC PATCH 0/4] cache parent project's gitdir in submodules Jacob Keller @ 2021-06-14 7:26 ` Junio C Hamano 2021-06-15 21:18 ` Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer 6 siblings, 1 reply; 51+ messages in thread From: Junio C Hamano @ 2021-06-14 7:26 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, ZheNing Hu Emily Shaffer <emilyshaffer@google.com> writes: > It's necessary for a superproject to know which submodules it contains. > However, historically submodules do not know they are anything but a > normal single-repo Git project (or a superproject, in nested-submodules > cases). This decision does help prevent us from having to support > divergent behaviors in submodule projects vs. superprojects, which makes > sure Git is (somewhat) less confusing for the reader, and helps simplify > our code. https://github.com/git/git/actions/runs/934803365 is the CI run of 'seen' that passes. With this and another topic merged to 'seen', as can be seen in https://github.com/git/git/actions/runs/934680687, a handful of tests fail. Thanks. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC PATCH 0/4] cache parent project's gitdir in submodules 2021-06-14 7:26 ` Junio C Hamano @ 2021-06-15 21:18 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-15 21:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, ZheNing Hu On Mon, Jun 14, 2021 at 04:26:17PM +0900, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > It's necessary for a superproject to know which submodules it contains. > > However, historically submodules do not know they are anything but a > > normal single-repo Git project (or a superproject, in nested-submodules > > cases). This decision does help prevent us from having to support > > divergent behaviors in submodule projects vs. superprojects, which makes > > sure Git is (somewhat) less confusing for the reader, and helps simplify > > our code. > > https://github.com/git/git/actions/runs/934803365 is the CI run of > 'seen' that passes. With this and another topic merged to 'seen', > as can be seen in https://github.com/git/git/actions/runs/934680687, > a handful of tests fail. Thanks, I will investigate. ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 0/4] cache parent project's gitdir in submodules 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (5 preceding siblings ...) 2021-06-14 7:26 ` Junio C Hamano @ 2021-06-16 0:45 ` Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer ` (4 more replies) 6 siblings, 5 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-16 0:45 UTC (permalink / raw) To: git Cc: Emily Shaffer, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar The reception for this series seemed pretty good in v1, so I'm dropping the RFC. Tested: https://github.com/nasamuffin/git/actions/runs/941100646 Sinec v1, mostly platform-friendliness fixes. Also added documentation for the new config option - wordsmithing help is always welcome. - Emily Emily Shaffer (4): t7400-submodule-basic: modernize inspect() helper introduce submodule.superprojectGitDir cache submodule: cache superproject gitdir during absorbgitdirs submodule: cache superproject gitdir during 'update' Documentation/config/submodule.txt | 12 ++++++++ builtin/submodule--helper.c | 4 +++ git-submodule.sh | 10 ++++++ submodule.c | 10 ++++++ t/t7400-submodule-basic.sh | 49 ++++++++++++++---------------- t/t7406-submodule-update.sh | 10 ++++++ t/t7412-submodule-absorbgitdirs.sh | 9 +++++- 7 files changed, 77 insertions(+), 27 deletions(-) Range-diff against v1: 1: d6284438fb = 1: a6718eea80 t7400-submodule-basic: modernize inspect() helper 2: 56470e2eab ! 2: 4cebe7bcb5 introduce submodule.superprojectGitDir cache @@ Commit message Signed-off-by: Emily Shaffer <emilyshaffer@google.com> + ## Documentation/config/submodule.txt ## +@@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy:: + `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` + or `info`, and if there is an error with the computed alternate, the + clone proceeds as if no alternate was specified. ++ ++submodule.superprojectGitDir:: ++ The relative path from the submodule's worktree to the superproject's ++ gitdir. This config should only be present in projects which are ++ submodules, but is not guaranteed to be present in every submodule. It ++ is set automatically during submodule creation. +++ ++ In situations where more than one superproject references the same ++ submodule worktree, the value of this config and the behavior of ++ operations which use it are undefined. To reference a single project ++ from multiple superprojects, it is better to create a worktree of the ++ submodule for each superproject. + ## builtin/submodule--helper.c ## @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) git_config_set_in_file(p, "submodule.alternateErrorStrategy", 3: 42f954f523 ! 3: df97a9c2bb submodule: cache superproject gitdir during absorbgitdirs @@ submodule.c: static void relocate_single_git_dir_into_superproject(const char *p ## t/t7412-submodule-absorbgitdirs.sh ## @@ t/t7412-submodule-absorbgitdirs.sh: test_expect_success 'absorb the git dir' ' - test -d .git/modules/sub1 && git status >actual.1 && git -C sub1 rev-parse HEAD >actual.2 && -+ test . -ef "$(git -C sub1 config submodule.superprojectGitDir)" && test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 +- test_cmp expect.2 actual.2 ++ test_cmp expect.2 actual.2 && ++ ++ # make sure the submodule cached the superproject gitdir correctly ++ test-tool path-utils real_path . >expect && ++ test-tool path-utils real_path \ ++ "$(git -C sub1 config submodule.superprojectGitDir)" >actual && ++ ++ test_cmp expect actual ' + + test_expect_success 'absorbing does not fail for deinitialized submodules' ' 4: 4f55ab42c7 ! 4: a3f3be58ad submodule: cache superproject gitdir during 'update' @@ git-submodule.sh: cmd_update() + # Cache a pointer to the superproject's gitdir. This may have + # changed, so rewrite it unconditionally. Writes it to worktree + # if applicable, otherwise to local. ++ relative_gitdir="$(git rev-parse --path-format=relative \ ++ --prefix "${sm_path}" \ ++ --git-dir)" + -+ sp_gitdir="$(git rev-parse --absolute-git-dir)" -+ relative_gitdir="$(realpath --relative-to "$sm_path" "$sp_gitdir")" + git -C "$sm_path" config --worktree \ + submodule.superprojectgitdir "$relative_gitdir" + @@ git-submodule.sh: cmd_update() ( ## t/t7406-submodule-update.sh ## -@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to merge/rebase' +@@ t/t7406-submodule-update.sh: test_expect_success 'submodule update --quiet passes quietness to fetch with a s ) ' -- 2.32.0.272.g935e593368-goog ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer @ 2021-06-16 0:45 ` Emily Shaffer 2021-07-27 17:12 ` Jonathan Tan 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer ` (3 subsequent siblings) 4 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-16 0:45 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Since the inspect() helper in the submodule-basic test suite was written, 'git -C <dir>' was added. By using -C, we no longer need a reference to the base directory for the test. This simplifies callsites, and will make the addition of other arguments in later patches more readable. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t7400-submodule-basic.sh | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index a924fdb7a6..f5dc051a6e 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' ' # generates, which will expand symbolic links. submodurl=$(pwd -P) -listbranches() { - git for-each-ref --format='%(refname)' 'refs/heads/*' -} - inspect() { dir=$1 && - dotdot="${2:-..}" && - ( - cd "$dir" && - listbranches >"$dotdot/heads" && - { git symbolic-ref HEAD || :; } >"$dotdot/head" && - git rev-parse HEAD >"$dotdot/head-sha1" && - git update-index --refresh && - git diff-files --exit-code && - git clean -n -d -x >"$dotdot/untracked" - ) + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$dir" symbolic-ref HEAD || :; } >head && + git -C "$dir" rev-parse HEAD >head-sha1 && + git -C "$dir" update-index --refresh && + git -C "$dir" diff-files --exit-code && + git -C "$dir" clean -n -d -x >untracked } + test_expect_success 'submodule add' ' echo "refs/heads/main" >expect && @@ -146,7 +139,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod ../.. && + inspect addtest/submod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -237,7 +230,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch ../.. && + inspect addtest/submod-branch && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -253,7 +246,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz ../../.. && + inspect addtest/dotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -269,7 +262,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz ../../.. && + inspect addtest/dotslashdotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -285,7 +278,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz ../../.. && + inspect addtest/slashslashsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -301,7 +294,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod ../.. && + inspect addtest/realsubmod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -317,7 +310,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 ../.. && + inspect addtest/realsubmod2 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -348,7 +341,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 ../.. && + inspect addtest/realsubmod3 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper 2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer @ 2021-07-27 17:12 ` Jonathan Tan 2021-08-19 17:46 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Jonathan Tan @ 2021-07-27 17:12 UTC (permalink / raw) To: emilyshaffer; +Cc: git, Jonathan Tan > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index a924fdb7a6..f5dc051a6e 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' ' > # generates, which will expand symbolic links. > submodurl=$(pwd -P) > > -listbranches() { > - git for-each-ref --format='%(refname)' 'refs/heads/*' > -} > - > inspect() { > dir=$1 && > - dotdot="${2:-..}" && > > - ( > - cd "$dir" && > - listbranches >"$dotdot/heads" && > - { git symbolic-ref HEAD || :; } >"$dotdot/head" && > - git rev-parse HEAD >"$dotdot/head-sha1" && > - git update-index --refresh && > - git diff-files --exit-code && > - git clean -n -d -x >"$dotdot/untracked" > - ) > + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > + { git -C "$dir" symbolic-ref HEAD || :; } >head && > + git -C "$dir" rev-parse HEAD >head-sha1 && > + git -C "$dir" update-index --refresh && > + git -C "$dir" diff-files --exit-code && > + git -C "$dir" clean -n -d -x >untracked > } > > + Stray extra line. Other than that, this is definitely a good simplification. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper 2021-07-27 17:12 ` Jonathan Tan @ 2021-08-19 17:46 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 17:46 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Tue, Jul 27, 2021 at 10:12:25AM -0700, Jonathan Tan wrote: > > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > > index a924fdb7a6..f5dc051a6e 100755 > > --- a/t/t7400-submodule-basic.sh > > +++ b/t/t7400-submodule-basic.sh > > @@ -107,25 +107,18 @@ test_expect_success 'setup - repository to add submodules to' ' > > # generates, which will expand symbolic links. > > submodurl=$(pwd -P) > > > > -listbranches() { > > - git for-each-ref --format='%(refname)' 'refs/heads/*' > > -} > > - > > inspect() { > > dir=$1 && > > - dotdot="${2:-..}" && > > > > - ( > > - cd "$dir" && > > - listbranches >"$dotdot/heads" && > > - { git symbolic-ref HEAD || :; } >"$dotdot/head" && > > - git rev-parse HEAD >"$dotdot/head-sha1" && > > - git update-index --refresh && > > - git diff-files --exit-code && > > - git clean -n -d -x >"$dotdot/untracked" > > - ) > > + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > > + { git -C "$dir" symbolic-ref HEAD || :; } >head && > > + git -C "$dir" rev-parse HEAD >head-sha1 && > > + git -C "$dir" update-index --refresh && > > + git -C "$dir" diff-files --exit-code && > > + git -C "$dir" clean -n -d -x >untracked > > } > > > > + > > Stray extra line. > > Other than that, this is definitely a good simplification. Ack, fixed locally. Thanks. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer @ 2021-06-16 0:45 ` Emily Shaffer 2021-06-16 4:40 ` Junio C Hamano ` (2 more replies) 2021-06-16 0:45 ` [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer ` (2 subsequent siblings) 4 siblings, 3 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-16 0:45 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Teach submodules a reference to their superproject's gitdir. This allows us to A) know that we're running from a submodule, and B) have a shortcut to the superproject's vitals, for example, configs. By using a relative path instead of an absolute path, we can move the superproject directory around on the filesystem without breaking the submodule's cache. Since this cached value is only introduced during new submodule creation via `git submodule add`, though, there is more work to do to allow the cache to be created at other times. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/config/submodule.txt | 12 +++++++++ builtin/submodule--helper.c | 4 +++ t/t7400-submodule-basic.sh | 40 ++++++++++++++++-------------- 3 files changed, 38 insertions(+), 18 deletions(-) diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt index d7a63c8c12..7c459cc19e 100644 --- a/Documentation/config/submodule.txt +++ b/Documentation/config/submodule.txt @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy:: `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` or `info`, and if there is an error with the computed alternate, the clone proceeds as if no alternate was specified. + +submodule.superprojectGitDir:: + The relative path from the submodule's worktree to the superproject's + gitdir. This config should only be present in projects which are + submodules, but is not guaranteed to be present in every submodule. It + is set automatically during submodule creation. ++ + In situations where more than one superproject references the same + submodule worktree, the value of this config and the behavior of + operations which use it are undefined. To reference a single project + from multiple superprojects, it is better to create a worktree of the + submodule for each superproject. diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d55f6262e9..d60fcd2c7d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) git_config_set_in_file(p, "submodule.alternateErrorStrategy", error_strategy); + git_config_set_in_file(p, "submodule.superprojectGitdir", + relative_path(absolute_path(get_git_dir()), + path, &sb)); + free(sm_alternate); free(error_strategy); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index f5dc051a6e..e45f42588f 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' ' submodurl=$(pwd -P) inspect() { - dir=$1 && - - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && - { git -C "$dir" symbolic-ref HEAD || :; } >head && - git -C "$dir" rev-parse HEAD >head-sha1 && - git -C "$dir" update-index --refresh && - git -C "$dir" diff-files --exit-code && - git -C "$dir" clean -n -d -x >untracked + sub_dir=$1 && + super_dir=$2 && + + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && + git -C "$sub_dir" rev-parse HEAD >head-sha1 && + git -C "$sub_dir" update-index --refresh && + git -C "$sub_dir" diff-files --exit-code && + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ + -ef "$sub_dir/$cached_super_dir" ] && + git -C "$sub_dir" clean -n -d -x >untracked } @@ -139,7 +143,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod && + inspect addtest/submod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -230,7 +234,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch && + inspect addtest/submod-branch addtest && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -246,7 +250,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz && + inspect addtest/dotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -262,7 +266,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz && + inspect addtest/dotslashdotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -278,7 +282,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz && + inspect addtest/slashslashsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -294,7 +298,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod && + inspect addtest/realsubmod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -310,7 +314,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 && + inspect addtest/realsubmod2 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -341,7 +345,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 && + inspect addtest/realsubmod3 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -482,7 +486,7 @@ test_expect_success 'update should work when path is an empty dir' ' git submodule update -q >update.out && test_must_be_empty update.out && - inspect init && + inspect init . && test_cmp expect head-sha1 ' @@ -541,7 +545,7 @@ test_expect_success 'update should checkout rev1' ' echo "$rev1" >expect && git submodule update init && - inspect init && + inspect init . && test_cmp expect head-sha1 ' -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer @ 2021-06-16 4:40 ` Junio C Hamano 2021-06-16 4:43 ` Junio C Hamano 2021-06-18 0:00 ` Emily Shaffer 2021-07-27 17:46 ` Jonathan Tan 2021-10-14 19:25 ` Ævar Arnfjörð Bjarmason 2 siblings, 2 replies; 51+ messages in thread From: Junio C Hamano @ 2021-06-16 4:40 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> writes: > Teach submodules a reference to their superproject's gitdir. This allows > us to A) know that we're running from a submodule, and B) have a > shortcut to the superproject's vitals, for example, configs. > > By using a relative path instead of an absolute path, we can move the > superproject directory around on the filesystem without breaking the > submodule's cache. > > Since this cached value is only introduced during new submodule creation > via `git submodule add`, though, there is more work to do to allow the > cache to be created at other times. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > Documentation/config/submodule.txt | 12 +++++++++ > builtin/submodule--helper.c | 4 +++ > t/t7400-submodule-basic.sh | 40 ++++++++++++++++-------------- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt > index d7a63c8c12..7c459cc19e 100644 > --- a/Documentation/config/submodule.txt > +++ b/Documentation/config/submodule.txt > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy:: > `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` > or `info`, and if there is an error with the computed alternate, the > clone proceeds as if no alternate was specified. > + > +submodule.superprojectGitDir:: > + The relative path from the submodule's worktree to the superproject's > + gitdir. This config should only be present in projects which are > + submodules, but is not guaranteed to be present in every submodule. It > + is set automatically during submodule creation. > ++ > + In situations where more than one superproject references the same > + submodule worktree, the value of this config and the behavior of > + operations which use it are undefined. To reference a single project > + from multiple superprojects, it is better to create a worktree of the > + submodule for each superproject. You'd need to dedent the second paragraph that follows a lone '+' sign to typeset this correctly. The new paragraph suggests separate worktrees for the same submodule repository, but for that to work correctly, - "git clone [--recurse-submodules]" that clones the second superproject that shares the same submodule with a superproject that we already locally has to support a way for users to tell where to grab that existing submodule from and arrange a new worktree, instead of creating another instance of the submodule repository by cloning it afresh. - The "submodule.superprojectGitDir" needs to be set to per-worktree half of the repo-local configuration file. Because I usually do not pay much attention to the submodule part of the toolset, I may well be mistaken, but I suspect that the former does not exist yet. If I recall correctly, the latter was a NEEDSWORK item in the previous round of this patchset? As I said, I think it is OK for now to stop at declaring that you cannot simply do it without hinting something that may not fully work. Thanks. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 4:40 ` Junio C Hamano @ 2021-06-16 4:43 ` Junio C Hamano 2021-06-18 0:03 ` Emily Shaffer 2021-06-18 0:00 ` Emily Shaffer 1 sibling, 1 reply; 51+ messages in thread From: Junio C Hamano @ 2021-06-16 4:43 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > As I said, I think it is OK for now to stop at declaring that you > cannot simply do it without hinting something that may not fully > work. I'll add the following to the tip of the topic for now. Documentation/config/submodule.txt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt index 7c459cc19e..58ba63a75e 100644 --- c/Documentation/config/submodule.txt +++ w/Documentation/config/submodule.txt @@ -97,8 +97,5 @@ submodule.superprojectGitDir:: submodules, but is not guaranteed to be present in every submodule. It is set automatically during submodule creation. + - In situations where more than one superproject references the same - submodule worktree, the value of this config and the behavior of - operations which use it are undefined. To reference a single project - from multiple superprojects, it is better to create a worktree of the - submodule for each superproject. +Because of this configuration variable, it is forbidden to use the +same submodule worktree shared by multiple superprojects. ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 4:43 ` Junio C Hamano @ 2021-06-18 0:03 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-18 0:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jun 16, 2021 at 01:43:36PM +0900, Junio C Hamano wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > As I said, I think it is OK for now to stop at declaring that you > > cannot simply do it without hinting something that may not fully > > work. > > I'll add the following to the tip of the topic for now. Just saw this - yes, it looks fine to me. I'll squash that locally in case anybody has more comments and wants a reroll. > > > Documentation/config/submodule.txt | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git c/Documentation/config/submodule.txt w/Documentation/config/submodule.txt > index 7c459cc19e..58ba63a75e 100644 > --- c/Documentation/config/submodule.txt > +++ w/Documentation/config/submodule.txt > @@ -97,8 +97,5 @@ submodule.superprojectGitDir:: > submodules, but is not guaranteed to be present in every submodule. It > is set automatically during submodule creation. > + > - In situations where more than one superproject references the same > - submodule worktree, the value of this config and the behavior of > - operations which use it are undefined. To reference a single project > - from multiple superprojects, it is better to create a worktree of the > - submodule for each superproject. > +Because of this configuration variable, it is forbidden to use the > +same submodule worktree shared by multiple superprojects. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 4:40 ` Junio C Hamano 2021-06-16 4:43 ` Junio C Hamano @ 2021-06-18 0:00 ` Emily Shaffer 1 sibling, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-18 0:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jun 16, 2021 at 01:40:36PM +0900, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > +submodule.superprojectGitDir:: > > + The relative path from the submodule's worktree to the superproject's > > + gitdir. This config should only be present in projects which are > > + submodules, but is not guaranteed to be present in every submodule. It > > + is set automatically during submodule creation. > > ++ > > + In situations where more than one superproject references the same > > + submodule worktree, the value of this config and the behavior of > > + operations which use it are undefined. To reference a single project > > + from multiple superprojects, it is better to create a worktree of the > > + submodule for each superproject. > > You'd need to dedent the second paragraph that follows a lone '+' > sign to typeset this correctly. Ok. > > The new paragraph suggests separate worktrees for the same submodule > repository, but for that to work correctly, > > - "git clone [--recurse-submodules]" that clones the second > superproject that shares the same submodule with a superproject > that we already locally has to support a way for users to tell > where to grab that existing submodule from and arrange a new > worktree, instead of creating another instance of the submodule > repository by cloning it afresh. > > - The "submodule.superprojectGitDir" needs to be set to > per-worktree half of the repo-local configuration file. > > Because I usually do not pay much attention to the submodule part of > the toolset, I may well be mistaken, but I suspect that the former > does not exist yet. If I recall correctly, the latter was a NEEDSWORK > item in the previous round of this patchset? > > As I said, I think it is OK for now to stop at declaring that you > cannot simply do it without hinting something that may not fully > work. Yeah, that is all correct. Ok, I will drop the broken suggestion. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer 2021-06-16 4:40 ` Junio C Hamano @ 2021-07-27 17:46 ` Jonathan Tan 2021-08-19 17:53 ` Emily Shaffer 2021-10-14 19:25 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 51+ messages in thread From: Jonathan Tan @ 2021-07-27 17:46 UTC (permalink / raw) To: emilyshaffer; +Cc: git, Jonathan Tan > Teach submodules a reference to their superproject's gitdir. This allows > us to A) know that we're running from a submodule, and B) have a > shortcut to the superproject's vitals, for example, configs. The first sentence is probably better "Introduce a new config variable storing a submodule's reference to its superproject's gitdir, and teach 'git submodule add' to write it". Also, I think there should be more detail about the planned use both here in the commit message and in the config documentation. Is the plan just to use it for best-effort explanatory messages? (Using it as a true cache is probably too performance-intensive, I would think, since in its absence, we have no idea whether the repo is a submodule and would always have to search to the root of the filesystem.) If it is just for best-effort explanatory messages, maybe write: If present, commands like "git status" in this submodule may print additional information about this submodule's status with respect to its superproject. This would further reinforce that this variable being missing is OK. > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt > index d7a63c8c12..7c459cc19e 100644 > --- a/Documentation/config/submodule.txt > +++ b/Documentation/config/submodule.txt > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy:: > `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` > or `info`, and if there is an error with the computed alternate, the > clone proceeds as if no alternate was specified. > + > +submodule.superprojectGitDir:: > + The relative path from the submodule's worktree to the superproject's > + gitdir. This config should only be present in projects which are > + submodules, but is not guaranteed to be present in every submodule. It > + is set automatically during submodule creation. > ++ > + In situations where more than one superproject references the same > + submodule worktree, the value of this config and the behavior of > + operations which use it are undefined. To reference a single project > + from multiple superprojects, it is better to create a worktree of the > + submodule for each superproject. So my suggestion would be: The relative path from this repository's worktree to its superproject's gitdir. When Git is run in a repository, it usually makes no difference whether this repository is standalone or a submodule, but if this configuration variable is present, commands like "git status" in this submodule may print additional information about this submodule's status with respect to its superproject. (I believe Junio has commented on the second paragraph - I don't have additional comments on that.) ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-07-27 17:46 ` Jonathan Tan @ 2021-08-19 17:53 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 17:53 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Tue, Jul 27, 2021 at 10:46:50AM -0700, Jonathan Tan wrote: > > > Teach submodules a reference to their superproject's gitdir. This allows > > us to A) know that we're running from a submodule, and B) have a > > shortcut to the superproject's vitals, for example, configs. > > The first sentence is probably better "Introduce a new config variable > storing a submodule's reference to its superproject's gitdir, and teach > 'git submodule add' to write it". > > Also, I think there should be more detail about the planned use both > here in the commit message and in the config documentation. Is the plan > just to use it for best-effort explanatory messages? (Using it as a true > cache is probably too performance-intensive, I would think, since in its > absence, we have no idea whether the repo is a submodule and would > always have to search to the root of the filesystem.) If it is just for > best-effort explanatory messages, maybe write: > > If present, commands like "git status" in this submodule may print > additional information about this submodule's status with respect to > its superproject. > > This would further reinforce that this variable being missing is OK. Ok, I'll expand the commit message. The first use case for this extra pointer is for a shared config between superproject and submodule, which I've sent a series for already; I'll mention that in the commit message too. > > > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt > > index d7a63c8c12..7c459cc19e 100644 > > --- a/Documentation/config/submodule.txt > > +++ b/Documentation/config/submodule.txt > > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy:: > > `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` > > or `info`, and if there is an error with the computed alternate, the > > clone proceeds as if no alternate was specified. > > + > > +submodule.superprojectGitDir:: > > + The relative path from the submodule's worktree to the superproject's > > + gitdir. This config should only be present in projects which are > > + submodules, but is not guaranteed to be present in every submodule. It > > + is set automatically during submodule creation. > > ++ > > + In situations where more than one superproject references the same > > + submodule worktree, the value of this config and the behavior of > > + operations which use it are undefined. To reference a single project > > + from multiple superprojects, it is better to create a worktree of the > > + submodule for each superproject. > > So my suggestion would be: > > The relative path from this repository's worktree to its > superproject's gitdir. When Git is run in a repository, it usually > makes no difference whether this repository is standalone or a > submodule, but if this configuration variable is present, commands > like "git status" in this submodule may print additional information > about this submodule's status with respect to its superproject. > > (I believe Junio has commented on the second paragraph - I don't have > additional comments on that.) The spirit of this suggestion seems to be "describe a possible side effect of this config", so I'll do that, although the wording may not be exactly the same. Thanks. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 2/4] introduce submodule.superprojectGitDir cache 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer 2021-06-16 4:40 ` Junio C Hamano 2021-07-27 17:46 ` Jonathan Tan @ 2021-10-14 19:25 ` Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 51+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-14 19:25 UTC (permalink / raw) To: Emily Shaffer; +Cc: git On Tue, Jun 15 2021, Emily Shaffer wrote: > Teach submodules a reference to their superproject's gitdir. This allows > us to A) know that we're running from a submodule, and B) have a > shortcut to the superproject's vitals, for example, configs. > > By using a relative path instead of an absolute path, we can move the > superproject directory around on the filesystem without breaking the > submodule's cache. > > Since this cached value is only introduced during new submodule creation > via `git submodule add`, though, there is more work to do to allow the > cache to be created at other times. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > Documentation/config/submodule.txt | 12 +++++++++ > builtin/submodule--helper.c | 4 +++ > t/t7400-submodule-basic.sh | 40 ++++++++++++++++-------------- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt > index d7a63c8c12..7c459cc19e 100644 > --- a/Documentation/config/submodule.txt > +++ b/Documentation/config/submodule.txt > @@ -90,3 +90,15 @@ submodule.alternateErrorStrategy:: > `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` > or `info`, and if there is an error with the computed alternate, the > clone proceeds as if no alternate was specified. > + > +submodule.superprojectGitDir:: > + The relative path from the submodule's worktree to the superproject's > + gitdir. This config should only be present in projects which are > + submodules, but is not guaranteed to be present in every submodule. It > + is set automatically during submodule creation. > ++ > + In situations where more than one superproject references the same > + submodule worktree, the value of this config and the behavior of > + operations which use it are undefined. To reference a single project > + from multiple superprojects, it is better to create a worktree of the > + submodule for each superproject. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d55f6262e9..d60fcd2c7d 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) > git_config_set_in_file(p, "submodule.alternateErrorStrategy", > error_strategy); > > + git_config_set_in_file(p, "submodule.superprojectGitdir", > + relative_path(absolute_path(get_git_dir()), > + path, &sb)); > + > free(sm_alternate); > free(error_strategy); > Am I correct that what this series really does is avoid needing to: 1. Run the equivalent of $(git rev-parse --absolute-git-dir), let's call the result of that $X. 2. Feed that to the equvialent of $(git -C $X/../ rev-parse --absolute-git-dir) 3. Check if the relationship between the two is really that of a submodule, i.e. running "git submodule status", check if $X contains ".git/modules/" etc. I see an earlier iteration of this series had such a shell-out, and that this is the "cache": https://lore.kernel.org/git/20210423001539.4059524-5-emilyshaffer@google.com/; and your v1 cover letter seems to support the above summary: https://lore.kernel.org/git/20210611225428.1208973-1-emilyshaffer@google.com/ I think it's fine to fine to add such a cache in principle if it's needed. But I'm a bit iffy on a series that's structured in a way as to not start by asserting that we should have given semantics without the cache, and then adds the cache later as an optional optimization. Particularly as the whole submodule business is moving to C, so isn't this whole caching business something we can avoid doing, and instead just call a C function? The optimization part of this was not calling "git rev-parse" on every submodule invocation wasn't it, not avoiding a few syscalls that deal with the FS. Your initial RFC had modifications to git-submodule.sh, in the interim it seems that's been moved sufficiently to C that we're modifying just the submodule.c here. I'm not very familiar with setup_git_directory_gently_1(), discover_git_directory() etc, but wherever you are in a git worktree we'll chdir() to the top-level when running built-ins. So that gives us step #1 of the above. And #2 would be adding "/../" to the end of that path and running those functions again? Perhaps with a #3 for "is there a submodule relationship?". Even if we still have some bits in shellscript etc, couldn't we then setenv() that in some GIT_* variable, e.g. GIT_SUPERPROJECT_DIR? Or is the problem really that this isn't a cache, because we can't say with absolute certainty that there is such a gitdir/submodule relationship, except at the time of running "git submodule" in the former for some reason? ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer @ 2021-06-16 0:45 ` Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer 4 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-06-16 0:45 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Already during 'git submodule add' we cache a pointer to the superproject's gitdir. However, this doesn't help brand-new submodules created with 'git init' and later absorbed with 'git submodule absorbgitdir'. Let's start adding that pointer during 'git submodule absorbgitdir' too. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- submodule.c | 10 ++++++++++ t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 0b1d9c1dde..4b314bf09c 100644 --- a/submodule.c +++ b/submodule.c @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path) char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; char *new_git_dir; const struct submodule *sub; + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; if (submodule_uses_worktrees(path)) die(_("relocate_gitdir for submodule '%s' with " @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path) relocate_gitdir(path, real_old_git_dir, real_new_git_dir); + /* cache pointer to superproject's gitdir */ + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ + strbuf_addf(&config_path, "%s/config", real_new_git_dir); + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", + relative_path(get_super_prefix_or_empty(), + path, &sb)); + + strbuf_release(&config_path); + strbuf_release(&sb); free(old_git_dir); free(real_old_git_dir); free(real_new_git_dir); diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 1cfa150768..e2d78e01df 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' ' git status >actual.1 && git -C sub1 rev-parse HEAD >actual.2 && test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + # make sure the submodule cached the superproject gitdir correctly + test-tool path-utils real_path . >expect && + test-tool path-utils real_path \ + "$(git -C sub1 config submodule.superprojectGitDir)" >actual && + + test_cmp expect actual ' test_expect_success 'absorbing does not fail for deinitialized submodules' ' -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer ` (2 preceding siblings ...) 2021-06-16 0:45 ` [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer @ 2021-06-16 0:45 ` Emily Shaffer 2021-07-27 17:51 ` Jonathan Tan 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer 4 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-06-16 0:45 UTC (permalink / raw) To: git; +Cc: Emily Shaffer A cached path to the superproject's gitdir might be added during 'git submodule add', but in some cases - like submodules which were created before 'git submodule add' learned to cache that info - it might be useful to update the cache. Let's do it during 'git submodule update', when we already have a handle to the superproject while calling operations on the submodules. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- git-submodule.sh | 10 ++++++++++ t/t7406-submodule-update.sh | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index 4678378424..f98dcc16ae 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -648,6 +648,16 @@ cmd_update() fi fi + # Cache a pointer to the superproject's gitdir. This may have + # changed, so rewrite it unconditionally. Writes it to worktree + # if applicable, otherwise to local. + relative_gitdir="$(git rev-parse --path-format=relative \ + --prefix "${sm_path}" \ + --git-dir)" + + git -C "$sm_path" config --worktree \ + submodule.superprojectgitdir "$relative_gitdir" + if test -n "$recursive" then ( diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index f4f61fe554..c39821ba8e 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s ) ' +test_expect_success 'submodule update adds superproject gitdir to older repos' ' + (cd super && + git -C submodule config --unset submodule.superprojectGitdir && + git submodule update && + echo "../.git" >expect && + git -C submodule config submodule.superprojectGitdir >actual && + test_cmp expect actual + ) +' + test_done -- 2.32.0.272.g935e593368-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' 2021-06-16 0:45 ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer @ 2021-07-27 17:51 ` Jonathan Tan 2021-08-19 18:02 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Jonathan Tan @ 2021-07-27 17:51 UTC (permalink / raw) To: emilyshaffer; +Cc: git, Jonathan Tan > A cached path to the superproject's gitdir might be added during 'git [snip] > + # Cache a pointer to the superproject's gitdir. This may have Patches 3 and 4 look good to me except that for me, a cache is something that lets us avoid an expensive computation. But as far as I know, we're not planning to perform the expensive computation in the absence of a cache. Maybe the word "record" would fit better. This is nit-picky, though, and if others think that the word "cache" fits here, that's fine by me too. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' 2021-07-27 17:51 ` Jonathan Tan @ 2021-08-19 18:02 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 18:02 UTC (permalink / raw) To: Jonathan Tan; +Cc: git On Tue, Jul 27, 2021 at 10:51:51AM -0700, Jonathan Tan wrote: > > > A cached path to the superproject's gitdir might be added during 'git > > [snip] > > > + # Cache a pointer to the superproject's gitdir. This may have > > Patches 3 and 4 look good to me except that for me, a cache is something > that lets us avoid an expensive computation. But as far as I know, we're > not planning to perform the expensive computation in the absence of a > cache. Maybe the word "record" would fit better. > > This is nit-picky, though, and if others think that the word "cache" > fits here, that's fine by me too. Thanks. I switched the commit messages around to use 'record' and 'hint' when appropriate instead. I'll send a reroll with those nits as soon as it passes CI. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer ` (3 preceding siblings ...) 2021-06-16 0:45 ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer @ 2021-08-19 20:09 ` Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer ` (6 more replies) 4 siblings, 7 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 20:09 UTC (permalink / raw) To: git Cc: Emily Shaffer, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan Since v2, mostly documentation changes and a handful of small nits from Junio and Jonathan Tan. Thanks for the reviews, both. CI run here: https://github.com/nasamuffin/git/actions/runs/1147984974 - Emily Emily Shaffer (4): t7400-submodule-basic: modernize inspect() helper introduce submodule.superprojectGitDir record submodule: record superproject gitdir during absorbgitdirs submodule: record superproject gitdir during 'update' Documentation/config/submodule.txt | 15 ++++++++++ builtin/submodule--helper.c | 4 +++ git-submodule.sh | 10 +++++++ submodule.c | 10 +++++++ t/t7400-submodule-basic.sh | 48 ++++++++++++++---------------- t/t7406-submodule-update.sh | 10 +++++++ t/t7412-submodule-absorbgitdirs.sh | 9 +++++- 7 files changed, 79 insertions(+), 27 deletions(-) Range-diff against v2: 1: a6718eea80 ! 1: f1236dc9d7 t7400-submodule-basic: modernize inspect() helper @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo + git -C "$dir" clean -n -d -x >untracked } -+ test_expect_success 'submodule add' ' - echo "refs/heads/main" >expect && - @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' ' ) && 2: 4cebe7bcb5 ! 2: 2caf9eb8ee introduce submodule.superprojectGitDir cache @@ Metadata Author: Emily Shaffer <emilyshaffer@google.com> ## Commit message ## - introduce submodule.superprojectGitDir cache + introduce submodule.superprojectGitDir record Teach submodules a reference to their superproject's gitdir. This allows us to A) know that we're running from a submodule, and B) have a @@ Commit message superproject directory around on the filesystem without breaking the submodule's cache. - Since this cached value is only introduced during new submodule creation + Since this hint value is only introduced during new submodule creation via `git submodule add`, though, there is more work to do to allow the - cache to be created at other times. + record to be created at other times. + + If the new config is present, we can do some optional value-added + behavior, like letting "git status" print additional info about the + submodule's status in relation to its superproject, or like letting the + superproject and submodule share an additional config file separate from + either one's local config. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> + Helped-by: Junio C Hamano <gitster@pobox.com> ## Documentation/config/submodule.txt ## @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy:: @@ Documentation/config/submodule.txt: submodule.alternateErrorStrategy:: clone proceeds as if no alternate was specified. + +submodule.superprojectGitDir:: -+ The relative path from the submodule's worktree to the superproject's -+ gitdir. This config should only be present in projects which are -+ submodules, but is not guaranteed to be present in every submodule. It -+ is set automatically during submodule creation. ++ The relative path from the submodule's worktree to its superproject's ++ gitdir. When Git is run in a repository, it usually makes no difference ++ whether this repository is standalone or a submodule, but if this ++ configuration variable is present, additional behavior may be possible, ++ such as "git status" printing additional information about this ++ submodule's status with respect to its superproject. This config should ++ only be present in projects which are submodules, but is not guaranteed ++ to be present in every submodule, so only optional value-added behavior ++ should be linked to it. It is set automatically during ++ submodule creation. ++ -+ In situations where more than one superproject references the same -+ submodule worktree, the value of this config and the behavior of -+ operations which use it are undefined. To reference a single project -+ from multiple superprojects, it is better to create a worktree of the -+ submodule for each superproject. ++ Because of this configuration variable, it is forbidden to use the ++ same submodule worktree shared by multiple superprojects. ## builtin/submodule--helper.c ## @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix) @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository to add submo + git -C "$sub_dir" clean -n -d -x >untracked } - + test_expect_success 'submodule add' ' @@ t/t7400-submodule-basic.sh: test_expect_success 'submodule add' ' ) && 3: df97a9c2bb ! 3: d278568a8e submodule: cache superproject gitdir during absorbgitdirs @@ Metadata Author: Emily Shaffer <emilyshaffer@google.com> ## Commit message ## - submodule: cache superproject gitdir during absorbgitdirs + submodule: record superproject gitdir during absorbgitdirs - Already during 'git submodule add' we cache a pointer to the + Already during 'git submodule add' we record a pointer to the superproject's gitdir. However, this doesn't help brand-new submodules created with 'git init' and later absorbed with 'git submodule absorbgitdir'. Let's start adding that pointer during 'git 4: a3f3be58ad ! 4: 6866c36620 submodule: cache superproject gitdir during 'update' @@ Metadata Author: Emily Shaffer <emilyshaffer@google.com> ## Commit message ## - submodule: cache superproject gitdir during 'update' + submodule: record superproject gitdir during 'update' - A cached path to the superproject's gitdir might be added during 'git - submodule add', but in some cases - like submodules which were created - before 'git submodule add' learned to cache that info - it might be - useful to update the cache. Let's do it during 'git submodule update', - when we already have a handle to the superproject while calling + A recorded hint path to the superproject's gitdir might be added during + 'git submodule add', but in some cases - like submodules which were + created before 'git submodule add' learned to record that info - it might + be useful to update the hint. Let's do it during 'git submodule + update', when we already have a handle to the superproject while calling operations on the submodules. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer @ 2021-08-19 20:09 ` Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer ` (5 subsequent siblings) 6 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 20:09 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Since the inspect() helper in the submodule-basic test suite was written, 'git -C <dir>' was added. By using -C, we no longer need a reference to the base directory for the test. This simplifies callsites, and will make the addition of other arguments in later patches more readable. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t7400-submodule-basic.sh | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index a924fdb7a6..4bc6b6c886 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -107,23 +107,15 @@ test_expect_success 'setup - repository to add submodules to' ' # generates, which will expand symbolic links. submodurl=$(pwd -P) -listbranches() { - git for-each-ref --format='%(refname)' 'refs/heads/*' -} - inspect() { dir=$1 && - dotdot="${2:-..}" && - ( - cd "$dir" && - listbranches >"$dotdot/heads" && - { git symbolic-ref HEAD || :; } >"$dotdot/head" && - git rev-parse HEAD >"$dotdot/head-sha1" && - git update-index --refresh && - git diff-files --exit-code && - git clean -n -d -x >"$dotdot/untracked" - ) + git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$dir" symbolic-ref HEAD || :; } >head && + git -C "$dir" rev-parse HEAD >head-sha1 && + git -C "$dir" update-index --refresh && + git -C "$dir" diff-files --exit-code && + git -C "$dir" clean -n -d -x >untracked } test_expect_success 'submodule add' ' @@ -146,7 +138,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod ../.. && + inspect addtest/submod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -237,7 +229,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch ../.. && + inspect addtest/submod-branch && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -253,7 +245,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz ../../.. && + inspect addtest/dotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -269,7 +261,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz ../../.. && + inspect addtest/dotslashdotsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -285,7 +277,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz ../../.. && + inspect addtest/slashslashsubmod/frotz && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -301,7 +293,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod ../.. && + inspect addtest/realsubmod && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -317,7 +309,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 ../.. && + inspect addtest/realsubmod2 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -348,7 +340,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 ../.. && + inspect addtest/realsubmod3 && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* [PATCH v3 2/4] introduce submodule.superprojectGitDir record 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer @ 2021-08-19 20:09 ` Emily Shaffer 2021-08-20 0:38 ` Derrick Stolee 2021-09-04 17:20 ` Matheus Tavares 2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer ` (4 subsequent siblings) 6 siblings, 2 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 20:09 UTC (permalink / raw) To: git; +Cc: Emily Shaffer, Junio C Hamano Teach submodules a reference to their superproject's gitdir. This allows us to A) know that we're running from a submodule, and B) have a shortcut to the superproject's vitals, for example, configs. By using a relative path instead of an absolute path, we can move the superproject directory around on the filesystem without breaking the submodule's cache. Since this hint value is only introduced during new submodule creation via `git submodule add`, though, there is more work to do to allow the record to be created at other times. If the new config is present, we can do some optional value-added behavior, like letting "git status" print additional info about the submodule's status in relation to its superproject, or like letting the superproject and submodule share an additional config file separate from either one's local config. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Junio C Hamano <gitster@pobox.com> --- Documentation/config/submodule.txt | 15 +++++++++++ builtin/submodule--helper.c | 4 +++ t/t7400-submodule-basic.sh | 40 ++++++++++++++++-------------- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt index d7a63c8c12..23e0a01d90 100644 --- a/Documentation/config/submodule.txt +++ b/Documentation/config/submodule.txt @@ -90,3 +90,18 @@ submodule.alternateErrorStrategy:: `ignore`, `info`, `die`. Default is `die`. Note that if set to `ignore` or `info`, and if there is an error with the computed alternate, the clone proceeds as if no alternate was specified. + +submodule.superprojectGitDir:: + The relative path from the submodule's worktree to its superproject's + gitdir. When Git is run in a repository, it usually makes no difference + whether this repository is standalone or a submodule, but if this + configuration variable is present, additional behavior may be possible, + such as "git status" printing additional information about this + submodule's status with respect to its superproject. This config should + only be present in projects which are submodules, but is not guaranteed + to be present in every submodule, so only optional value-added behavior + should be linked to it. It is set automatically during + submodule creation. ++ + Because of this configuration variable, it is forbidden to use the + same submodule worktree shared by multiple superprojects. diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d55f6262e9..d60fcd2c7d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) git_config_set_in_file(p, "submodule.alternateErrorStrategy", error_strategy); + git_config_set_in_file(p, "submodule.superprojectGitdir", + relative_path(absolute_path(get_git_dir()), + path, &sb)); + free(sm_alternate); free(error_strategy); diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 4bc6b6c886..e407329d81 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' ' submodurl=$(pwd -P) inspect() { - dir=$1 && - - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && - { git -C "$dir" symbolic-ref HEAD || :; } >head && - git -C "$dir" rev-parse HEAD >head-sha1 && - git -C "$dir" update-index --refresh && - git -C "$dir" diff-files --exit-code && - git -C "$dir" clean -n -d -x >untracked + sub_dir=$1 && + super_dir=$2 && + + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && + git -C "$sub_dir" rev-parse HEAD >head-sha1 && + git -C "$sub_dir" update-index --refresh && + git -C "$sub_dir" diff-files --exit-code && + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ + -ef "$sub_dir/$cached_super_dir" ] && + git -C "$sub_dir" clean -n -d -x >untracked } test_expect_success 'submodule add' ' @@ -138,7 +142,7 @@ test_expect_success 'submodule add' ' ) && rm -f heads head untracked && - inspect addtest/submod && + inspect addtest/submod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -229,7 +233,7 @@ test_expect_success 'submodule add --branch' ' ) && rm -f heads head untracked && - inspect addtest/submod-branch && + inspect addtest/submod-branch addtest && test_cmp expect-heads heads && test_cmp expect-head head && test_must_be_empty untracked @@ -245,7 +249,7 @@ test_expect_success 'submodule add with ./ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotsubmod/frotz && + inspect addtest/dotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -261,7 +265,7 @@ test_expect_success 'submodule add with /././ in path' ' ) && rm -f heads head untracked && - inspect addtest/dotslashdotsubmod/frotz && + inspect addtest/dotslashdotsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -277,7 +281,7 @@ test_expect_success 'submodule add with // in path' ' ) && rm -f heads head untracked && - inspect addtest/slashslashsubmod/frotz && + inspect addtest/slashslashsubmod/frotz addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -293,7 +297,7 @@ test_expect_success 'submodule add with /.. in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod && + inspect addtest/realsubmod addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -309,7 +313,7 @@ test_expect_success 'submodule add with ./, /.. and // in path' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod2 && + inspect addtest/realsubmod2 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -340,7 +344,7 @@ test_expect_success 'submodule add in subdirectory' ' ) && rm -f heads head untracked && - inspect addtest/realsubmod3 && + inspect addtest/realsubmod3 addtest && test_cmp expect heads && test_cmp expect head && test_must_be_empty untracked @@ -481,7 +485,7 @@ test_expect_success 'update should work when path is an empty dir' ' git submodule update -q >update.out && test_must_be_empty update.out && - inspect init && + inspect init . && test_cmp expect head-sha1 ' @@ -540,7 +544,7 @@ test_expect_success 'update should checkout rev1' ' echo "$rev1" >expect && git submodule update init && - inspect init && + inspect init . && test_cmp expect head-sha1 ' -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record 2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer @ 2021-08-20 0:38 ` Derrick Stolee 2021-10-13 19:36 ` Emily Shaffer 2021-09-04 17:20 ` Matheus Tavares 1 sibling, 1 reply; 51+ messages in thread From: Derrick Stolee @ 2021-08-20 0:38 UTC (permalink / raw) To: Emily Shaffer, git; +Cc: Junio C Hamano On 8/19/2021 4:09 PM, Emily Shaffer wrote: ... > +submodule.superprojectGitDir:: > + The relative path from the submodule's worktree to its superproject's > + gitdir. When Git is run in a repository, it usually makes no difference > + whether this repository is standalone or a submodule, but if this > + configuration variable is present, additional behavior may be possible, > + such as "git status" printing additional information about this > + submodule's status with respect to its superproject. This config should > + only be present in projects which are submodules, but is not guaranteed > + to be present in every submodule, so only optional value-added behavior > + should be linked to it. It is set automatically during > + submodule creation. > ++ > + Because of this configuration variable, it is forbidden to use the > + same submodule worktree shared by multiple superprojects. nit: this paragraph linked with the "+" line should have no tabbing. Also, could we use the same submodule worktree for multiple superprojects _before_ this configuration variable? That seems wild to me. Or, is that not a new requirement? Perhaps you mean something like this instead: It is forbidden to use the same submodule worktree for multiple superprojects, so this configuration variable stores the unique superproject and is not multi-valued. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d55f6262e9..d60fcd2c7d 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) > git_config_set_in_file(p, "submodule.alternateErrorStrategy", > error_strategy); > > + git_config_set_in_file(p, "submodule.superprojectGitdir", > + relative_path(absolute_path(get_git_dir()), > + path, &sb)); > + I see that all new submodules will have this configuration set. But we will also live in a world where some existing submodules do not have this variable set. I'll look elsewhere for compatibility checks. > inspect() { > - dir=$1 && > - > - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > - { git -C "$dir" symbolic-ref HEAD || :; } >head && > - git -C "$dir" rev-parse HEAD >head-sha1 && > - git -C "$dir" update-index --refresh && > - git -C "$dir" diff-files --exit-code && > - git -C "$dir" clean -n -d -x >untracked > + sub_dir=$1 && > + super_dir=$2 && > + > + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && > + git -C "$sub_dir" rev-parse HEAD >head-sha1 && > + git -C "$sub_dir" update-index --refresh && > + git -C "$sub_dir" diff-files --exit-code && > + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && > + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ > + -ef "$sub_dir/$cached_super_dir" ] && > + git -C "$sub_dir" clean -n -d -x >untracked You rewrote this test in the previous patch, and now every line is changed because you renamed 'dir' to 'sub_dir'. Could the previous patch use 'sub_dir' from the start so this change only shows the new lines instead of many edited lines? > } > > test_expect_success 'submodule add' ' > @@ -138,7 +142,7 @@ test_expect_success 'submodule add' ' > ) && > > rm -f heads head untracked && > - inspect addtest/submod && > + inspect addtest/submod addtest && Similarly, I would not be upset to see these lines be changed just the once, even if the second argument is ignored for a single commit. But this nitpick is definitely less important since I could see taste swaying things either way. Thanks, -Stolee ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record 2021-08-20 0:38 ` Derrick Stolee @ 2021-10-13 19:36 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-13 19:36 UTC (permalink / raw) To: Derrick Stolee; +Cc: git, Junio C Hamano On Thu, Aug 19, 2021 at 08:38:19PM -0400, Derrick Stolee wrote: > > On 8/19/2021 4:09 PM, Emily Shaffer wrote: > ... > > +submodule.superprojectGitDir:: > > + The relative path from the submodule's worktree to its superproject's > > + gitdir. When Git is run in a repository, it usually makes no difference > > + whether this repository is standalone or a submodule, but if this > > + configuration variable is present, additional behavior may be possible, > > + such as "git status" printing additional information about this > > + submodule's status with respect to its superproject. This config should > > + only be present in projects which are submodules, but is not guaranteed > > + to be present in every submodule, so only optional value-added behavior > > + should be linked to it. It is set automatically during > > + submodule creation. > > ++ > > + Because of this configuration variable, it is forbidden to use the > > + same submodule worktree shared by multiple superprojects. > > nit: this paragraph linked with the "+" line should have no tabbing. Done. > > Also, could we use the same submodule worktree for multiple superprojects > _before_ this configuration variable? That seems wild to me. Or, is that > not a new requirement? I guess it'd be possible to do something pretty evil with symlinks? I'm not sure why you would want to, though. But now that I think about it more, I'm not sure that it would work, at least if we understand submodule to mean "...and the gitdir lives in .git/modules/ of the superproject". If superA contains sub and superB contains a symlink to 'sub''s worktree in superA, then wouldn't superA and superB both be trying to contain their own gitdirs for sub? And having multiple gitdirs for a worktree is an unacceptable state anyway. Or maybe the issue is more like: you have super, which contains sub, and you have super-wt, which is a worktree of super; to avoid duplicating sub, you decided to use a symlink. So there's only one sub gitdir, and only one super gitdir. It's a little awkward, but since submodule worktrees aren't currently supported, I can see the appeal. In this configuration, a path from submodule *worktree* to superproject gitdir, which is what v3 and earlier propose, would be broken in one superproject worktree or the other And having multiple gitdirs for a worktree is an unacceptable state anyway. Or maybe the issue is more like: you have super, which contains sub, and you have super-wt, which is a worktree of super; to avoid duplicating sub, you decided to use a symlink. So there's only one sub gitdir, and only one super gitdir. It's a little awkward, but since submodule worktrees aren't currently supported, I can see the appeal. In this configuration, a path from submodule *worktree* to superproject gitdir, which is what v3 and earlier propose, would be broken in one superproject worktree or the other. But as I'm proposing in v4, folks in the review club pointed out to me that a pointer from gitdir to gitdir makes more sense - and that would fix this concern, too, because sub and the symlink of sub would both share a single gitdir, and that gitdir would point to the single gitdir of super and super-wt. All a long way to say: I think v4 will fix it by originating the relative path from submodule gitdir, instead. And I will remove the extra paragraph - I think it is just adding confusion around a case that nobody would really want to use... > > Perhaps you mean something like this instead: > > It is forbidden to use the same submodule worktree for multiple > superprojects, so this configuration variable stores the unique > superproject and is not multi-valued. > > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > > index d55f6262e9..d60fcd2c7d 100644 > > --- a/builtin/submodule--helper.c > > +++ b/builtin/submodule--helper.c > > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) > > git_config_set_in_file(p, "submodule.alternateErrorStrategy", > > error_strategy); > > > > + git_config_set_in_file(p, "submodule.superprojectGitdir", > > + relative_path(absolute_path(get_git_dir()), > > + path, &sb)); > > + > > I see that all new submodules will have this configuration set. But we will > also live in a world where some existing submodules do not have this variable > set. I'll look elsewhere for compatibility checks. Yep, the series intended to add them piecemeal where possible, over the course of a handful of commits. > > > inspect() { > > - dir=$1 && > > - > > - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > > - { git -C "$dir" symbolic-ref HEAD || :; } >head && > > - git -C "$dir" rev-parse HEAD >head-sha1 && > > - git -C "$dir" update-index --refresh && > > - git -C "$dir" diff-files --exit-code && > > - git -C "$dir" clean -n -d -x >untracked > > + sub_dir=$1 && > > + super_dir=$2 && > > + > > + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > > + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && > > + git -C "$sub_dir" rev-parse HEAD >head-sha1 && > > + git -C "$sub_dir" update-index --refresh && > > + git -C "$sub_dir" diff-files --exit-code && > > + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && > > + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ > > + -ef "$sub_dir/$cached_super_dir" ] && > > + git -C "$sub_dir" clean -n -d -x >untracked > > You rewrote this test in the previous patch, and now every line is changed > because you renamed 'dir' to 'sub_dir'. Could the previous patch use > 'sub_dir' from the start so this change only shows the new lines instead of > many edited lines? Sure. > > > } > > > > test_expect_success 'submodule add' ' > > @@ -138,7 +142,7 @@ test_expect_success 'submodule add' ' > > ) && > > > > rm -f heads head untracked && > > - inspect addtest/submod && > > + inspect addtest/submod addtest && > > Similarly, I would not be upset to see these lines be changed just the > once, even if the second argument is ignored for a single commit. But > this nitpick is definitely less important since I could see taste > swaying things either way. I feel less interested in that nit; I think a mechanical "strip the useless arg" change + a mechanical "add an unrelated useful arg" change is easier to review than doing both at once. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record 2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer 2021-08-20 0:38 ` Derrick Stolee @ 2021-09-04 17:20 ` Matheus Tavares 2021-10-13 19:39 ` Emily Shaffer 1 sibling, 1 reply; 51+ messages in thread From: Matheus Tavares @ 2021-09-04 17:20 UTC (permalink / raw) To: emilyshaffer; +Cc: git, gitster Emily Shaffer <emilyshaffer@google.com> wrote: > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d55f6262e9..d60fcd2c7d 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) > git_config_set_in_file(p, "submodule.alternateErrorStrategy", > error_strategy); > > + git_config_set_in_file(p, "submodule.superprojectGitdir", > + relative_path(absolute_path(get_git_dir()), > + path, &sb)); This will be executed when cloning a submodule with `git submodule add <url/path> <path>`. Do we also want to set submodule.superprojectGitdir when adding a repository that already exists in the working tree as a submodule? I.e., something like: git init super git init super/sub [ make commits in super/sub ] git -C super submodule add ./sub I don't know if this workflow is so commonly used, though... It may not be worth the additional work. Another option, which I believe was suggested by Jonathan Nieder on the Review Club, is to change the code to absorb the gitdir when adding the local submodule. Then, the configuration would already be set by the `absorb_git_dir...()` function itself. > free(sm_alternate); > free(error_strategy); > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 4bc6b6c886..e407329d81 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' ' > submodurl=$(pwd -P) > > inspect() { > - dir=$1 && > - > - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > - { git -C "$dir" symbolic-ref HEAD || :; } >head && > - git -C "$dir" rev-parse HEAD >head-sha1 && > - git -C "$dir" update-index --refresh && > - git -C "$dir" diff-files --exit-code && > - git -C "$dir" clean -n -d -x >untracked > + sub_dir=$1 && > + super_dir=$2 && > + > + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && > + git -C "$sub_dir" rev-parse HEAD >head-sha1 && > + git -C "$sub_dir" update-index --refresh && > + git -C "$sub_dir" diff-files --exit-code && > + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && > + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ > + -ef "$sub_dir/$cached_super_dir" ] && To avoid the non-POSIX `-ef`, we could perhaps do something like: super_gitdir="$(git -C "$super_dir" rev-parse --absolute-git-dir)" && cached_gitdir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && test "$cached_gitdir" = "$(test-tool path-utils relative_path "$super_gitdir" "$PWD/$sub_dir")" && (We need the "$PWD/" at the last command because `path.c:relative_path()` returns the first argument as-is when one of the two paths given to it is absolute and the other is not.) One bonus of testing the cached path this way is that we also check that it is indeed being stored as a relative path :) ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record 2021-09-04 17:20 ` Matheus Tavares @ 2021-10-13 19:39 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-13 19:39 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, gitster On Sat, Sep 04, 2021 at 02:20:51PM -0300, Matheus Tavares wrote: > > Emily Shaffer <emilyshaffer@google.com> wrote: > > > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > > index d55f6262e9..d60fcd2c7d 100644 > > --- a/builtin/submodule--helper.c > > +++ b/builtin/submodule--helper.c > > @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix) > > git_config_set_in_file(p, "submodule.alternateErrorStrategy", > > error_strategy); > > > > + git_config_set_in_file(p, "submodule.superprojectGitdir", > > + relative_path(absolute_path(get_git_dir()), > > + path, &sb)); > > This will be executed when cloning a submodule with > `git submodule add <url/path> <path>`. Do we also want to set > submodule.superprojectGitdir when adding a repository that already exists in > the working tree as a submodule? I.e., something like: > > git init super > git init super/sub > [ make commits in super/sub ] > git -C super submodule add ./sub > > I don't know if this workflow is so commonly used, though... It may not be > worth the additional work. Yeah, I think it is covered in the next patch with 'git submodule absorbgitdirs'. > > Another option, which I believe was suggested by Jonathan Nieder on the Review > Club, is to change the code to absorb the gitdir when adding the local > submodule. Then, the configuration would already be set by the > `absorb_git_dir...()` function itself. > > > free(sm_alternate); > > free(error_strategy); > > > > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > > index 4bc6b6c886..e407329d81 100755 > > --- a/t/t7400-submodule-basic.sh > > +++ b/t/t7400-submodule-basic.sh > > @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' ' > > submodurl=$(pwd -P) > > > > inspect() { > > - dir=$1 && > > - > > - git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > > - { git -C "$dir" symbolic-ref HEAD || :; } >head && > > - git -C "$dir" rev-parse HEAD >head-sha1 && > > - git -C "$dir" update-index --refresh && > > - git -C "$dir" diff-files --exit-code && > > - git -C "$dir" clean -n -d -x >untracked > > + sub_dir=$1 && > > + super_dir=$2 && > > + > > + git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads && > > + { git -C "$sub_dir" symbolic-ref HEAD || :; } >head && > > + git -C "$sub_dir" rev-parse HEAD >head-sha1 && > > + git -C "$sub_dir" update-index --refresh && > > + git -C "$sub_dir" diff-files --exit-code && > > + cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && > > + [ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \ > > + -ef "$sub_dir/$cached_super_dir" ] && > > To avoid the non-POSIX `-ef`, we could perhaps do something like: > > super_gitdir="$(git -C "$super_dir" rev-parse --absolute-git-dir)" && > cached_gitdir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" && > test "$cached_gitdir" = "$(test-tool path-utils relative_path "$super_gitdir" "$PWD/$sub_dir")" && > > (We need the "$PWD/" at the last command because `path.c:relative_path()` > returns the first argument as-is when one of the two paths given to it is > absolute and the other is not.) > > One bonus of testing the cached path this way is that we also check that > it is indeed being stored as a relative path :) Yep, that is what I settled on. Thanks. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer @ 2021-08-19 20:09 ` Emily Shaffer 2021-08-20 0:50 ` Derrick Stolee 2021-09-04 17:27 ` Matheus Tavares 2021-08-19 20:09 ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer ` (3 subsequent siblings) 6 siblings, 2 replies; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 20:09 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Already during 'git submodule add' we record a pointer to the superproject's gitdir. However, this doesn't help brand-new submodules created with 'git init' and later absorbed with 'git submodule absorbgitdir'. Let's start adding that pointer during 'git submodule absorbgitdir' too. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- submodule.c | 10 ++++++++++ t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 0b1d9c1dde..4b314bf09c 100644 --- a/submodule.c +++ b/submodule.c @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path) char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; char *new_git_dir; const struct submodule *sub; + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; if (submodule_uses_worktrees(path)) die(_("relocate_gitdir for submodule '%s' with " @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path) relocate_gitdir(path, real_old_git_dir, real_new_git_dir); + /* cache pointer to superproject's gitdir */ + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ + strbuf_addf(&config_path, "%s/config", real_new_git_dir); + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", + relative_path(get_super_prefix_or_empty(), + path, &sb)); + + strbuf_release(&config_path); + strbuf_release(&sb); free(old_git_dir); free(real_old_git_dir); free(real_new_git_dir); diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 1cfa150768..e2d78e01df 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' ' git status >actual.1 && git -C sub1 rev-parse HEAD >actual.2 && test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + # make sure the submodule cached the superproject gitdir correctly + test-tool path-utils real_path . >expect && + test-tool path-utils real_path \ + "$(git -C sub1 config submodule.superprojectGitDir)" >actual && + + test_cmp expect actual ' test_expect_success 'absorbing does not fail for deinitialized submodules' ' -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs 2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer @ 2021-08-20 0:50 ` Derrick Stolee 2021-10-13 19:42 ` Emily Shaffer 2021-09-04 17:27 ` Matheus Tavares 1 sibling, 1 reply; 51+ messages in thread From: Derrick Stolee @ 2021-08-20 0:50 UTC (permalink / raw) To: Emily Shaffer, git On 8/19/2021 4:09 PM, Emily Shaffer wrote: > + /* cache pointer to superproject's gitdir */ > + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); Regarding this NEEDSWORK: you are specifying the config file that covers every worktree of this submodule, which seems to make sense to me. The submodule shouldn't have worktrees to itself, right? It might become a worktree if its superproject has a worktree. But also: it's still correct to write to the local, non-worktree config file even if experimental.worktreeConfig is set. That config just means "we need to read the worktree config as well as the local config". Or, am I misunderstanding something? (I'm advocating for the removal of the NEEDSWORK, in case that wasn't clear.) > + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > + relative_path(get_super_prefix_or_empty(), > + path, &sb)); Thanks, -Stolee ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs 2021-08-20 0:50 ` Derrick Stolee @ 2021-10-13 19:42 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-13 19:42 UTC (permalink / raw) To: Derrick Stolee; +Cc: git On Thu, Aug 19, 2021 at 08:50:27PM -0400, Derrick Stolee wrote: > > On 8/19/2021 4:09 PM, Emily Shaffer wrote: > > + /* cache pointer to superproject's gitdir */ > > + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ > > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > > Regarding this NEEDSWORK: you are specifying the config file that > covers every worktree of this submodule, which seems to make sense > to me. The submodule shouldn't have worktrees to itself, right? It > might become a worktree if its superproject has a worktree. > > But also: it's still correct to write to the local, non-worktree > config file even if experimental.worktreeConfig is set. That > config just means "we need to read the worktree config as well > as the local config". > > Or, am I misunderstanding something? > > (I'm advocating for the removal of the NEEDSWORK, in case that > wasn't clear.) Yeah, I think by pointing from submodule gitdir to superproject gitdir this point goes away, anyways. Regardless of which worktree is in use, the gitdir->gitdir pointer will still be valid. I'll delete the NEEDSWORK. > > > + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > > + relative_path(get_super_prefix_or_empty(), > > + path, &sb)); > > Thanks, > -Stolee > ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs 2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer 2021-08-20 0:50 ` Derrick Stolee @ 2021-09-04 17:27 ` Matheus Tavares 2021-10-14 18:40 ` Emily Shaffer 1 sibling, 1 reply; 51+ messages in thread From: Matheus Tavares @ 2021-09-04 17:27 UTC (permalink / raw) To: emilyshaffer; +Cc: git Emily Shaffer <emilyshaffer@google.com> wrote: > > Already during 'git submodule add' we record a pointer to the > superproject's gitdir. However, this doesn't help brand-new > submodules created with 'git init' and later absorbed with 'git > submodule absorbgitdir'. Let's start adding that pointer during 'git > submodule absorbgitdir' too. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > submodule.c | 10 ++++++++++ > t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/submodule.c b/submodule.c > index 0b1d9c1dde..4b314bf09c 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path) This function has only one caller, `absorb_git_dir_into_superproject()`. Perhaps it would be better to set submodule.superprojectGitdir in the caller, right after the `else` block in which `relocate...()` is called. This way, the config would also be set in the case of nested submodules where the inner one already had its gitdir absorbed (which is the case handled by the code in the `if` block). > char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; > char *new_git_dir; > const struct submodule *sub; > + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; > > if (submodule_uses_worktrees(path)) > die(_("relocate_gitdir for submodule '%s' with " > @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path) > > relocate_gitdir(path, real_old_git_dir, real_new_git_dir); > > + /* cache pointer to superproject's gitdir */ > + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ s/experimental/extensions/ On the Review Club, I mentioned we might want to save submodule.superprojectGitdir at the worktree config file. But Jonathan and Josh gave a better suggestion, which is to cache the superproject gitdir relative to the submodule's gitdir, instead of its working tree. This way, the worktree config wouldn't be an issue. And more importantly, this would prevent `git mv` from making the cached path stale, as Stolee pointed out upthread. > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > + relative_path(get_super_prefix_or_empty(), > + path, &sb)); In this code, `the_repository` corresponds to the superproject, right? I think `get_super_prefix_or_empty()` should instead be `absolute_path(get_git_dir())`, like you did on the previous patch. And since the first argument to `relative_path()` will be an absolute path, I believe we also need to convert the second one to an absolute path. Otherwise, `relative_path()` would return the first argument as-is [1]. (I played around with using `get_git_dir()` directly as the first argument, but it seems this can sometimes already be absolute, in case of nested submodules.) If we store the path as relative to the submodule's gitdir, it should be simpler, as `real_new_git_dir` is already absolute: git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", relative_path(absolute_path(get_git_dir()) real_new_git_dir, &sb)); [1]: I'm not sure if this is intended or if it's a bug. I was expecting that, before comparing its two arguments, `relative_path()` would convert any relative path given as argument to absolute, using the current working dir path. But that's not the case. > + strbuf_release(&config_path); > + strbuf_release(&sb); > free(old_git_dir); > free(real_old_git_dir); > free(real_new_git_dir); > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > index 1cfa150768..e2d78e01df 100755 > --- a/t/t7412-submodule-absorbgitdirs.sh > +++ b/t/t7412-submodule-absorbgitdirs.sh > @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' ' > git status >actual.1 && > git -C sub1 rev-parse HEAD >actual.2 && > test_cmp expect.1 actual.1 && > - test_cmp expect.2 actual.2 > + test_cmp expect.2 actual.2 && > + > + # make sure the submodule cached the superproject gitdir correctly > + test-tool path-utils real_path . >expect && This should be '.git' instead of '.', since the config caches the path to the superproject's gitdir. But ... > + test-tool path-utils real_path \ > + "$(git -C sub1 config submodule.superprojectGitDir)" >actual && ... I think we could also avoid converting to an absolute path here, so that we can test whether the setting is really caching a relative path. I.e., the test could be: super_gitdir="$(git rev-parse --absolute-git-dir)" && test-tool path-utils relative_path "$super_gitdir" "$PWD/sub1" >expect && git -C sub1 config submodule.superprojectGitDir >actual && test_cmp expect actual > + > + test_cmp expect actual > ' It may also be interesting to test if the config is correctly set when absorbing the gitdir of nested submodules: diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index e2d78e01df..c2e5e7dd1c 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -70,3 +70,8 @@ test_expect_success 'absorb the git dir in a nested submodule' ' test_cmp expect.1 actual.1 && - test_cmp expect.2 actual.2 + test_cmp expect.2 actual.2 && + + sub1_gitdir="$(git -C sub1 rev-parse --absolute-git-dir)" && + test-tool path-utils relative_path "$sub1_gitdir" "$PWD/sub1/nested" >expect && + git -C sub1/nested config submodule.superprojectGitDir >actual && + test_cmp expect actual ' --- >8 --- ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs 2021-09-04 17:27 ` Matheus Tavares @ 2021-10-14 18:40 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-14 18:40 UTC (permalink / raw) To: Matheus Tavares; +Cc: git On Sat, Sep 04, 2021 at 02:27:46PM -0300, Matheus Tavares wrote: > > Emily Shaffer <emilyshaffer@google.com> wrote: > > > > Already during 'git submodule add' we record a pointer to the > > superproject's gitdir. However, this doesn't help brand-new > > submodules created with 'git init' and later absorbed with 'git > > submodule absorbgitdir'. Let's start adding that pointer during 'git > > submodule absorbgitdir' too. > > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > submodule.c | 10 ++++++++++ > > t/t7412-submodule-absorbgitdirs.sh | 9 ++++++++- > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/submodule.c b/submodule.c > > index 0b1d9c1dde..4b314bf09c 100644 > > --- a/submodule.c > > +++ b/submodule.c > > @@ -2065,6 +2065,7 @@ static void relocate_single_git_dir_into_superproject(const char *path) > > This function has only one caller, `absorb_git_dir_into_superproject()`. > Perhaps it would be better to set submodule.superprojectGitdir in the caller, > right after the `else` block in which `relocate...()` is called. This way, > the config would also be set in the case of nested submodules where the inner > one already had its gitdir absorbed (which is the case handled by the code in > the `if` block). Since relocate_single_git_dir() is the only place where the final submodule gitdir is resolved, I'll leave the code block where it is; neither side of the if/else in absorb_git_dir...() learns the final "new" gitdir, so I'd rather not re-derive it. Anyway, wouldn't the submodule-who-is-also-a-superproject have gone through this same codepath itself? I'll see if I can find a test to validate that. > > > char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL; > > char *new_git_dir; > > const struct submodule *sub; > > + struct strbuf config_path = STRBUF_INIT, sb = STRBUF_INIT; > > > > if (submodule_uses_worktrees(path)) > > die(_("relocate_gitdir for submodule '%s' with " > > @@ -2096,6 +2097,15 @@ static void relocate_single_git_dir_into_superproject(const char *path) > > > > relocate_gitdir(path, real_old_git_dir, real_new_git_dir); > > > > + /* cache pointer to superproject's gitdir */ > > + /* NEEDSWORK: this may differ if experimental.worktreeConfig is enabled */ > > s/experimental/extensions/ > > On the Review Club, I mentioned we might want to save > submodule.superprojectGitdir at the worktree config file. But Jonathan and Josh > gave a better suggestion, which is to cache the superproject gitdir relative to > the submodule's gitdir, instead of its working tree. > > This way, the worktree config wouldn't be an issue. And more importantly, this > would prevent `git mv` from making the cached path stale, as Stolee pointed out > upthread. Yep, done. Thanks. > > > + strbuf_addf(&config_path, "%s/config", real_new_git_dir); > > + git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > > + relative_path(get_super_prefix_or_empty(), > > + path, &sb)); > > > In this code, `the_repository` corresponds to the superproject, right? I > think `get_super_prefix_or_empty()` should instead be > `absolute_path(get_git_dir())`, like you did on the previous patch. > > And since the first argument to `relative_path()` will be an absolute path, I > believe we also need to convert the second one to an absolute path. Otherwise, > `relative_path()` would return the first argument as-is [1]. (I played around > with using `get_git_dir()` directly as the first argument, but it seems this > can sometimes already be absolute, in case of nested submodules.) > > If we store the path as relative to the submodule's gitdir, it should be > simpler, as `real_new_git_dir` is already absolute: > > git_config_set_in_file(config_path.buf, "submodule.superprojectGitdir", > relative_path(absolute_path(get_git_dir()) > real_new_git_dir, &sb)); > > [1]: I'm not sure if this is intended or if it's a bug. I was expecting that, > before comparing its two arguments, `relative_path()` would convert any > relative path given as argument to absolute, using the current working dir path. > But that's not the case. Thanks, fixed. > > > + strbuf_release(&config_path); > > + strbuf_release(&sb); > > free(old_git_dir); > > free(real_old_git_dir); > > free(real_new_git_dir); > > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > > index 1cfa150768..e2d78e01df 100755 > > --- a/t/t7412-submodule-absorbgitdirs.sh > > +++ b/t/t7412-submodule-absorbgitdirs.sh > > @@ -30,7 +30,14 @@ test_expect_success 'absorb the git dir' ' > > git status >actual.1 && > > git -C sub1 rev-parse HEAD >actual.2 && > > test_cmp expect.1 actual.1 && > > - test_cmp expect.2 actual.2 > > + test_cmp expect.2 actual.2 && > > + > > + # make sure the submodule cached the superproject gitdir correctly > > + test-tool path-utils real_path . >expect && > > This should be '.git' instead of '.', since the config caches the path to the > superproject's gitdir. But ... > > > + test-tool path-utils real_path \ > > + "$(git -C sub1 config submodule.superprojectGitDir)" >actual && > > ... I think we could also avoid converting to an absolute path here, so that we > can test whether the setting is really caching a relative path. I.e., the test > could be: > > super_gitdir="$(git rev-parse --absolute-git-dir)" && > test-tool path-utils relative_path "$super_gitdir" "$PWD/sub1" >expect && > git -C sub1 config submodule.superprojectGitDir >actual && > test_cmp expect actual Sure. > > > + > > + test_cmp expect actual > > ' > > It may also be interesting to test if the config is correctly set when > absorbing the gitdir of nested submodules: > > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > index e2d78e01df..c2e5e7dd1c 100755 > --- a/t/t7412-submodule-absorbgitdirs.sh > +++ b/t/t7412-submodule-absorbgitdirs.sh > @@ -70,3 +70,8 @@ test_expect_success 'absorb the git dir in a nested submodule' ' > test_cmp expect.1 actual.1 && > - test_cmp expect.2 actual.2 > + test_cmp expect.2 actual.2 && > + > + sub1_gitdir="$(git -C sub1 rev-parse --absolute-git-dir)" && > + test-tool path-utils relative_path "$sub1_gitdir" "$PWD/sub1/nested" >expect && > + git -C sub1/nested config submodule.superprojectGitDir >actual && > + test_cmp expect actual Ah, thanks. I tried it out, even without the changes you mentioned above, and this test passes. So I think as I expected, it works because 'sub1' goes through relocate_single_git_dir() as well as 'sub1/nested'. Thanks for the careful review. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v3 4/4] submodule: record superproject gitdir during 'update' 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (2 preceding siblings ...) 2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer @ 2021-08-19 20:09 ` Emily Shaffer 2021-08-20 0:59 ` Derrick Stolee 2021-08-19 21:56 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Junio C Hamano ` (2 subsequent siblings) 6 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-08-19 20:09 UTC (permalink / raw) To: git; +Cc: Emily Shaffer A recorded hint path to the superproject's gitdir might be added during 'git submodule add', but in some cases - like submodules which were created before 'git submodule add' learned to record that info - it might be useful to update the hint. Let's do it during 'git submodule update', when we already have a handle to the superproject while calling operations on the submodules. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- git-submodule.sh | 10 ++++++++++ t/t7406-submodule-update.sh | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/git-submodule.sh b/git-submodule.sh index 4678378424..f98dcc16ae 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -648,6 +648,16 @@ cmd_update() fi fi + # Cache a pointer to the superproject's gitdir. This may have + # changed, so rewrite it unconditionally. Writes it to worktree + # if applicable, otherwise to local. + relative_gitdir="$(git rev-parse --path-format=relative \ + --prefix "${sm_path}" \ + --git-dir)" + + git -C "$sm_path" config --worktree \ + submodule.superprojectgitdir "$relative_gitdir" + if test -n "$recursive" then ( diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index f4f61fe554..c39821ba8e 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1061,4 +1061,14 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s ) ' +test_expect_success 'submodule update adds superproject gitdir to older repos' ' + (cd super && + git -C submodule config --unset submodule.superprojectGitdir && + git submodule update && + echo "../.git" >expect && + git -C submodule config submodule.superprojectGitdir >actual && + test_cmp expect actual + ) +' + test_done -- 2.33.0.rc2.250.ged5fa647cd-goog ^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update' 2021-08-19 20:09 ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer @ 2021-08-20 0:59 ` Derrick Stolee 2021-10-14 18:45 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Derrick Stolee @ 2021-08-20 0:59 UTC (permalink / raw) To: Emily Shaffer, git On 8/19/2021 4:09 PM, Emily Shaffer wrote: > + # Cache a pointer to the superproject's gitdir. This may have > + # changed, so rewrite it unconditionally. Writes it to worktree > + # if applicable, otherwise to local. > + relative_gitdir="$(git rev-parse --path-format=relative \ > + --prefix "${sm_path}" \ > + --git-dir)" > + > + git -C "$sm_path" config --worktree \> + submodule.superprojectgitdir "$relative_gitdir" Ok, I see now why you care about the worktree config. The scenario you are covering is something like moving a submodule within a worktree and not wanting to change the relative path of other copies of that submodule within other worktrees, yes? For commands such as 'init' and 'add', we don't have the possibility of colliding with other worktrees because the submodule is being created for the first time, so the relative path should be safe to place in the non-worktree config. I do struggle with the fact that these are inconsistent across the two commits. It makes me think that there should only be one way to do it, and either the NEEDSWORK needs to be fixed now, or this line shouldn't include --worktree. Much of this can depend on the reason the worktree config exists for a submodule. I expect you have more context than me, so could you help me understand? Moving to a different concern I am now realizing with this config: What happens if a submodule moves, and then a user runs 'git checkout' in the superproject to move it back? How do we make this config value update across those movements? Is there a possibility of integrating with unpack_trees() to notice that a submodule has moved and hence we should update this config value? Thanks, -Stoolee ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 4/4] submodule: record superproject gitdir during 'update' 2021-08-20 0:59 ` Derrick Stolee @ 2021-10-14 18:45 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-14 18:45 UTC (permalink / raw) To: Derrick Stolee; +Cc: git On Thu, Aug 19, 2021 at 08:59:55PM -0400, Derrick Stolee wrote: > > On 8/19/2021 4:09 PM, Emily Shaffer wrote: > > + # Cache a pointer to the superproject's gitdir. This may have > > + # changed, so rewrite it unconditionally. Writes it to worktree > > + # if applicable, otherwise to local. > > + relative_gitdir="$(git rev-parse --path-format=relative \ > > + --prefix "${sm_path}" \ > > + --git-dir)" > > + > > + git -C "$sm_path" config --worktree \> + submodule.superprojectgitdir "$relative_gitdir" > > Ok, I see now why you care about the worktree config. The scenario you > are covering is something like moving a submodule within a worktree and > not wanting to change the relative path of other copies of that submodule > within other worktrees, yes? > > For commands such as 'init' and 'add', we don't have the possibility of > colliding with other worktrees because the submodule is being created > for the first time, so the relative path should be safe to place in the > non-worktree config. > > I do struggle with the fact that these are inconsistent across the > two commits. It makes me think that there should only be one way to > do it, and either the NEEDSWORK needs to be fixed now, or this line > shouldn't include --worktree. Much of this can depend on the reason > the worktree config exists for a submodule. I expect you have more > context than me, so could you help me understand? > > Moving to a different concern I am now realizing with this config: > What happens if a submodule moves, and then a user runs 'git checkout' > in the superproject to move it back? How do we make this config value > update across those movements? Is there a possibility of integrating > with unpack_trees() to notice that a submodule has moved and hence we > should update this config value? I think that switching from "sub worktree to super gitdir" to "sub gitdir to super gitdir" fixes this neatly - the gitdir-to-gitdir path will be identical regardless of worktree, so we can set it in the main submodule config (lose the '--worktree' arg). This also will fix the case you describe, moving around the submodule in the superproject's tree from checkout to checkout, as the gitdir will not move. Thanks a bunch for the review and sorry it took me so long to reply. v4 incoming. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (3 preceding siblings ...) 2021-08-19 20:09 ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer @ 2021-08-19 21:56 ` Junio C Hamano 2021-08-20 1:09 ` Derrick Stolee 2021-09-04 17:50 ` Matheus Tavares Bernardino 6 siblings, 0 replies; 51+ messages in thread From: Junio C Hamano @ 2021-08-19 21:56 UTC (permalink / raw) To: Emily Shaffer Cc: git, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan Emily Shaffer <emilyshaffer@google.com> writes: > Since v2, mostly documentation changes and a handful of small nits from > Junio and Jonathan Tan. Thanks for the reviews, both. Will queue, but ... > +submodule.superprojectGitDir:: > -+ The relative path from the submodule's worktree to the superproject's > -+ gitdir. This config should only be present in projects which are > -+ submodules, but is not guaranteed to be present in every submodule. It > -+ is set automatically during submodule creation. > ++ The relative path from the submodule's worktree to its superproject's > ++ gitdir. When Git is run in a repository, it usually makes no difference > ++ whether this repository is standalone or a submodule, but if this > ++ configuration variable is present, additional behavior may be possible, > ++ such as "git status" printing additional information about this > ++ submodule's status with respect to its superproject. This config should > ++ only be present in projects which are submodules, but is not guaranteed > ++ to be present in every submodule, so only optional value-added behavior > ++ should be linked to it. It is set automatically during > ++ submodule creation. > ++ > -+ In situations where more than one superproject references the same > -+ submodule worktree, the value of this config and the behavior of > -+ operations which use it are undefined. To reference a single project > -+ from multiple superprojects, it is better to create a worktree of the > -+ submodule for each superproject. > ++ Because of this configuration variable, it is forbidden to use the > ++ same submodule worktree shared by multiple superprojects. ... I think this will regress the format from what I've queued in 'seen' unless you dedent the "Because of this..." paragraph. It is unfortunate but AsciiDoc wants the follow-up paragraphs that follow the single '+' line to start at the left edge without indentation. ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (4 preceding siblings ...) 2021-08-19 21:56 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Junio C Hamano @ 2021-08-20 1:09 ` Derrick Stolee 2021-10-13 18:51 ` Emily Shaffer 2021-09-04 17:50 ` Matheus Tavares Bernardino 6 siblings, 1 reply; 51+ messages in thread From: Derrick Stolee @ 2021-08-20 1:09 UTC (permalink / raw) To: Emily Shaffer, git Cc: Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan On 8/19/2021 4:09 PM, Emily Shaffer wrote: > Since v2, mostly documentation changes and a handful of small nits from > Junio and Jonathan Tan. Thanks for the reviews, both. Sorry to show up late with many questions (mostly because my understanding of submodules is not as strong as yours), but I do have some concerns that we have not covered all the cases that could lead to the relative path needing an update, such as a 'git checkout' across commits in the super- project which moves a submodule. Leading more to my confusion is that I thought 'git submodule update' was the way to move a submodule, but that does not appear to be the case. I used 'git mv' to move a submodule and that correctly updated the .gitmodules file, but did not run any 'git submodule' subcommands (based on GIT_TRACE2_PERF=1). You mention in an earlier cover letter that this config does not need to exist, but it seems to me that if it exists it needs to be correct for us to depend on it for future features. I'm not convinced that we can rely on it as-written, but you might be able to convince me by pointing out why these submodule movements are safe. Thanks, -Stolee ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-08-20 1:09 ` Derrick Stolee @ 2021-10-13 18:51 ` Emily Shaffer 2021-10-14 17:12 ` Derrick Stolee 0 siblings, 1 reply; 51+ messages in thread From: Emily Shaffer @ 2021-10-13 18:51 UTC (permalink / raw) To: Derrick Stolee Cc: git, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote: > > On 8/19/2021 4:09 PM, Emily Shaffer wrote: > > Since v2, mostly documentation changes and a handful of small nits from > > Junio and Jonathan Tan. Thanks for the reviews, both. > > Sorry to show up late with many questions (mostly because my understanding > of submodules is not as strong as yours), Well, here I am apologizing for showing up even later with a reply ;) > but I do have some concerns that > we have not covered all the cases that could lead to the relative path > needing an update, such as a 'git checkout' across commits in the super- > project which moves a submodule. > > Leading more to my confusion is that I thought 'git submodule update' > was the way to move a submodule, but that does not appear to be the case. > I used 'git mv' to move a submodule and that correctly updated the > .gitmodules file, but did not run any 'git submodule' subcommands (based > on GIT_TRACE2_PERF=1). During a live review a few weeks ago it was pointed out, I forget by who, that this whole series would make a lot more sense if it was treated as the path from the submodule's gitdir to the superproject's gitdir. I think this would also fix your 'git mv' example here, as the submodule gitdir would not change. > > You mention in an earlier cover letter that this config does not need to > exist, but it seems to me that if it exists it needs to be correct for us > to depend on it for future features. I'm not convinced that we can rely > on it as-written, but you might be able to convince me by pointing out > why these submodule movements are safe. Yeah, that is a good point, and I wonder how to be defensive about this... Maybe it makes sense to BUG() if we don't find the superproject's gitdir from this config? Maybe it makes sense to warn (asking users to 'git submodule update') and erase the incorrect config line? Both those approaches would require a wrapper around 'git_config_get_string()' to cope with a failure, but it might be worth it. I'll try proposing such a wrapper in the reroll, unless I hear back sooner. - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-10-13 18:51 ` Emily Shaffer @ 2021-10-14 17:12 ` Derrick Stolee 2021-10-14 18:52 ` Emily Shaffer 0 siblings, 1 reply; 51+ messages in thread From: Derrick Stolee @ 2021-10-14 17:12 UTC (permalink / raw) To: Emily Shaffer Cc: git, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan On 10/13/2021 2:51 PM, Emily Shaffer wrote: > On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote: >> but I do have some concerns that >> we have not covered all the cases that could lead to the relative path >> needing an update, such as a 'git checkout' across commits in the super- >> project which moves a submodule. >> >> Leading more to my confusion is that I thought 'git submodule update' >> was the way to move a submodule, but that does not appear to be the case. >> I used 'git mv' to move a submodule and that correctly updated the >> .gitmodules file, but did not run any 'git submodule' subcommands (based >> on GIT_TRACE2_PERF=1). > > During a live review a few weeks ago it was pointed out, I forget by > who, that this whole series would make a lot more sense if it was > treated as the path from the submodule's gitdir to the superproject's > gitdir. I think this would also fix your 'git mv' example here, as the > submodule gitdir would not change. I think that's a great way to deliver similar functionality without the issues I was thinking about. >> You mention in an earlier cover letter that this config does not need to >> exist, but it seems to me that if it exists it needs to be correct for us >> to depend on it for future features. I'm not convinced that we can rely >> on it as-written, but you might be able to convince me by pointing out >> why these submodule movements are safe. > > Yeah, that is a good point, and I wonder how to be defensive about > this... Maybe it makes sense to BUG() if we don't find the > superproject's gitdir from this config? Maybe it makes sense to warn > (asking users to 'git submodule update') and erase the incorrect config > line? I think we can complain with something like a die() if the config points to data that doesn't make sense (not a .git directory), but a BUG() is too heavy-handed because it can just be a user modifying their config file incorrectly. I'm happy to shut down a process because a user messed with config incorrectly. Since your proposed change allows operations like 'git mv' to move submodules without needing to change this config, I'm much happier with the design. It becomes impossible for the config to get out of sync unless the user does something outside of a Git command. Thanks, -Stolee ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-10-14 17:12 ` Derrick Stolee @ 2021-10-14 18:52 ` Emily Shaffer 0 siblings, 0 replies; 51+ messages in thread From: Emily Shaffer @ 2021-10-14 18:52 UTC (permalink / raw) To: Derrick Stolee Cc: git, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan On Thu, Oct 14, 2021 at 01:12:11PM -0400, Derrick Stolee wrote: > > On 10/13/2021 2:51 PM, Emily Shaffer wrote: > > On Thu, Aug 19, 2021 at 09:09:46PM -0400, Derrick Stolee wrote: > >> but I do have some concerns that > >> we have not covered all the cases that could lead to the relative path > >> needing an update, such as a 'git checkout' across commits in the super- > >> project which moves a submodule. > >> > >> Leading more to my confusion is that I thought 'git submodule update' > >> was the way to move a submodule, but that does not appear to be the case. > >> I used 'git mv' to move a submodule and that correctly updated the > >> .gitmodules file, but did not run any 'git submodule' subcommands (based > >> on GIT_TRACE2_PERF=1). > > > > During a live review a few weeks ago it was pointed out, I forget by > > who, that this whole series would make a lot more sense if it was > > treated as the path from the submodule's gitdir to the superproject's > > gitdir. I think this would also fix your 'git mv' example here, as the > > submodule gitdir would not change. > > I think that's a great way to deliver similar functionality without > the issues I was thinking about. > > >> You mention in an earlier cover letter that this config does not need to > >> exist, but it seems to me that if it exists it needs to be correct for us > >> to depend on it for future features. I'm not convinced that we can rely > >> on it as-written, but you might be able to convince me by pointing out > >> why these submodule movements are safe. > > > > Yeah, that is a good point, and I wonder how to be defensive about > > this... Maybe it makes sense to BUG() if we don't find the > > superproject's gitdir from this config? Maybe it makes sense to warn > > (asking users to 'git submodule update') and erase the incorrect config > > line? > > I think we can complain with something like a die() if the config points > to data that doesn't make sense (not a .git directory), but a BUG() is > too heavy-handed because it can just be a user modifying their config > file incorrectly. Ok. Having re-thought since the other day, I think I will skip the wrapper for this reroll and instead propose it in a series that actually uses this pointer (so, the shared submodule-and-superproject config), if it looks like we'd want one. I expect doing that work in the context of a caller will make the correct behavior a little more clear - in the context of this series I'm not totally sure what we'd want to do. > > I'm happy to shut down a process because a user messed with config > incorrectly. Since your proposed change allows operations like 'git mv' > to move submodules without needing to change this config, I'm much > happier with the design. It becomes impossible for the config to get > out of sync unless the user does something outside of a Git command. Thanks for pulling the context back up here. I appreciate it. Like I mentioned in reply to your comment on v4, look for a reroll in the next 30 minutes or so (assuming the CI passes ok). - Emily ^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v3 0/4] cache parent project's gitdir in submodules 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer ` (5 preceding siblings ...) 2021-08-20 1:09 ` Derrick Stolee @ 2021-09-04 17:50 ` Matheus Tavares Bernardino 6 siblings, 0 replies; 51+ messages in thread From: Matheus Tavares Bernardino @ 2021-09-04 17:50 UTC (permalink / raw) To: Emily Shaffer Cc: git, Albert Cui, Phillip Wood, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Junio C Hamano, Jonathan Nieder, Jacob Keller, Atharva Raykar, Jonathan Tan Hi, Emily On Thu, Aug 19, 2021 at 5:10 PM Emily Shaffer <emilyshaffer@google.com> wrote: > > Emily Shaffer (4): > t7400-submodule-basic: modernize inspect() helper > introduce submodule.superprojectGitDir record > submodule: record superproject gitdir during absorbgitdirs > submodule: record superproject gitdir during 'update' I left a few comments on patches 2 and 3. They are mostly about things we have already discussed on the Review Club, but I got the chance to experiment with the code a bit more after that, and I thought it could be helpful to provide these inline suggestions. Hope it helps, and thanks for the series! Thanks, Matheus ^ permalink raw reply [flat|nested] 51+ messages in thread
end of thread, other threads:[~2021-10-14 19:51 UTC | newest] Thread overview: 51+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-11 22:54 [RFC PATCH 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-06-14 4:52 ` Junio C Hamano 2021-06-11 22:54 ` [RFC PATCH 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer 2021-06-14 5:09 ` Junio C Hamano 2021-06-15 22:00 ` Emily Shaffer 2021-06-11 22:54 ` [RFC PATCH 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer 2021-06-14 6:18 ` Junio C Hamano 2021-06-11 22:54 ` [RFC PATCH 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer 2021-06-14 6:22 ` Junio C Hamano 2021-06-15 21:27 ` Emily Shaffer 2021-06-12 20:12 ` [RFC PATCH 0/4] cache parent project's gitdir in submodules Jacob Keller 2021-06-14 7:26 ` Junio C Hamano 2021-06-15 21:18 ` Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 " Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-07-27 17:12 ` Jonathan Tan 2021-08-19 17:46 ` Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 2/4] introduce submodule.superprojectGitDir cache Emily Shaffer 2021-06-16 4:40 ` Junio C Hamano 2021-06-16 4:43 ` Junio C Hamano 2021-06-18 0:03 ` Emily Shaffer 2021-06-18 0:00 ` Emily Shaffer 2021-07-27 17:46 ` Jonathan Tan 2021-08-19 17:53 ` Emily Shaffer 2021-10-14 19:25 ` Ævar Arnfjörð Bjarmason 2021-06-16 0:45 ` [PATCH v2 3/4] submodule: cache superproject gitdir during absorbgitdirs Emily Shaffer 2021-06-16 0:45 ` [PATCH v2 4/4] submodule: cache superproject gitdir during 'update' Emily Shaffer 2021-07-27 17:51 ` Jonathan Tan 2021-08-19 18:02 ` Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 1/4] t7400-submodule-basic: modernize inspect() helper Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 2/4] introduce submodule.superprojectGitDir record Emily Shaffer 2021-08-20 0:38 ` Derrick Stolee 2021-10-13 19:36 ` Emily Shaffer 2021-09-04 17:20 ` Matheus Tavares 2021-10-13 19:39 ` Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 3/4] submodule: record superproject gitdir during absorbgitdirs Emily Shaffer 2021-08-20 0:50 ` Derrick Stolee 2021-10-13 19:42 ` Emily Shaffer 2021-09-04 17:27 ` Matheus Tavares 2021-10-14 18:40 ` Emily Shaffer 2021-08-19 20:09 ` [PATCH v3 4/4] submodule: record superproject gitdir during 'update' Emily Shaffer 2021-08-20 0:59 ` Derrick Stolee 2021-10-14 18:45 ` Emily Shaffer 2021-08-19 21:56 ` [PATCH v3 0/4] cache parent project's gitdir in submodules Junio C Hamano 2021-08-20 1:09 ` Derrick Stolee 2021-10-13 18:51 ` Emily Shaffer 2021-10-14 17:12 ` Derrick Stolee 2021-10-14 18:52 ` Emily Shaffer 2021-09-04 17:50 ` Matheus Tavares Bernardino
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).