Re: Alternates corruption issue

From: Jeff King <peff@peff.net>
Date: Jan 31, 2012, 20:44 UTC
Message-ID: <20120131204417.GA30969@sigill.intra.peff.net>
In-Reply-To: <7v1uqf8vqu.fsf@alter.siamese.dyndns.org>

On Tue, Jan 31, 2012 at 12:25:45PM -0800, Junio C Hamano wrote:

Show 25 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I believe that would work in your case, but it seems like the most
> > correct thing would actually be:
> >
> >   { "", "/.git", ".git" }
> >
> > That is:
> >
> >   1. Try the literal path the user gave as a repo
> >
> >   2. Otherwise, try it as the root of a working tree (containing .git)
> >
> >   3. Otherwise, assume they were too lazy to type ".git" and include it
> 
> That sounds sensible, together with this, to which I agree with:
> [...]
> ... but ...
> [...]
> > -	static char *suffix[] = { "/.git", ".git", "" };
> > +	static char *suffix[] = { "/.git", "", ".git" };
> 
> ... this does not match that simple and clear guideline.
> 
> Shouldn't this simply be { "", "/.git", ".git" }?

No, it does not match. While the sequence I outlined above makes the most sense to me, it does not match what setup_git_directory does, which prefers "foo/.git" to using "foo" as a bare repo. I think being consistent between all of the lookup points makes sense. The patch took the least-invasive approach and aligned clone and enter_repo with setup_git_directory.

However, we could also tweak setup_git_directory to prefer bare repos over ".git" to keep things consistent. While it makes me feel good from a theoretical standpoint (because the rules above seem simple and intuitive to me), I'm not sure it's a good idea in practice.

The case we would "catch" with such a change is when you have a ".git" directory inside a bare repo. Right now, we prefer the ".git" inside it, and ignore the containing bare repo. With such a change, we would prefer the outer bare repo. Which makes sense to me. It does break a use case like this:

  # make a new repo
  git init
  cd .git
  # now track parts of the repo
  git init
  git add config
  git commit -m 'tracking our repo config'

but I'm not sure if that is sane or not.

But also consider false positives. What if you have a repository that looks like a git repo (i.e., has "objects", "refs", and "HEAD" in it), but also has ".git". Right now we say "Oh, it has .git, that must be the repo". But with the proposed change, we could accidentally find the enclosing repo.

Now, the chances of is_git_directory being wrong seem quite slim to me. But then, the chances of somebody actually have a repo with a ".git" _inside_ it seem pretty slim to me. So I think we are really dealing with a tiny corner case, and it is perhaps anybody's guess whether anyone is depending on the current behavior in the wild. I don't overly care either way, but when in doubt, I tend to stick with the existing behavior.

Show 9 quoted lines

> >  	if (!strict) {
> >  		static const char *suffix[] = {
> > -			".git/.git", "/.git", ".git", "", NULL,
> > +			"/.git", "", ".git/.git", ".git", NULL,
> >  		};
> 
> Neither does this.
> 
> Shouldn't this be { "", "/.git", ".git", ".git/.git", NULL }?

Right, same case.

> I must be missing something from your description...

I mentioned the issue in my original message, but perhaps didn't emphasize it very well.

-Peff