Re: [GSoC Patch v7 2/8] submodule: use repository to find superproject

[Justin Tobler <jltobler@gmail.com>]

On 26/09/27 05:14PM, K Jayatheerth wrote:
> `get_superproject_working_tree()` currently uses `xgetcwd()` to
> determine the repository whose superproject should be checked.
> 
> This is incorrect when `--git-dir` points to a repository different
> from the one associated with the current working directory. In that
> case, the current working directory may cause the function to return
> the wrong superproject or an empty result.

Ok, so we are not taking into account here that the git dir location
could be different than the current working directory.

> Pass the `repository` to `get_superproject_working_tree()` so that the
> superproject is determined from the repository being inspected rather
> than the current working directory.

Makes sense.

> Mentored-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
>  builtin/rev-parse.c        |  2 +-
>  submodule.c                | 43 +++++++++++++++++++-------------------
>  submodule.h                |  2 +-
>  t/t7400-submodule-basic.sh | 19 +++++++++++++++++
>  4 files changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 43693454d5..e1a6da0076 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -997,7 +997,7 @@ int cmd_rev_parse(int argc,
>  			}
>  			if (!strcmp(arg, "--show-superproject-working-tree")) {
>  				struct strbuf superproject = STRBUF_INIT;
> -				if (get_superproject_working_tree(&superproject))
> +				if (get_superproject_working_tree(the_repository, &superproject))
>  					print_path(superproject.buf, prefix, format, DEFAULT_UNMODIFIED);
>  				strbuf_release(&superproject);
>  				continue;
> diff --git a/submodule.c b/submodule.c
> index 6fcb606f7e..4db90ef9b0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2610,34 +2610,35 @@ void absorb_git_dir_into_superproject(const char *path,
>  	absorb_git_dir_into_superproject_recurse(path, super_prefix);
>  }
>  
> -int get_superproject_working_tree(struct strbuf *buf)
> +int get_superproject_working_tree(struct repository *r, struct strbuf *buf)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf one_up = STRBUF_INIT;
> -	char *cwd = xgetcwd();
> +	struct strbuf target_wt = STRBUF_INIT;
> +	const char *worktree;
>  	int ret = 0;
>  	const char *subpath;
>  	int code;
>  	ssize_t len;
>  
> -	if (!is_inside_work_tree(the_repository))
> -		/*
> -		 * FIXME:
> -		 * We might have a superproject, but it is harder
> -		 * to determine.
> -		 */
> +	worktree = repo_get_work_tree(r);

We now use the passed repository to get the working tree instead of
relying getting the current working directory and checking if we are
inside a worktree. Looks good.

> +	if (!worktree)
> +		goto out;
> +
> +	if (!strbuf_realpath(&target_wt, worktree, 0))
>  		goto out;
>  
> -	if (!strbuf_realpath(&one_up, "../", 0))
> +	strbuf_addf(&one_up, "%s/..", target_wt.buf);
> +	if (!strbuf_realpath(&one_up, one_up.buf, 0))

I was confused at first why we bother invoking `strbuf_realpath()` here
since it's just an intermediate step, but I assume that is so we can use
it in the error statement later on.

>  		goto out;
>  
> -	subpath = relative_path(cwd, one_up.buf, &sb);
> +	subpath = relative_path(target_wt.buf, one_up.buf, &sb);
>  
>  	prepare_submodule_repo_env(&cp.env);
>  	strvec_pop(&cp.env);
>  
> -	strvec_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> +	strvec_pushl(&cp.args, "--literal-pathspecs", "-C", one_up.buf,
>  		     "ls-files", "-z", "--stage", "--full-name", "--",
>  		     subpath, NULL);
>  	strbuf_reset(&sb);
> @@ -2648,14 +2649,14 @@ int get_superproject_working_tree(struct strbuf *buf)
>  	cp.git_cmd = 1;
>  
>  	if (start_command(&cp))
> -		die(_("could not start ls-files in .."));
> +		die(_("could not start ls-files in %s"), one_up.buf);
>  
>  	len = strbuf_read(&sb, cp.out, PATH_MAX);
>  	close(cp.out);
>  
>  	if (starts_with(sb.buf, "160000")) {
>  		int super_sub_len;
> -		int cwd_len = strlen(cwd);
> +		int wt_len = target_wt.len;
>  		char *super_sub, *super_wt;
>  
>  		/*
[snip]
> +test_expect_success 'path.superproject-root works with --git-dir' '
> +	test_when_finished "rm -rf sub super" &&
> +	git init sub &&
> +	test_commit -C sub initial &&
> +	git init super &&
> +	(
> +		cd super &&
> +		git -c protocol.file.allow=always submodule add "../sub" sub &&
> +		git commit -m "add submodule" &&
> +
> +		SUPER_ROOT="$(test-tool path-utils real_path .)" &&
> +		MODULE_DIR="$SUPER_ROOT/.git/modules/sub" &&
> +
> +		echo "path.superproject-root.absolute=$SUPER_ROOT" >expect &&
> +		git --git-dir="$MODULE_DIR" repo info path.superproject-root.absolute >actual &&

`path.superproject-root` isn't introduced until the next patch, so this
test fails at this commit. Maybe the test should exercise the existing
`git --git-dir=... rev-parse --show-superproject-working-tree` caller in
t1500 instead?

-Justin