Re: [PATCH v3 2/4] introduce submodule.superprojectGitDir record

[Matheus Tavares <matheus.bernardino@usp.br>]

Emily Shaffer <emilyshaffer@google.com> wrote:
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..d60fcd2c7d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1910,6 +1910,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
>  					   error_strategy);
> 
> +	git_config_set_in_file(p, "submodule.superprojectGitdir",
> +			       relative_path(absolute_path(get_git_dir()),
> +					     path, &sb));

This will be executed when cloning a submodule with
`git submodule add <url/path> <path>`. Do we also want to set
submodule.superprojectGitdir when adding a repository that already exists in
the working tree as a submodule? I.e., something like:

git init super
git init super/sub
[ make commits in super/sub ]
git -C super submodule add ./sub

I don't know if this workflow is so commonly used, though... It may not be
worth the additional work.

Another option, which I believe was suggested by Jonathan Nieder on the Review
Club, is to change the code to absorb the gitdir when adding the local
submodule. Then, the configuration would already be set by the
`absorb_git_dir...()` function itself.

>  	free(sm_alternate);
>  	free(error_strategy);
> 
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 4bc6b6c886..e407329d81 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -108,14 +108,18 @@ test_expect_success 'setup - repository to add submodules to' '
>  submodurl=$(pwd -P)
> 
>  inspect() {
> -	dir=$1 &&
> -
> -	git -C "$dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&
> -	{ git -C "$dir" symbolic-ref HEAD || :; } >head &&
> -	git -C "$dir" rev-parse HEAD >head-sha1 &&
> -	git -C "$dir" update-index --refresh &&
> -	git -C "$dir" diff-files --exit-code &&
> -	git -C "$dir" clean -n -d -x >untracked
> +	sub_dir=$1 &&
> +	super_dir=$2 &&
> +
> +	git -C "$sub_dir" for-each-ref --format='%(refname)' 'refs/heads/*' >heads &&
> +	{ git -C "$sub_dir" symbolic-ref HEAD || :; } >head &&
> +	git -C "$sub_dir" rev-parse HEAD >head-sha1 &&
> +	git -C "$sub_dir" update-index --refresh &&
> +	git -C "$sub_dir" diff-files --exit-code &&
> +	cached_super_dir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" &&
> +	[ "$(git -C "$super_dir" rev-parse --absolute-git-dir)" \
> +		-ef "$sub_dir/$cached_super_dir" ] &&

To avoid the non-POSIX `-ef`, we could perhaps do something like: 

	super_gitdir="$(git -C "$super_dir" rev-parse --absolute-git-dir)" &&
	cached_gitdir="$(git -C "$sub_dir" config --get submodule.superprojectGitDir)" &&
	test "$cached_gitdir" = "$(test-tool path-utils relative_path "$super_gitdir" "$PWD/$sub_dir")" &&

(We need the "$PWD/" at the last command because `path.c:relative_path()`
returns the first argument as-is when one of the two paths given to it is
absolute and the other is not.)

One bonus of testing the cached path this way is that we also check that
it is indeed being stored as a relative path :)