From: Junio C Hamano <junkio@cox.net>
Date: Sun, 31 Dec 2006 05:46:51 GMT
Subject: Re: [PATCH 3/4] Automatically detect a bare git repository.
Message-ID: <7v8xgok46s.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20061231052606.GA5722@spearce.org>

Shawn Pearce <spearce@spearce.org> writes:

> Junio C Hamano <junkio@cox.net> wrote:
> ...
>
> 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.

Ok.

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

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.