patch, 4 partsReplace "GIT_DIR" with GIT_DIR_ENVIRONMENT.

We tend to use the nice constant GIT_DIR_ENVIRONMENT when we are referring to the "GIT_DIR" constant, but git.c didn't do so. Now it does.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Many users find it unfriendly that they can create a bare git repository easily with `git clone --bare` but are then unable to run simple commands like `git log` once they cd into that newly created bare repository. This occurs because we do not check to see if the current working directory is a git repository.

Instead of failing out with "fatal: Not a git repository" we should try to automatically detect if the current working directory is a bare repository and use that for GIT_DIR, and fail out only if that doesn't appear to be true.

We test the current working directory only after we have tried searching up the directory tree. This is to retain backwards compatibility with our previous behavior on the off chance that a user has a 'refs' and 'objects' subdirectories and a 'HEAD' file that looks like a symref, all stored within a repository's associated working directory.

This change also consolidates the validation logic between the case of GIT_DIR being supplied and GIT_DIR not being supplied, cleaning up the code.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is in response to Theodore Tso's email asking why 'git log'
 doesn't work in a bare repository.  Now it does.  :-)

 setup.c |   73 +++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 39 insertions(+), 34 deletions(-)

"Shawn O. Pearce" <spearce@spearce.org> writes:

Does it?

I think this is a good refactoring.

I do not think this is correct.

What happens when GIT_DIR is set, and nongit_ok is passed? Earlier code returned NULL after setting *nongit_ok so that the caller knows the environment points at a directory that is not yet a git repository control area.

I do not know what the new behaviour of this part of the code is trying to do. This is supposed to see if "." is the toplevel (equivalently, ".git" is the git_dir, in your implementation), otherwise chdir("..") repeatedly until it finds one, and the normal return condition is for the working directory of the process to be at the toplevel. So chdir(cwd) you introduced is obviously changing the behaviour.

The existing chdir(cwd) is for an error return -- when there was no directory that has ".git" even when you went all the way up to the root level, we give up and come back to where we started, only when the caller suspected that there was no git directory and is prepared to handle that case, which is signalled by us storing 1 to *nongit_ok.

Junio C Hamano <junkio@cox.net> wrote:

It did in my testing.  ;-)
 

The new code is correct, or at least does what the old code did.

If GIT_DIR is set we call is_git_directory(); if that returns 1 then the checks passed. In this case the old code returned NULL and ignored nongit_ok. We do the same in the new code.

If GIT_DIR is set and we fail is_git_directory() then one of the
checks failed.  In this case the old code jumped to bad_dir_env.
In the new code we don't return NULL and fall through into the if
nongit_ok testing, or into the die("Not a git repository").
 

No, its not.

We only bother to look to see if the original cwd (before we started chdir("..")'ing up) is a git directory if we did not find one during that chdir up loop. This means our current working directory is now "/" but we found a valid repository in cwd.

In this case we chdir back to the repository directory before returning back to the caller, as what's the point of being in "/" when running in a bare repository? Probably better to be in the repository directory itself.

One could argue that maybe we should run in "/", or in "/tmp" in this case as there is no working directory associated with this repository, but that argument is about the same as just saying we go back into the now discovered GIT_DIR.

Shawn Pearce <spearce@spearce.org> writes:

Ok.

Ah, "changing the behaviour" is the correct thing to do, because the code is now allowing a new fallback to allow the original directory to be a bare git repository, and the new chdir/setenv makes perfect sense. Thanks for clarification.

For a short time I wondered if this should be a fallback position, or we might want to make this the very first thing to check, but I think adding this as the last ditch fallback position as you did is much safer than making it the first thing to check.

Thanks.

On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:

Thanks for coding this up!   

If you do this, does this mean that we can also eliminate the global variable --bare, since git will be able to figure out we're in a bare repository all by itself?

						- Ted

Theodore Tso <tytso@mit.edu> wrote:

Probably not. `git --bare foo` is a Poreclain-ish level option which has been supported for several versions. Removing it may cause pain for users to retrain their fingers to just ignore it.

Besides `git --bare` can be used in cases where the bare Git repository is actually located within another repository's working directory and the automatic detection is picking up the other repository's metadata and not the bare repository's metadata.

So I think --bare is still here to stay for a while. The new automatic detection just offers slightly less surprise for users.

hoi :)

