Re: [PATCH 39/47] setup: limit get_git_work_tree()'s to explicit setup case only
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 18, 2011, 19:41 UTC
- Message-ID
- <7v1v4aknij.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110118074400.GA4185@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 13 quoted lines
> 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?
Show 13 quoted lines
> /* #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).