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

[Brandon Williams <bmwill@google.com>]

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