On Sat, Dec 30, 2006 at 11:30:19PM -0500, Shawn O. Pearce wrote:

Hmm, I have my dot files under GIT control so I can't use this mechanism to use bare GIT repositories under ~/git/*.git.

Perhaps we should test the current directory first, but check that it ends in .git?

Martin Waitz <tali@admingilde.org> wrote:

Whoops.  I assume you have your .gitignore populated with the other
files/directories in your home directory that shouldn't be under
Git control?
 

Junio already stated why that may not be a good idea. My original version of the patch did check the current directory first, but it could match a working directory which isn't really a Git repository but just looks sort of like one.

That wouldn't be backwards compatible, so I moved the current directory check to just before failure. That version is the one that I submitted, and that Junio accepted...

If the user tries to run a porcelainish command which requires a working directory in a bare repository they may get unexpected results which are difficult to predict and may differ from command to command.

Instead we should detect that the current repository is a bare repository and refuse to run the command there, as there is no working directory associated with it.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is more of an RFC than an actual patch.  I think its a good
 idea (clearly, as I spent the time to write it) but this may break
 users who explicitly set GIT_DIR to some path which doesn't end in
 "/.git" (e.g. GIT_DIR=$HOME/foo.git).

 I've tried to only alter poreclainish and leave plumbing alone,
 under the rationale that a different Porcelain may have different
 behavior with regards to GIT_DIR.

 git-am.sh       |    1 +
 git-checkout.sh |    1 +
 git-clean.sh    |    1 +
 git-commit.sh   |    1 +
 git-merge.sh    |    1 +
 git-pull.sh     |    1 +
 git-rebase.sh   |    1 +
 git-reset.sh    |    1 +
 git-revert.sh   |    1 +
 git-sh-setup.sh |    7 +++++++
 git.c           |   11 +++++++----
 11 files changed, 23 insertions(+), 4 deletions(-)

I think the intent is good.

But execution has room for improvement, and it is not your fault. The shell script version of require_not_bare is as fragile as is_bare_git_dir(), which I already do not like, and I think if we are going to use is_bare_git_dir() more, I think we would want to have something a bit more robust.

If we could outlaw $GIT_DIR/index in a bare repository, then lack of $GIT_DIR/index combined with nonexistence of ref that is pointed at by $GIT_DIR/HEAD could become a good indication that the repository is bare ("the current branch unborn" check is needed not to mistake a repository before the initial commit as a bare one).

Alas, many public repositories you would see (e.g. check kernel.org) have been primed with rsync of .git/ from developer's working repository and have leftover index that is otherwise unused. Because of this heavy historical baggage, I suspect that it is rather hard to convince people to allow us to use this technique.

When you have $GIT_DIR in your environment, no working-tree command is expected to work unless you are at the toplevel of the working-tree. In the past, people talked about their workflows using more than one working trees that are associated with a single $GIT_DIR and that is certainly supposed to work. But I wonder how widely such a set-up is employed in practice. If we outlawed working-tree commands when $GIT_DIR environment exists, how much hurt are we talking about, I wonder.

Another thing to think about is if we are happy with the above restriction that makes environment $GIT_DIR to imply you are always working at the toplevel. Maybe it could be a good idea to kill this bird as well by introducing $GIT_WORKTREE_TOP environment variable. Presence of it obviously means we are not in a bare repository, but at the same time it would allow us to teach setup_git_directory_gentry() to cd up to that directory to make the commands behave as expected.

Junio C Hamano <junkio@cox.net> wrote:

Agreed.  But as you point out...
 

I almost coded the require_not_bare() to look for $GIT_DIR/index but didn't for the reasons you point out above. It is too bad that we didn't enforce the existance of the index file better in the past.

What happens with $GIT_DIR/index in this case? Or $GIT_DIR/HEAD? Its insane because both need to be affiliated with the working directory, yet there's only the one set. Sure the user could also arrange for $GIT_INDEX_FILE to be set based on the working directory, but what about $GIT_DIR/HEAD? There is no equivilant.

