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

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 
> ---
>  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(, "%s/.git", path);
> - sub_git_dir = resolve_gitdir(gitdir.buf);
> + sub_git_dir = resolve_gitdir_gently(gitdir.buf, _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, ))
> - 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' 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 

[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 
---
 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(, "%s/.git", path);
-   sub_git_dir = resolve_gitdir(gitdir.buf);
+   sub_git_dir = resolve_gitdir_gently(gitdir.buf, _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, ))
-   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, ))
+   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