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

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

[Stefan Beller]

Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
When nested is already absorbed into sub, but sub is not absorbed into
its superproject, then we need to fixup the gitfile and core.worktree
setting for 'nested' when absorbing 'sub', but we do not need to move
its git dir around.

Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

An alternative I considered to do this work lazily, i.e. when resolving
"../.git/modules/nested", we would notice the ".git" being a gitfile
linking to another path.  That seemed to be robuster by design, but harder
to get the implementation right.  Maybe we have to do that anyway once we
try to have submodules and worktrees working nicely together, but for now
just produce 'correct' (i.e. direct) pointers.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

  cc Jeff and Brandon, who worked on the setup code recently,
  and the alternative design mentioned was messing around a lot in setup.c.
  
  Thanks,
  Stefan

 submodule.c                        | 41 +++++++++++++++++++-------------------
 t/t7412-submodule-absorbgitdirs.sh | 27 +++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4c4f033e8a..7deb0fca6a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1393,16 +1393,9 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
 	char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
 	const char *new_git_dir;
 	const struct submodule *sub;
-
-	if (submodule_uses_worktrees(path))
-		die(_("relocate_gitdir for submodule '%s' with "
-		      "more than one worktree not supported"), path);
+	int err_code;
 
 	old_git_dir = xstrfmt("%s/.git", path);
-	if (read_gitfile(old_git_dir))
-		/* If it is an actual gitfile, it doesn't need migration. */
-		return;
-
 	real_old_git_dir = real_pathdup(old_git_dir);
 
 	sub = submodule_from_path(null_sha1, path);
@@ -1414,6 +1407,24 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
 		die(_("could not create directory '%s'"), new_git_dir);
 	real_new_git_dir = real_pathdup(new_git_dir);
 
+	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);
+		return;
+	}
+
+	if (submodule_uses_worktrees(path))
+		die(_("relocate_gitdir for submodule '%s' with "
+		      "more than one worktree not supported"), path);
+
 	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;
 
-	/* 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);
 
 	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

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

[Brandon Williams]

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?

>  
> -	/* 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.

>  
>  	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

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

[Stefan Beller]

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

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

[Brandon Williams]

On 01/24, Stefan Beller wrote:
> 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.

Hmm, then maybe is_submodule_populated should be rewritten to not die on
an error then?

> 
> 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

-- 
Brandon Williams

[PATCHv2 0/3] fix recursive submodule absorbing

[Stefan Beller]

In this second iteration, only absorb_git_dir_into_superproject is touched,
which does the check if connect_work_tree_and_git_dir is needed.

Internally I also had a patch that converts is_submodule_populated
to be gentle, but it is not needed here, so I dropped it before sending out.

Thanks,
Stefan


Stefan Beller (3):
  Add gentle version of resolve_git_dir
  cache.h: expose the dying procedure for reading gitlinks
  submodule absorbing: fix worktree/gitdir pointers recursively for
    non-moves

 cache.h                            |  5 +++-
 setup.c                            | 52 ++++++++++++++++++++------------------
 submodule.c                        | 43 ++++++++++++++++++++++++-------
 t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++
 4 files changed, 93 insertions(+), 34 deletions(-)

-- 
2.11.0.495.g04f60290a0.dirty

[PATCHv2 1/3] Add gentle version of resolve_git_dir

[Stefan Beller]

This follows a93bedada (setup: add gentle version of read_gitfile,
2015-06-09), and assumes the same reasoning. resolve_git_dir is unsuited
for speculative calls, so we want to use the gentle version to find out
about potential errors.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 4 +++-
 setup.c | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 00a029af36..cafa3d10ae 100644
--- a/cache.h
+++ b/cache.h
@@ -509,7 +509,9 @@ extern int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_TOO_LARGE 8
 extern const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
-extern const char *resolve_gitdir(const char *suspect);
+extern const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
+#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
+
 extern void set_git_work_tree(const char *tree);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
diff --git a/setup.c b/setup.c
index 1b534a7508..4605fd3c3c 100644
--- a/setup.c
+++ b/setup.c
@@ -1017,11 +1017,11 @@ const char *setup_git_directory(void)
 	return setup_git_directory_gently(NULL);
 }
 
-const char *resolve_gitdir(const char *suspect)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
 {
 	if (is_git_directory(suspect))
 		return suspect;
-	return read_gitfile(suspect);
+	return read_gitfile_gently(suspect, return_error_code);
 }
 
 /* if any standard file descriptor is missing open it to /dev/null */
-- 
2.11.0.495.g04f60290a0.dirty

[PATCHv2 2/3] cache.h: expose the dying procedure for reading gitlinks

[Stefan Beller]

