Re: [PATCH] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Stefan Beller <sbeller@google.com>]

On Tue, Jan 24, 2017 at 1:58 PM, Brandon Williams <bmwill@google.com> wrote:
> On 01/24, Stefan Beller wrote:
>> +     if (read_gitfile_gently(old_git_dir, &err_code) ||
>> +         err_code == READ_GITFILE_ERR_NOT_A_REPO) {
>> +             /*
>> +              * If it is an actual gitfile, it doesn't need migration,
>> +              * however in case of a recursively nested submodule, the
>> +              * gitfile content may be stale, as its superproject
>> +              * (which may be a submodule of another superproject)
>> +              * may have been moved. So expect a bogus pointer to be read,
>> +              * which materializes as error READ_GITFILE_ERR_NOT_A_REPO.
>> +              */
>> +             connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> So connect_work_tree_and_git_dir() will update the .gitfile if it is
> stale.
>
>> +             return;
>> +     }
>> +
>> +     if (submodule_uses_worktrees(path))
>> +             die(_("relocate_gitdir for submodule '%s' with "
>> +                   "more than one worktree not supported"), path);
>
> No current support for worktrees (yet!).
>
>> +
>>       if (!prefix)
>>               prefix = get_super_prefix();
>>
>> @@ -1437,22 +1448,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
>>                                     const char *path,
>>                                     unsigned flags)
>>  {
>> -     const char *sub_git_dir, *v;
>> -     char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
>>       struct strbuf gitdir = STRBUF_INIT;
>> -
>>       strbuf_addf(&gitdir, "%s/.git", path);
>> -     sub_git_dir = resolve_gitdir(gitdir.buf);
>>
>>       /* Not populated? */
>> -     if (!sub_git_dir)
>> +     if (!file_exists(gitdir.buf))
>>               goto out;
>
> There should be a is_submodule_populated() function now, maybe
> we should start using it when performing population checks?

Yes I am aware of that, but the problem is we cannot use it here.
is_submodule_populated[1], just like the code here, uses
resolve_gitdir, which is

    const char *resolve_gitdir(const char *suspect)
    {
        if (is_git_directory(suspect))
           return suspect;
        return read_gitfile(suspect);
    }

And there you see the problem: read_gitfile will die on error.
we'd have to have use read_gitfile_gently(old_git_dir, &err_code),
and then allow READ_GITFILE_ERR_NOT_A_REPO to go through,
just as above.

And that is also the reason why we had to move submodule_uses_worktrees
down, as it also uses no gentle function to look for a git directory
(read: it would die as well). When you have bogus content in your
.git file, there is really nothing you can do to determine if the submodule
is part of a worktree setup, so it is fine to postpone the check until after we
fixed up the link.

So here is the bug you spotted: If it is a worktree already, then
read_gitfile_gently would work fine, no need to "fix" it.

I'll resend with logic as follows:

    char *retvalue = read_gitfile_gently(old_git_dir, &err_code);
    if (retvalue)
        // return early; a worktree is fine here, no need to check
        // because we do nothing

    if (err_code == READ_GITFILE_ERR_NOT_A_REPO)
        // connect; then check for worktree and return early;

    // do the actual relocation.


[1] as found e.g. at
https://public-inbox.org/git/1481915002-162130-2-git-send-email-bmwill@google.com/

>
>>
>> -     /* Is it already absorbed into the superprojects git dir? */
>> -     real_sub_git_dir = real_pathdup(sub_git_dir);
>> -     real_common_git_dir = real_pathdup(get_git_common_dir());
>> -     if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
>> -             relocate_single_git_dir_into_superproject(prefix, path);
>> +     relocate_single_git_dir_into_superproject(prefix, path);
>
> So the check was just pushed into the relocation function.

The check was pushed down, so we can use the
connect_work_tree_and_git_dir instead.

>
>>
>>       if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>>               struct child_process cp = CHILD_PROCESS_INIT;
>> @@ -1481,6 +1484,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
>>
>>  out:
>>       strbuf_release(&gitdir);
>> -     free(real_sub_git_dir);
>> -     free(real_common_git_dir);
>>  }
>> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
>> index 1c47780e2b..e2bbb449b6 100755
>> --- a/t/t7412-submodule-absorbgitdirs.sh
>> +++ b/t/t7412-submodule-absorbgitdirs.sh
>> @@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested submodule' '
>>       test_cmp expect.2 actual.2
>>  '
>>
>> +test_expect_success 're-setup nested submodule' '
>> +     # un-absorb the direct submodule, to test if the nested submodule
>> +     # is still correct (needs a rewrite of the gitfile only)
>> +     rm -rf sub1/.git &&
>> +     mv .git/modules/sub1 sub1/.git &&
>> +     GIT_WORK_TREE=. git -C sub1 config --unset core.worktree &&
>> +     # fixup the nested submodule
>> +     echo "gitdir: ../.git/modules/nested" >sub1/nested/.git &&
>> +     GIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \
>> +             core.worktree "../../../nested" &&
>> +     # make sure this re-setup is correct
>> +     git status --ignore-submodules=none
>> +'
>> +
>> +test_expect_success 'absorb the git dir in a nested submodule' '
>> +     git status >expect.1 &&
>> +     git -C sub1/nested rev-parse HEAD >expect.2 &&
>> +     git submodule absorbgitdirs &&
>> +     test -f sub1/.git &&
>> +     test -f sub1/nested/.git &&
>> +     test -d .git/modules/sub1/modules/nested &&
>> +     git status >actual.1 &&
>> +     git -C sub1/nested rev-parse HEAD >actual.2 &&
>> +     test_cmp expect.1 actual.1 &&
>> +     test_cmp expect.2 actual.2
>> +'
>> +
>>  test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>>       git init sub2 &&
>>       test_commit -C sub2 first &&
>> --
>> 2.11.0.486.g67830dbe1c
>
>
> Aside from my one question the rest of this looks good to me.
>
> --
> Brandon Williams