Re: [PATCH v4 3/4] submodule: record superproject gitdir during absorbgitdirs

On Mon, Oct 18, 2021 at 04:18:18PM -0700, Jonathan Tan wrote:

quoted

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)

quoted

@@ -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@google.com/ (local)