In a later patch we want to react to only a subset of errors, defaulting
the rest to die as usual. Separate the block that takes care of dying
into its own function so we have easy access to it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h |  1 +
 setup.c | 48 ++++++++++++++++++++++++++----------------------
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/cache.h b/cache.h
index cafa3d10ae..d55f5dccb1 100644
--- a/cache.h
+++ b/cache.h
@@ -507,6 +507,7 @@ extern int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+extern void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 extern const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
 extern const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
diff --git a/setup.c b/setup.c
index 4605fd3c3c..967f289f1e 100644
--- a/setup.c
+++ b/setup.c
@@ -486,6 +486,30 @@ int verify_repository_format(const struct repository_format *format,
 	return 0;
 }
 
+void read_gitfile_error_die(int error_code, const char *path, const char *dir)
+{
+	switch (error_code) {
+	case READ_GITFILE_ERR_STAT_FAILED:
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		/* non-fatal; follow return path */
+		break;
+	case READ_GITFILE_ERR_OPEN_FAILED:
+		die_errno("Error opening '%s'", path);
+	case READ_GITFILE_ERR_TOO_LARGE:
+		die("Too large to be a .git file: '%s'", path);
+	case READ_GITFILE_ERR_READ_FAILED:
+		die("Error reading %s", path);
+	case READ_GITFILE_ERR_INVALID_FORMAT:
+		die("Invalid gitfile format: %s", path);
+	case READ_GITFILE_ERR_NO_PATH:
+		die("No path in gitfile: %s", path);
+	case READ_GITFILE_ERR_NOT_A_REPO:
+		die("Not a git repository: %s", dir);
+	default:
+		die("BUG: unknown error code");
+	}
+}
+
 /*
  * Try to read the location of the git directory from the .git file,
  * return path to git directory if found.
@@ -559,28 +583,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 cleanup_return:
 	if (return_error_code)
 		*return_error_code = error_code;
-	else if (error_code) {
-		switch (error_code) {
-		case READ_GITFILE_ERR_STAT_FAILED:
-		case READ_GITFILE_ERR_NOT_A_FILE:
-			/* non-fatal; follow return path */
-			break;
-		case READ_GITFILE_ERR_OPEN_FAILED:
-			die_errno("Error opening '%s'", path);
-		case READ_GITFILE_ERR_TOO_LARGE:
-			die("Too large to be a .git file: '%s'", path);
-		case READ_GITFILE_ERR_READ_FAILED:
-			die("Error reading %s", path);
-		case READ_GITFILE_ERR_INVALID_FORMAT:
-			die("Invalid gitfile format: %s", path);
-		case READ_GITFILE_ERR_NO_PATH:
-			die("No path in gitfile: %s", path);
-		case READ_GITFILE_ERR_NOT_A_REPO:
-			die("Not a git repository: %s", dir);
-		default:
-			assert(0);
-		}
-	}
+	else if (error_code)
+		read_gitfile_error_die(error_code, path, dir);
 
 	free(buf);
 	return error_code ? NULL : path;
-- 
2.11.0.495.g04f60290a0.dirty

[PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Stefan Beller]

Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
When nested is already absorbed into sub, but sub is not absorbed into
its superproject, then we need to fixup the gitfile and core.worktree
setting for 'nested' when absorbing 'sub', but we do not need to move
its git dir around.

Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

An alternative I considered to do this work lazily, i.e. when resolving
"../.git/modules/nested", we would notice the ".git" being a gitfile
linking to another path.  That seemed to be robuster by design, but harder
to get the implementation right.  Maybe we have to do that anyway once we
try to have submodules and worktrees working nicely together, but for now
just produce 'correct' (i.e. direct) pointers.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c                        | 43 ++++++++++++++++++++++++++++++--------
 t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4c4f033e8a..95437105bf 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1437,22 +1437,47 @@ void absorb_git_dir_into_superproject(const char *prefix,
 				      const char *path,
 				      unsigned flags)
 {
+	int err_code;
 	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);
+	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
 
 	/* Not populated? */
