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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 19, 2011, 19:31 UTC
Message-ID: <20110119193116.GB30618@burratino>
In-Reply-To: <AANLkTinE5gNZM_HDJq31qs5ARJn-DrO9HW66cszTayPa@mail.gmail.com>

Nguyen Thai Ngoc Duy wrote:

> 2011/1/19 Jonathan Nieder <jrnieder@gmail.com>:

Show 9 quoted lines

>> @@ -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) {
>
> Can we leave git_work_tree_cfg out? If this code is to support misused
> scripts, then $GIT_WORK_TREE alone ought to be enough.

Sure, it would be easy to exclude git_work_tree_cfg here. I guess the relevant question is

Maaartin wrote:

> On 11-01-19 13:42, Jonathan Nieder wrote:

>> Unfortunately the existence of GIT_WORK_TREE makes it tempting to
>> use without setting GIT_DIR.
>
> Maybe I'm asking nonsense, but why should I always use both?

That is, why do we want to discourage setting the work tree without GIT_DIR in the first place?

The issues seem to be:

1. Previously there was some confusion about what path the worktree
   is relative to.  Now setup_explicit_git_dir makes it clear:
   + GIT_WORK_TREE and --work-tree are relative to the original cwd;
   + the "[core] worktree" setting is relative to the gitdir.

2. Previously there was some confusion about the right order of
   operations --- move to worktree first, or find the git dir?
   Now the setup procedure is clearly "first find git dir, then
   act on worktree".

3. A global "[core] worktree" setting with relative path is
   nonsensical since there is not necessarily a gitdir for it to be
   relative to.

4. Likewise, setting GIT_WORK_TREE when outside any git repository
   has no clear meaning.

5. The interaction with core.bare and implicit bareness are not
   obvious.  Clearly core.bare should conflict with core.worktree,
   but can GIT_WORK_TREE override that?  Maybe
   check_repository_format_gently is the right place for this check
   (rather than the setup procedure).

(1) and (2) have been resolved by your work (nice!), (3) seems like a case of "don't do that, then", and (4) out to error out in setup_nongit (though my patch doesn't take care of that). Given an answer to (5) we could wholeheartedly and consistently support worktree with implicit gitdir, as a new feature.

Is that a fair summary?

[...]

Show 8 quoted lines

>> +               if (offset != len && !is_absolute_path(gitdir))
>> +                       gitdir = xstrdup(make_absolute_path(gitdir));
>
> The behavior regarding relative $GIT_WORK_TREE before nd/setup series
> is inconsistent. If setup_git_directory() is used, work_tree is
> relative to user's cwd. In other cases, when get_git_work_tree() is
> called, work_tree is made absolute relative to _current_ cwd (usually
> at discovered work_tree root). Which way do you want to keep?

The former. But this is worrisome: scripts using cd_to_toplevel are getting the latter behavior. Maybe a warning for relative GIT_WORK_TREE is in order, like you hinted before.