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

[Brandon Williams <bmwill@google.com>]

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