Re: [PATCH 3/3] setup: always honor GIT_WORK_TREE and core.worktree

[Junio C Hamano <gitster@pobox.com>]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In the usual case, git commands are run from within a git worktree.
> The toplevel directory and associated .git dir are found by chdir-ing
> to .. repeatedly until .git is a valid repository.
>
> That behavior can be overridden in two ways.

Hmm, I wouldn't call them "two ways".

We originally had GIT_DIR and people who wanted to use a working tree
without an embedded .git (hence having to use GIT_DIR) complained that
they cannot work from subdirectories while cd'ing around; GIT_WORK_TREE
was added to augment the mechanism to allow them to specify where their
root is.  Saying "two ways" sounds as if it is sane to do one without
doing the other, which is not the case here.

For the same reason (another tangent), although t1510 seems to have
complex rules about "relative to", it does not make much sense to specify
a path in GIT_WORK_TREE relative to user's $cwd.  The mechanism is to
allow people to chdir around inside their working tree that does not have
an embedded .git directory to begin with.

> diff --git a/setup.c b/setup.c
> index 3d73269..c0f5846 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -411,6 +411,16 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>  	if (check_repository_format_gently(gitdir, nongit_ok))
>  		return NULL;
>  
> +	/* Accept --work-tree to support old scripts that played with fire. */
> +	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> +		warning("pretending GIT_DIR was supplied alongside GIT_WORK_TREE");
> +		if (offset != len && !is_absolute_path(gitdir))
> +			gitdir = xstrdup(make_absolute_path(gitdir));
> +		if (chdir(cwd))
> +			die_errno("Could not come back to cwd");
> +		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
> +	}
> +
>  	/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
>  	if (is_bare_repository_cfg > 0) {
>  		set_git_dir(offset == len ? gitdir : make_absolute_path(gitdir));
> @@ -443,6 +453,19 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
>  	if (check_repository_format_gently(".", nongit_ok))
>  		return NULL;
>  
> +	/*
> +	 * Accept --work-tree, reluctantly.
> +	 */
> +	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> +		const char *gitdir;
> +
> +		warning("pretending GIT_DIR was supplied alongside GIT_WORK_TREE");
> +		gitdir = offset == len ? "." : xmemdupz(cwd, offset);
> +		if (chdir(cwd))
> +			die_errno("Could not come back to cwd");
> +		return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
> +	}
> +
>  	inside_git_dir = 1;
>  	inside_work_tree = 0;
>  	if (offset != len) {

This seems to be a good workaround to resurrect the historical buggy
behaviour, but it makes the already ugly code even uglier. Can we do
something about it?

I wonder if it would make it more readable if we restructure
setup_git_directory_gently_1() slightly, perhaps by making the loop to
discover the .git directory/file into a separate helper function that
returns:

 - the path to the discovered .git;
 - if the .git was found as an ancestor of the original $cwd (to handle "bare");
 - if the discovery was aborted due to ceiling (or hitting root);

and make the caller decide what kind of setup to call outside the loop.

But that is a minor point.