Re: [PATCH(amend)] introduce GIT_WORK_TREE environment variable

Matthias Lederhofer [off-list ref] writes:

With splitting is_bare_repository() it is no longer abused to check
for a working tree.  This raises the question what 'bare' is actually
supposed to mean:
(1) git may do operations on the repository which have strange effects
    if I'm using the repository with a working tree (e.g. update HEAD
    during a fetch)
(2) git may do (1) but also disallows to use the repository with a
working tree

With (1) it would be ok to let a user work with a repository that has
core.bare = true if she really wants to (e.g. by setting GIT_WORK_TREE
explicitly, details to be figured out later).
With (2) git should disallow the use of a working tree if core.bare = true.

Well, core.bare was designed without any support for something
like GIT_WORK_TREE in mind, so I do not think we should be tied
too much to whatever the current implementation happens to do.

If you are using GIT_WORK_TREE and GIT_DIR to name two
directories that are not connected in the usual way
(i.e. $GIT_DIR != $GIT_WORK_TREE/.git) to work in, I *think*
your intention is "I want to checkout, diff, commit and all the
usual git operations in GIT_WORK_TREE to grow branches in
GIT_DIR".  I do not think core.bare should even be looked at in
this case.

A quick "git grep is_bare" reveals that:

 - "git-branch foo $commit" refuses if the repository is not
   bare and foo is the current branch.  The intention here is if
   we allow it then it makes the HEAD and index+working tree out
   of sync, so this clearly is about "does it have a working
   tree, and is it used with one"?

 - "git gc" runs pack-refs only if the repository is not bare.
   This is to prevent public repositories from getting
   pack-ref-pruned (which would make older dumb clients not to
   be able to fetch from it).  This _is_ about is_bare; it does
   not matter if somebody else happens to use it with a working
   tree.

 - reflog update code in refs.c defaults log_all_ref_updates
   value to true when !is-bare; the intent is if the repository
   is used to actively build history with working tree we would
   want the reflog, otherwise if it is primarily used to track
   other repositories (either pushing into it, or fetching from
   elsewhere) reflog is often clutter.  So this is not about
   bare, but "does it have a working tree?".

 - "git ls-files" refuses if "is-bare" or "is-inside-git-dir" is
   true.  The intention is we want ls-files to be run inside the
   working tree.

 - Any other commands that says NOT_BARE does the same as
   ls-files, and the error message is "must be run in a work
   tree".

One option to solve this is to make core.bare have three possible
values, so the user can decide if he wants (1) or (2).
    core.bare = true: no working tree allowed
    core.bare = false: don't allow git-fetch to update HEAD etc.
    core.bare = mixed: allow both
In the third case is_bare_repository() would be true and
inside_work_tree() would change depending on cwd and GIT_WORK_TREE
setting.  This would allow git fetch to update the current branch even
while inside the working tree.

I am not sure if you even want the "no working tree allowed"
case. Doesn't it directly contradict with the purpose of the
whole GIT_WORK_TREE idea?

I did not look at the Porcelain-ish scripts, but for example in
the case of git-fetch, the reason it does not allow updating the
current branch is the same as "git-branch" above.

So I suspect that most of the existing use of is_bare is just an
implementation detail of "do we have a working tree?", and it
has been a good implementation primarily because we did not even
allow doing something like GIT_WORK_TREE to begin with.  They
need to be adjusted to work well with GIT_WORK_TREE.

quoted

I think the only thing you care about in your "where is the repo
and where is the worktree" codepath are get_git_dir() and
is_bare_repository().  As a side effect of calling these
functions for your own purpose, you later have to call
setup_git_env() again to clean up, which is fine.  I would feel
better if there is an assert in setup_git_env that catches the
case where it is called for the second time even though the
first caller was something other than this repository/worktree
discovery code.

You mean for the third time (first: git_get_dir() from git_config(),
second: setup_gdg, third: someone who should not call setup_git_env)?

I am more worried about a case where

 (1) a program calls some function A (perhaps in environment.c);

 (2) both of us are forgetting right now that A ends up calling
    setup_git_env();

 (3) the result from A is used to make decision by the calling
     program, and...

 (4) then the program calls setup_git_directory().

Currently the damange is limited as A cannot be outside
environment.c because setup_git_env() is static there, but
whatever the program decides in step (3) may need to be rolled
back and recomputed if setup_git_directory() ends up moving
GIT_DIR and friends around.

You mean something like assert(inside_work_tree >= 0); to check if
setup_gdg was called yet?  I didn't see such checks in the git source
yet but I like it, it is better to have an error message pointing at
the bug instead of strange behaviour which may result in a bug.

Yes, but I would rather have something stroner than assert()
that cannot be compiled away.  An explicit if (...) die("Gaah").