On Tue, Jan 24, 2017 at 1:58 PM, Brandon Williams <bmwill@xxxxxxxxxx> 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@xxxxxxxxxx/
>
>>
>> - /* 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