Re: Alternates corruption issue

On Tue, Jan 31, 2012 at 02:05:29PM +0000, Richard Purdie wrote:

I think the alternates thing is just a side effect. The real problem is that clone is picking the wrong repo to clone from! So a simple complete test would be something like:

    # make confusing bare repos
    git init --bare bare &&
    git init --bare bare.git &&

    # now make some commits; after this
    # segment, "bare" should have its tip
    # at commit "one", and "bare.git" should
    # be at "two".
    git init source &&
    (cd source &&
     echo content >file &&
     git add file &&
     git commit -m one &&
     git push ../bare HEAD &&
     echo content >>file &&
     git commit -a -m two &&
     git push ../bare.git HEAD
    ) &&

    # now try to clone bare and see which one we get;
    # we would expect the tip to be at "one", but it's not
    git clone bare clone &&
    cd clone &&
    echo one >expect &&
    git log -1 --format=%s >actual &&
    diff -u expect actual

    # we cloned the wrong repository!

Yeah, this seems brain-dead. It's good that git looks through a magic list to guess what I meant, but shouldn't the first thing it tries be the literal string that I gave, and only _then_ kick in the magic?

But even weirder, this doesn't match the lookup in other parts of git. I see four distinct places in git where we do repository lookup:

  1. get_repo_path used by clone (i.e., this)

  2. enter_repo used by upload-pack, receive-pack, and git-daemon; this
     looks first for ".git/.git", to find a non-bare repo which is named
     like a bare one.

  3. setup_git_directory_*, used by most git programs. This starts
     looking in the current directory and going up, looking first for
     ".git", and then checking for a bare repo. So if you were to do
     "cd bare && git foo", then we would correctly find the current
     directory, not "bare.git".

  4. setup_explicit_git_dir, which gets called if you use --git-dir or
     set GIT_DIR. This expects to find the repo directly in the
     directory you specified, with no magic.

So I think (3) and (4) are sensible, but I tend to think that (1) and (2) are wrong, and should prefer the directory you gave before the magic.

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

If you simply swap the last two, then there is one obscure case you would miss: a bare repo which contains a .git directory. Of course, that seems like an insane thing to have, but if you did have it, I would expect git to follow the rules I outlined above. That being said, if you follow the rules in setup_git_directory_gently_1, it actually prefers "/.git" to a bare repo. So probably your swap is better, as it matches the lookup here with the regular setup_git_directory one.

One slight complication is that get_repo_path only checks whether the paths exist, not whether they are actually git repositories. So by simply swapping the order, you are breaking some other obscure cases, like where a "foo" directory exists but is _not_ a git repo, and a "foo.git" exists alongside it (which sounds a bit crazy, but I could see somebody using that scheme if they wanted didn't want the git repository to live inside the worktree for some reason).

One way of dealing with that would be to make get_repo_path a little more robust by only selecting paths which actually look like git directories.

The patch below does this, and makes my test above pass. But I think before talking too much about code, it would be best to decide on what the lookup order _should_ be, and then we can write tests and implement to them.

-Peff

---