Re: [PATCH 39/47] setup: limit get_git_work_tree()'s to explicit setup case only

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

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/setup.c b/setup.c
> index 3d73269..28b2fef 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -419,6 +419,11 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>  		return NULL;
>  	}
>  
> +	if (getenv(GIT_WORK_TREE_ENVIRONMENT)) {
> +		warning("GIT_WORK_TREE without explicit GIT_DIR is deprecated");
> +		return setup_explicit_git_dir(gitdir, cwd, offset, nongit_ok);
> +	}
> +

My knee-jerk reaction is that calling this "deprecated" is probably
confusing. git merely failed to notice misconfiguration so far and went
ahead to produce a random result when GIT_DIR is not set (hence the usual
rule is in effect to find out where the .git directory is, starting from
the assumption that you _are_ somewhere _inside_ the work tree, _and_ the
root of the work tree _is_ where that .git directory is found in) and at
the same time GIT_WORK_TREE is pointing at a random other location.

Unless that location happens to match the parent directory of the
discovered .git, this invalidates the assumption of the whole discovery
logic (iow, how you found .git to begin with). In other words, it is my
understanding that setting only GIT_WORK_TREE without setting GIT_DIR
wasn't meant to be supported at all, because there is no sane logic to
specify a consistent behaviour in such a case.

If "we first run discovery as if you were somewhere in your work tree to
find .git directory, but you are allowed to say that your working tree is
elsewhere, possibly completely unrelated to the tree hierarchy used to
find .git directory to begin with; we could accomodate these conflicting
wishes because hopefully the necessary set-up happens before we actually
need to touch anything in the work tree" were a sane specification, this
patch may start making sense. Not breaking existing scripts that rely on
the behaviour is a nice bonus. The "hopefully" part makes me worried
enough to keep me from saying that semantics is sane without thinking
though.

I notice that git(1) manual page discusses --work-tree=<path> (and
GIT_WORK_TREE) to a reasonable level of details (the most importantly, the
rule to determine where the root of the working tree is when nothing is
specified is given), but the corresponding description on --git-dir=<path>
(and GIT_DIR) is too sketchy to tell how the discovery works when nothing
is given. Perhaps that part needs to address what the semantics should be?

>  	/* #0, #1, #5, #8, #9, #12, #13 */
>  	set_git_work_tree(".");
>  	if (strcmp(gitdir, DEFAULT_GIT_DIR_ENVIRONMENT))
> @@ -443,6 +448,11 @@ static const char *setup_bare_git_dir(char *cwd, int offset, int len, int *nongi
>  	if (check_repository_format_gently(".", nongit_ok))
>  		return NULL;
>  
> +	if (getenv(GIT_WORK_TREE_ENVIRONMENT)) {
> +		warning("GIT_WORK_TREE without explicit GIT_DIR is deprecated");
> +		return setup_explicit_git_dir(".", cwd, offset, nongit_ok);
> +	}
>  	inside_git_dir = 1;
>  	inside_work_tree = 0;

This is for people who do "cd .git && GIT_WORK_TREE=.. git cmd". I have to
wonder what happens to the pathspec given to the cmd---you are clearly
outside of your working tree.

A tangent.

Do we make sure that whatever GIT_WORK_TREE we end up with using is an
ancestor directory of the $CWD when we require us to be inside the working
tree? I think we should, as I don't think of a sane use case otherwise
(unless you call "cd ../neigh; GIT_WORK_TREE=../work git diff ../work/foo"
a sane way to futz with the file "foo" in the working tree "work" from a
directory "neigh" that is unrelated to the repository).