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

Nguyen Thai Ngoc Duy wrote:

2011/1/19 Jonathan Nieder [off-list ref]:

quoted

@@ -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:

quoted

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?

[...]

quoted

+               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.