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

From: Junio C Hamano <junkio@cox.net>
Date: Apr 11, 2007, 01:29 UTC
Message-ID: <7vzm5fr954.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070406132109.GA5682@moooo.ath.cx>

Matthias Lederhofer <matled@gmx.net> writes:

Show 13 quoted lines

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

Show 9 quoted lines

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

Show 12 quoted lines

>> 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").