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

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

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