Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2017, 05:13 UTC
Message-ID: <xmqqvarly8hn.fsf@junio-linux.mtv.corp.google.com>
In-Reply-To: <20170307034553.10770-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 8 quoted lines

> diff --git a/submodule.c b/submodule.c
> index 3b98766a6b..a63aef2c6b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1514,3 +1514,90 @@ void absorb_git_dir_into_superproject(const char *prefix,
>  		strbuf_release(&sb);
>  	}
>  }

Please have a comment here what this function expects (e.g. it is called after we ran repository discovery and $cwd is at the root of the working tree of the current project, we _know_ we have a working tree, etc.).

Show 13 quoted lines

> +
> +static int superproject_exists(void)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *one_up = real_path_if_valid("../");
> +	const char *dirname;
> +	int code, has_superproject = 0;
> +
> +	if (!one_up)
> +		/* At the root of the file system. */
> +		return 0;

Hmph.  I would have expected that "/../" would be the same as "/".

> +	dirname = relative_path(xgetcwd(), one_up, &sb);

So, the idea is we start at the root level of the current project's working tree, and we go up one level, then we know the last component of the path our submodule is bound at in the superproject.

> +	prepare_submodule_repo_env(&cp.env_array);
> +	argv_array_pop(&cp.env_array);
> +	argv_array_pushl(&cp.args, "--literal-pathspecs", "-C", "..",
> +			"ls-tree", "HEAD", "--", dirname, NULL);

This would ask our superproject what is at the "dirname" in its HEAD. Two possible issues:

 - Shouldn't that be looking at its index instead?  It would be more
   correct for unborn branch case, and new or moved submodule case.

 - If our submodule is bound at path sub/dir in the superproject,
   the relative-path thing above would get "dir" and this ls-tree
   ends up asking what is at "dir", but the question you really want
   to ask is what is at "sub/dir", isn't it?

> +const char *get_superproject_working_tree()

const char *get_superproject_working_tree(void)