Re: [PATCH 3/4] Automatically detect a bare git repository.

[Shawn Pearce <spearce@spearce.org>]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> >  This is in response to Theodore Tso's email asking why 'git log'
> >  doesn't work in a bare repository.  Now it does.  :-)
> 
> Does it?

It did in my testing.  ;-)
 
> > @@ -160,36 +178,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  	 * to do any discovery, but we still do repository
> >  	 * validation.
> >  	 */
> > -	if (getenv(GIT_DIR_ENVIRONMENT)) {
> > -		char path[PATH_MAX];
> > -		int len = strlen(getenv(GIT_DIR_ENVIRONMENT));
> > -		if (sizeof(path) - 40 < len)
> > +	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
> > +	if (gitdirenv) {
> > +		if (PATH_MAX - 40 < strlen(gitdirenv))
> >  			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
> > -		memcpy(path, getenv(GIT_DIR_ENVIRONMENT), len);
> > -		
> > -		strcpy(path + len, "/refs");
> > -		if (access(path, X_OK))
> > -			goto bad_dir_environ;
> > -		strcpy(path + len, "/HEAD");
> > -		if (validate_symref(path))
> > -			goto bad_dir_environ;
> > -		if (getenv(DB_ENVIRONMENT)) {
> > -			if (access(getenv(DB_ENVIRONMENT), X_OK))
> > -				goto bad_dir_environ;
> > -		}
> > -		else {
> > -			strcpy(path + len, "/objects");
> > -			if (access(path, X_OK))
> > -				goto bad_dir_environ;
> > -		}
> > -		return NULL;
> > -	bad_dir_environ:
> > +		if (is_git_directory(gitdirenv))
> > +			return NULL;
> >  		if (nongit_ok) {
> >  			*nongit_ok = 1;
> >  			return NULL;
> >  		}
> 
> 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.

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").
 
> > @@ -197,11 +196,17 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  
> >  	offset = len = strlen(cwd);
> >  	for (;;) {
> > -		if (is_toplevel_directory())
> > +		if (is_git_directory(".git"))
> >  			break;
> >  		chdir("..");
> >  		do {
> >  			if (!offset) {
> > +				if (is_git_directory(cwd)) {
> > +					if (chdir(cwd))
> > +						die("Cannot come back to cwd");
> > +					setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
> > +					return NULL;
> > +				}
> >  				if (nongit_ok) {
> >  					if (chdir(cwd))
> >  						die("Cannot come back to cwd");
> 
> 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.

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.