-	if (!sub_git_dir)
-		goto out;
+	if (!sub_git_dir) {
+		char *real_new_git_dir;
+		const char *new_git_dir;
+		const struct submodule *sub;
+
+		if (err_code == READ_GITFILE_ERR_STAT_FAILED)
+			goto out; /* unpopulated as expected */
+		if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
+			/* We don't know what broke here. */
+			read_gitfile_error_die(err_code, path, NULL);
 
-	/* 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);
+		/*
+		* Maybe populated, but no git directory was found?
+		* This can happen if the superproject is a submodule
+		* itself and was just absorbed. The absorption of the
+		* superproject did not rewrite the git file links yet,
+		* fix it now.
+		*/
+		sub = submodule_from_path(null_sha1, path);
+		if (!sub)
+			die(_("could not lookup name for submodule '%s'"), path);
+		new_git_dir = git_path("modules/%s", sub->name);
+		if (safe_create_leading_directories_const(new_git_dir) < 0)
+			die(_("could not create directory '%s'"), new_git_dir);
+		real_new_git_dir = real_pathdup(new_git_dir);
+		connect_work_tree_and_git_dir(path, real_new_git_dir);
+	} else {
+		/* 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);
+	}
 
 	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
 		struct child_process cp = CHILD_PROCESS_INIT;
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.495.g04f60290a0.dirty

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

[Brandon Williams]

On 01/24, Stefan Beller wrote:
> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
> 
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".
> 
> An alternative I considered to do this work lazily, i.e. when resolving
> "../.git/modules/nested", we would notice the ".git" being a gitfile
> linking to another path.  That seemed to be robuster by design, but harder
> to get the implementation right.  Maybe we have to do that anyway once we
> try to have submodules and worktrees working nicely together, but for now
> just produce 'correct' (i.e. direct) pointers.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  submodule.c                        | 43 ++++++++++++++++++++++++++++++--------
>  t/t7412-submodule-absorbgitdirs.sh | 27 ++++++++++++++++++++++++
>  2 files changed, 61 insertions(+), 9 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 4c4f033e8a..95437105bf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1437,22 +1437,47 @@ void absorb_git_dir_into_superproject(const char *prefix,
>  				      const char *path,
>  				      unsigned flags)
>  {
> +	int err_code;
>  	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);
> +	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
>  
>  	/* Not populated? */
> -	if (!sub_git_dir)
> -		goto out;
> +	if (!sub_git_dir) {
> +		char *real_new_git_dir;
> +		const char *new_git_dir;
> +		const struct submodule *sub;
> +
> +		if (err_code == READ_GITFILE_ERR_STAT_FAILED)
> +			goto out; /* unpopulated as expected */
> +		if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
> +			/* We don't know what broke here. */
> +			read_gitfile_error_die(err_code, path, NULL);
>  
> -	/* 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);
> +		/*
> +		* Maybe populated, but no git directory was found?
> +		* This can happen if the superproject is a submodule
> +		* itself and was just absorbed. The absorption of the
> +		* superproject did not rewrite the git file links yet,
> +		* fix it now.
> +		*/
> +		sub = submodule_from_path(null_sha1, path);
> +		if (!sub)
> +			die(_("could not lookup name for submodule '%s'"), path);
> +		new_git_dir = git_path("modules/%s", sub->name);
> +		if (safe_create_leading_directories_const(new_git_dir) < 0)
> +			die(_("could not create directory '%s'"), new_git_dir);
> +		real_new_git_dir = real_pathdup(new_git_dir);
> +		connect_work_tree_and_git_dir(path, real_new_git_dir);

Memory leak with 'real_new_git_dir'

> +	} else {
> +		/* 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());

The scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can
be narrowed.  Move their declaration here and free it prior to exiting
the else block.

> +		if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))

'v' isn't ever used, just use starts_with() and get rid of 'v'

> +			relocate_single_git_dir_into_superproject(prefix, path);
> +	}
>  

There's a label 'out' at the end of the function which can be removed as
there is no 'goto' statement using it.

>  	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
>  		struct child_process cp = CHILD_PROCESS_INIT;
> 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.495.g04f60290a0.dirty
> 

You can squash them into your patch, assuming you agree with the changes
:)

-- 
Brandon Williams

--8<--

From 0336c4bee60a85d8ad319ff55deea95debb3ceda Mon Sep 17 00:00:00 2001
From: Brandon Williams <bmwill@google.com>
Date: Tue, 24 Jan 2017 16:44:43 -0800
Subject: [PATCH] SQUASH

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 submodule.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index 95437105b..0d9c4bbbe 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1438,8 +1438,7 @@ void absorb_git_dir_into_superproject(const char *prefix,
 				      unsigned flags)
 {
 	int err_code;
-	const char *sub_git_dir, *v;
-	char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
+	const char *sub_git_dir;
 	struct strbuf gitdir = STRBUF_INIT;
 	strbuf_addf(&gitdir, "%s/.git", path);
 	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
@@ -1471,12 +1470,18 @@ void absorb_git_dir_into_superproject(const char *prefix,
 			die(_("could not create directory '%s'"), new_git_dir);
 		real_new_git_dir = real_pathdup(new_git_dir);
 		connect_work_tree_and_git_dir(path, real_new_git_dir);
+
+		free(real_new_git_dir);
 	} else {
 		/* 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))
+		char *real_sub_git_dir = real_pathdup(sub_git_dir);
+		char *real_common_git_dir = real_pathdup(get_git_common_dir());
+
+		if (!starts_with(real_sub_git_dir, real_common_git_dir))
 			relocate_single_git_dir_into_superproject(prefix, path);
+
+		free(real_sub_git_dir);
+		free(real_common_git_dir);
 	}
 
 	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
@@ -1506,6 +1511,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
 out:
 	strbuf_release(&gitdir);
-	free(real_sub_git_dir);
-	free(real_common_git_dir);
 }
-- 
2.11.0.483.g087da7b7c-goog

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

[Stefan Beller]

On Tue, Jan 24, 2017 at 4:46 PM, Brandon Williams <bmwill@google.com> wrote:
> On 01/24, Stefan Beller wrote:
>> +             connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> Memory leak with 'real_new_git_dir'

yup. :(


> The scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can
> be narrowed.  Move their declaration here and free it prior to exiting
> the else block.

ok.

> 'v' isn't ever used, just use starts_with() and get rid of 'v'

makes sense.

>
>> +                     relocate_single_git_dir_into_superproject(prefix, path);
>> +     }
>>
>
> There's a label 'out' at the end of the function which can be removed as
> there is no 'goto' statement using it.

We do use it in

    if (err_code == READ_GITFILE_ERR_STAT_FAILED)
        goto out; /* unpopulated as expected */

I took your proposed SQUASH and removed the goto, see the followup email.

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
>
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

That sounds like a sensible way to make things consistent.

[PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Stefan Beller]

Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
When nested is already absorbed into sub, but sub is not absorbed into
its superproject, then we need to fixup the gitfile and core.worktree
setting for 'nested' when absorbing 'sub', but we do not need to move
its git dir around.

Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

An alternative I considered to do this work lazily, i.e. when resolving
"../.git/modules/nested", we would notice the ".git" being a gitfile
linking to another path.  That seemed to be robuster by design, but harder
to get the implementation right.  Maybe we have to do that anyway once we
try to have submodules and worktrees working nicely together, but for now
just produce 'correct' (i.e. direct) pointers.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 This replaces the last patch of the series, containing Brandons SQUASH proposal
 as well as the removal of the goto.
 Thanks,
 Stefan

 submodule.c                        | 62 ++++++++++++++++++++++++++++----------
 t/t7412-submodule-absorbgitdirs.sh | 27 +++++++++++++++++
 2 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4c4f033e8a..3b98766a6b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1437,22 +1437,57 @@ 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;
+	int err_code;
+	const char *sub_git_dir;
 	struct strbuf gitdir = STRBUF_INIT;
-
 	strbuf_addf(&gitdir, "%s/.git", path);
-	sub_git_dir = resolve_gitdir(gitdir.buf);
+	sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
 
 	/* Not populated? */
-	if (!sub_git_dir)
-		goto out;
+	if (!sub_git_dir) {
+		char *real_new_git_dir;
+		const char *new_git_dir;
+		const struct submodule *sub;
+
+		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+			/* unpopulated as expected */
+			strbuf_release(&gitdir);
+			return;
+		}
+
+		if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
+			/* We don't know what broke here. */
+			read_gitfile_error_die(err_code, path, NULL);
+
+		/*
+		* Maybe populated, but no git directory was found?
+		* This can happen if the superproject is a submodule
+		* itself and was just absorbed. The absorption of the
+		* superproject did not rewrite the git file links yet,
+		* fix it now.
+		*/
+		sub = submodule_from_path(null_sha1, path);
+		if (!sub)
+			die(_("could not lookup name for submodule '%s'"), path);
+		new_git_dir = git_path("modules/%s", sub->name);
+		if (safe_create_leading_directories_const(new_git_dir) < 0)
+			die(_("could not create directory '%s'"), new_git_dir);
+		real_new_git_dir = real_pathdup(new_git_dir);
+		connect_work_tree_and_git_dir(path, real_new_git_dir);
+
+		free(real_new_git_dir);
+	} else {
+		/* Is it already absorbed into the superprojects git dir? */
+		char *real_sub_git_dir = real_pathdup(sub_git_dir);
+		char *real_common_git_dir = real_pathdup(get_git_common_dir());
 
-	/* 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);
+		if (!starts_with(real_sub_git_dir, real_common_git_dir))
+			relocate_single_git_dir_into_superproject(prefix, path);
+
+		free(real_sub_git_dir);
+		free(real_common_git_dir);
+	}
+	strbuf_release(&gitdir);
 
 	if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1478,9 +1513,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
 		strbuf_release(&sb);
 	}
-
-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.495.g04f60290a0.dirty

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

[Brandon Williams]

On 01/25, Stefan Beller wrote:
> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
> 
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".
> 
> An alternative I considered to do this work lazily, i.e. when resolving
> "../.git/modules/nested", we would notice the ".git" being a gitfile
> linking to another path.  That seemed to be robuster by design, but harder
> to get the implementation right.  Maybe we have to do that anyway once we
> try to have submodules and worktrees working nicely together, but for now
> just produce 'correct' (i.e. direct) pointers.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>

Looks good to me!

-- 
Brandon Williams