I wouldn't be hurt, but I don't call Porcelain-ish unless I'm entering commands directly on the command line, and I never set GIT_DIR except in scripts, and even then its very rare and is more to point at a bare repository than one with a working directory. I suspect that probably isn't true for everyone.

I'm not sure what value that environment variable offers here.

Users who set GIT_DIR but don't today set GIT_WORKTREE_TOP are broken by this change because its not set and we assume its bare. So they need to also set this environment variable. But that is probably going to be annoying for those users as I doubt they are setting GIT_DIR on a per-directory basis.

Why not just tell these users to setup the working directories with local .git directories and not use GIT_DIR?

"Shawn O. Pearce" <spearce@spearce.org> writes:

This, and this,...

suggest that we might want to bite the bullet and declare that these things are not supported anymore in v1.5.0.

So far, especially after fixing the i18n issue in response to qgit problem report, I do not think we have any serious backward incompatibility that the users cannot opt out of that we need to mention in v1.5.0 announcement. Even not the incompatibilities the 'old news' section talks about are.

I think an index file in a bare repository is not what the user wanted to have because it was useful, but is left there because it does not hurt. So in that sense, I do not think we need to ask users' permission to do this change, as long as we make sure we give them enough advance warning.

I do not offhand think of a valid workflow that relies on setting GIT_DIR in the environment and being able to run working-tree commands, that cannot be worked around if we declar that GIT_DIR is for our internal use (and no, naming the directory other people call .git to .svn does not count as a valid workflow), but I think this may be a bit more serious change than the index file one.

Comments?

On Sun, Dec 31, 2006 at 12:01:01AM -0800, Junio C Hamano wrote:

While we're talking about potentially deprecating GIT_DIR for users, out of curiosity, what valid workflows would cause users to want to use GIT_INDEX_FILE and GIT_OBJECT_DIRECTORY? Seems like they would cause more confusion and support problems than anything else.

						- Ted

Hi,

On Sun, 31 Dec 2006, Theodore Tso wrote:

Easy, guys.

It is a valid -- indeed, useful -- thing to be able to script nice helpers. For example, in one project I track tar balls. So, I wrote a script which will unpack the tar ball in a directory, build a new index from it, and commit the corresponding tree on top of the tracking branch. This setup relies _heavily_ on being able to redirect GIT_INDEX_FILE and GIT_DIR.

So, these things are _useful_. Please don't break them.

As for the presence of "index" in a bare repo: I think this is not a problem, _as long_ as things continue to work as before, i.e.

	$ GIT_DIR=$(pwd) git log

and

	$ git --bare log

do _not_ complain if "index" is there. Now, if somebody starts git in a bare repo, where "index" is present, it could die with a helpful message like

	It seems that this is a bare git repository, but there is an index 
	file present, which contradicts that assumption. If the repository
	is indeed bare, please remove the index file.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I do agree INDEX_FILE and OBJECT_DIRECTORY are handy things for the user to muck around. What I am not sure about is GIT_DIR, in the sense that I suspect it is not such a pain to do without for such a script.

That is probably worse.  

 * there is no reason for non working-tree operations such as
   git-log to fail when you go to a bare repository (or for that
   matter .git in a repository with a working-tree).  we should
   not have to error out nor remove the index we will not use.

 * if you did the above in response to a misguided 'git
   checkout' in a bare repository, the next error message the
   user will get will be 'huh?  you are in a bare repository,
   bozo'.

So I do not think the helpful message should even be necessary.

Hi,

On Sun, 31 Dec 2006, Junio C Hamano wrote:

Uhm. In the example I illustrated, I need to set GIT_DIR, because I want to commit a new version into a branch of my current repository, but without touching the working tree and the index.

Thus, I have to create a temporary directory, unpack the new (upstream) version, set GIT_DIR _and_ GIT_INDEX_FILE, do a write-tree and a commit-tree with some automatic message, and then update the ref. Yes, I could separate the steps, but why make it harder than it needs be?

Yeah, I meant this message only to appear when you call a program which needs a working directory. But I agree that this is probably stupid.

Ciao, Dscho