On Mon, Oct 18, 2021 at 04:18:18PM -0700, Jonathan Tan 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.
>
> s/absorbgitdir/absorbgitdirs/ (note the "s" at the end)
>
> > @@ -2114,6 +2115,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(absolute_path(get_git_dir()),
> > + real_new_git_dir, &sb));
> > +
> > + strbuf_release(&config_path);
> > + strbuf_release(&sb);
> > free(old_git_dir);
> > free(real_old_git_dir);
> > free(real_new_git_dir);
>
> Here [1] you mention that you'll delete the NEEDSWORK, but it's still
> there.
>
> Having said that, it might be better to make a test in which we call
> this command while in a worktree of a superproject. The test might
> reveal that (as pointed out to me internally) you might need to use the
> common dir functions instead of the git dir functions to point to the
> directory that you want (git-worktree.txt distinguishes the 2 if you
> search for GIT_COMMON_DIR).
Huh, something interesting happened, actually.
I wrote a little test:
test_expect_success 'absorbgitdirs works when called from a superproject worktree' '
# set up a worktree of the superproject
git worktree add wt &&
(
cd wt &&
# create a new unembedded git dir
git init sub4 &&
test_commit -C sub4 first &&
git submodule add ./sub4 &&
test_tick &&
# absorb the git dir
git submodule absorbgitdirs sub4 &&
# make sure the submodule cached the superproject gitdir correctly
submodule_gitdir="$(git -C sub4 rev-parse --absolute-git-dir)" &&
superproject_gitdir="$(git rev-parse --absolute-git-dir)" &&
test-tool path-utils relative_path "$superproject_gitdir" \
"$submodule_gitdir" >expect &&
git -C sub4 config submodule.superprojectGitDir >actual &&
test_pause &&
test_cmp expect actual
)
'
However, the `git submodule absorbgitdirs` command didn't do quite what
I expected.
When I made a new worktree, that worktree's gitdir showed up in
'$TEST_DIR/.git/worktrees/wt/'. That's not very surprising. But what did
surprise me was that when I called `git submodule absorbgitdirs sub4`,
sub4's gitdir ended up in '$TEST_DIR/.git/worktrees/wt/modules/sub4',
rather than in '$TEST_DIR/.git/modules/sub4'. That's a little
surprising!
Anyway, this test has a sort of pernicious mistake too - I'm checking
relative path between 'git rev-parse --absolute-git-dir's, but the
relative path from .git/worktrees/wt/ to .git/worktrees/wt/modules/sub4
is the same as the relative path from .git/ to .git/modules/sub4, so
this test actually passes.
I'll change the tests here and elsewhere to use 'git rev-parse
--path-format=absolute --git-common-dir', but I think that still leaves
a kind of dangerous state for people working a lot with worktrees and
submodules - if I like to make throwaway worktrees in my regular
workflow, and I create a new submodule in one, and then get rid of the
worktree when I'm done with the task that added the new submodule, I
think it will explode if I try to checkout the branch I was using in
that worktree.
I tried it out locally:
# setup
git init test && cd test
git commit --allow-empty -m "first commit"
git worktree add wt
(
cd wt
git init sub
git -C sub commit --allow-empty -m "first-commit"
git submodule add ./sub
git commit -m "add submodule (as initted)
git submodule absorbgitdirs sub
# This told me "Migrating git directory of 'sub' from
# test/wt/sub/.git to test/.git/worktrees/wt/modules/sub, oops
)
# Make it possible to checkout wt branch somewhere else
git -C wt checkout HEAD --detach
# Try and delete the worktree; presumably my work is done
git worktree remove wt
At this point, Git wouldn't let me remove the worktree:
fatal: working trees containing submodules cannot be moved or removed
But wouldn't it be better to just absorb the submodule into the common
dir instead...?
By the way, when I tried checking out 'wt' branch from the original
worktree without deleting the new worktree, and then running 'git
submodule update', Git cloned 'sub' from .git/worktrees/wt/modules/sub
into .git/modules/sub. That was pretty surprising too!
Anyway, all of that is kind of a tangent. The takeaways I got are
twofold:
1) Yes, use common dir, not gitdir, in all the cached
path-to-superproject stuff.
2) Someone (me?) should send a patch keeping the "you can't delete a
submodule with a gitdir in it" die(), but *also* changing the behavior
to put the new submodule gitdir into get_common_dir() instead of
get_git_dir().
I'll try to send (2) separately - I think it will be a pretty small change,
and by keeping around the die() for deleting a worktree that already has
a submodule gitdir in it, we won't be risking deleting anybody's
existing work.
- Emily
>
> Besides that, all 4 patches look good (including the description of the
> new config variable).
>
> [1] https://lore.kernel.org/git/YWc2iJ7FQJYCnQ7w@xxxxxxxxxx/
