From: Junio C Hamano <hidden> Date: 2016-06-15 22:52:54
Jeff King [off-list ref] 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:
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.
From: Jeff King <hidden> Date: 2016-06-15 22:52:54
On Tue, Jan 31, 2012 at 12:25:45PM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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 ...
[...]
... 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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:54
Jeff King wrote:
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.
Wait, don't these two functions serve two completely different purposes?
One is the implementation of (A):
cd foo
git rev-parse --git-dir
The other implements (B):
git ls-remote foo
If "foo" is actually a bare repository that moonlights as a worktree for
a non-bare repository, then:
1) Whoever set up these directories is completely insane[*]. Maybe we
should emit a warning.
2) As a naive user, I would expect (A) to give a different result
from (B).
Hope that helps,
Jonathan
[*] ok, ok, they can be confused instead of insane:
http://bugs.debian.org/399041
From: Jeff King <hidden> Date: 2016-06-15 22:52:54
On Tue, Jan 31, 2012 at 03:40:47PM -0600, Jonathan Nieder wrote:
Jeff King wrote:
quoted
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.
Wait, don't these two functions serve two completely different purposes?
Yes, but I would expect the lookup to be the same.
One is the implementation of (A):
cd foo
git rev-parse --git-dir
The other implements (B):
git ls-remote foo
Right. But would you expect:
git ls-remote foo
to behave the same as:
cd foo
git ls-remote .
and would you furthermore expect that to behave the same as:
cd foo
git rev-parse --git-dir
?
Maybe I am crazy, but I see all of them as ways of specifying "I am
interested in repository foo", and any lookup magic should be the same
in all cases.
If "foo" is actually a bare repository that moonlights as a worktree for
a non-bare repository, then:
1) Whoever set up these directories is completely insane[*]. Maybe we
should emit a warning.
Yes, I think we are dealing with an insane corner case.
2) As a naive user, I would expect (A) to give a different result
from (B).
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:54
Jeff King wrote:
Right. But would you expect:
git ls-remote foo
to behave the same as:
cd foo
git ls-remote .
and would you furthermore expect that to behave the same as:
cd foo
git rev-parse --git-dir
?
Nah. I never run "git ls-remote .". ;-)
[...]
quoted
2) As a naive user, I would expect (A) to give a different result
from (B).
Why?
Normally git commands expect me to be in (possibly a deeply nested
subdirectory) of the worktree. The typical case is a non-bare
repository. I have been taught that it walks to the toplevel and
finds a ".git" directory.
By contrast, the path passed to git transport commands like "git fetch
otherhost:/foo/bar/baz.git" is a path to the git repository metadata.
I am not allowed to pass a path to a subdirectory, for example.
As a user, when would the distinction ever come up? One is a fuzzy
"which repository am I working in", and the other is a precise "point
me to the metadata directory, but I allow you to abbreviate a little
by leaving out the trailing .git".
From: Jeff King <hidden> Date: 2016-06-15 22:52:54
On Tue, Jan 31, 2012 at 03:55:01PM -0600, Jonathan Nieder wrote:
quoted
quoted
2) As a naive user, I would expect (A) to give a different result
from (B).
Why?
Normally git commands expect me to be in (possibly a deeply nested
subdirectory) of the worktree. The typical case is a non-bare
repository. I have been taught that it walks to the toplevel and
finds a ".git" directory.
By contrast, the path passed to git transport commands like "git fetch
otherhost:/foo/bar/baz.git" is a path to the git repository metadata.
I am not allowed to pass a path to a subdirectory, for example.
True. But I consider that to make the walk-backwards-from-pwd case
simply a superset. That is, in (A) we are walking backwards and trying
to apply the lookup rule from (B) individually to each directory we
consider (though even that is not entirely true, as we don't look for
parallel "$PWD.git" directories in the walk).
I'll admit I don't care that much, though. This is extremely unlikely to
come up. The real issue is fixing the fact that we prefer "foo.git" to
"foo", even when the user told us "foo". I am content to leave the rest
of it as-is, which is what my original patch did.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:54
Jeff King wrote:
True. But I consider that to make the walk-backwards-from-pwd case
simply a superset. That is, in (A) we are walking backwards and trying
to apply the lookup rule from (B) individually to each directory we
consider (though even that is not entirely true, as we don't look for
parallel "$PWD.git" directories in the walk).
That parenthesis is important. Although in a nicer world maybe we
would want some symmetry like this, (A) and (B) really have nothing to
do with each other.
Forgetting the algorithms:
(A) means "find what repository we are in the worktree for, or,
barring that, what bare repository we are in".
(B) means "find which repository the user pointed to. To be extra
nice, we allow a '.git' extension to be left out, so the URL used
doesn't have to include the redundant information that this is a
git repository, and we even allow pointing to the toplevel of a
worktree instead of a repository, too."
Notice that the above description does not exactly match the actual
behavior of git. For example, if someone has a directory layout like
this:
repo-manipulator/
.git/
src/
testcases/
repo1.git/
repo2.git/
and git is run from repo1.git, according to the description above, the
naive user _wanted_ git commands to apply to the toplevel repository.
And in practice, I think that's often true, though changing the
behavior of git to match that would not be worth the downsides.
I'll admit I don't care that much, though. This is extremely unlikely to
come up.
I admit part of the reason I care is that just putting "" first would
probably taken care of the more important part of
<http://bugs.debian.org/399041>.
Anyway, thanks for explaining. Hopefully I can get to this soon and
factor out a common function for get_repo_path and enter_repo to call
so playing with the ordering becomes a little less scary. ;-)
Jonathan
From: Jeff King <hidden> Date: 2016-06-15 22:52:54
On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:
Forgetting the algorithms:
(A) means "find what repository we are in the worktree for, or,
barring that, what bare repository we are in".
(B) means "find which repository the user pointed to. To be extra
nice, we allow a '.git' extension to be left out, so the URL used
doesn't have to include the redundant information that this is a
git repository, and we even allow pointing to the toplevel of a
worktree instead of a repository, too."
I can see that as a plausible interpretation. But I think your "means"
is kind of vague. In other words, we are assigning a meaning that is
not actually documented anywhere, and which we are hoping matches user
expectations. So the correct behavior kind of depends on your mental
model. IOW, I find your mental model plausible, but it is not the one
that I had.
But like I said, I'm OK either way. I'll prepare this fix with the least
behavior change possible. Then doing your change on top should be a
trivial rearrangement of the lookup arrays (the "hard" part for your
proposed change is adding in the is_git_directory() calls[1], but I'll
do that as part of mine).
I admit part of the reason I care is that just putting "" first would
probably taken care of the more important part of
<http://bugs.debian.org/399041>.
Would that fix it? If I understand it, the repo in question is bare with
a ".git" directory inside it. Your proposed change would make "git
ls-remote foo" work, but wouldn't "cd foo && git log" still be broken?
At the very least, it would show a completely different repo than
ls-remote.
-Peff
[1] Without the is_git_directory() check, the ordering you propose
breaks things very badly. Because if I point to a non-bare directory
"foo", we say "OK, foo exists" and never look at "foo/.git". So
instead, we have to say "OK, foo exists, but hey, it looks nothing
like a .git directory" and keep trying.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:52:54
Jeff King wrote:
On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:
quoted
I admit part of the reason I care is that just putting "" first would
probably taken care of the more important part of
<http://bugs.debian.org/399041>.
Would that fix it? If I understand it, the repo in question is bare with
a ".git" directory inside it.
The layout was foo/.git/.git. The real repository was made with plain
"git init" or "git clone", and then "git svn init" or similar was run
from within .git, creating a .git subdirectory. (Something more must
have happened, actually, since the tree was weirdly sparse:
$ find .git/.git
.git/.git
.git/.git/svn
.git/.git/svn/git-svn
.git/.git/svn/git-svn/.rev_db
)
In the current scheme, to access such a broken repository remotely,
you have to use a URL ".../foo". URLs pointing to foo/.git (like gitk
once used) end up rewritten as .git/.git. Of course, your patch with
the is_git_directory() will fix this specific case. :)
Not a huge deal, especially since "git svn init" prints out where it
is writing these days.
From: Jeff King <hidden> Date: 2016-06-15 22:52:55
On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:
Anyway, thanks for explaining. Hopefully I can get to this soon and
factor out a common function for get_repo_path and enter_repo to call
so playing with the ordering becomes a little less scary. ;-)
So here's what I think we should apply to fix the particular issue that
Richard mentioned at the start of this thread.
Besides tweaking the ordering, the main contribution is a set of tests
that actually check some of these ambiguous cases (especially checking
the fact that both code paths behave identically!). I didn't factor the
logic into a common function, but doing so should be a little safer on
top of these tests, if you're still interested.
-- >8 --
Subject: standardize and improve lookup rules for external local repos
When you specify a local repository on the command line of
clone, ls-remote, upload-pack, receive-pack, or upload-archive,
or in a request to git-daemon, we perform a little bit of
lookup magic, doing things like looking in working trees for
.git directories and appending ".git" for bare repos.
For clone, this magic happens in get_repo_path. For
everything else, it happens in enter_repo. In both cases,
there are some ambiguous or confusing cases that aren't
handled well, and there is one case that is not handled the
same by both methods.
This patch tries to provide (and test!) standard, sensible
lookup rules for both code paths. The intended changes are:
1. When looking up "foo", we have always preferred
a working tree "foo" (containing "foo/.git" over the
bare "foo.git". But we did not prefer a bare "foo" over
"foo.git". With this patch, we do so.
2. We would select directories that existed but didn't
actually look like git repositories. With this patch,
we make sure a selected directory looks like a git
repo. Not only is this more sensible in general, but it
will help anybody who is negatively affected by change
(1) negatively (e.g., if they had "foo.git" next to its
separate work tree "foo", and expect to keep finding
"foo.git" when they reference "foo").
3. The enter_repo code path would, given "foo", look for
"foo.git/.git" (i.e., do the ".git" append magic even
for a repo with working tree). The clone code path did
not; with this patch, they now behave the same.
In the unlikely case of a working tree overlaying a bare
repo (i.e., a ".git" directory _inside_ a bare repo), we
continue to treat it as a working tree (prefering the
"inner" .git over the bare repo). This is mainly because the
combination seems nonsensical, and I'd rather stick with
existing behavior on the off chance that somebody is relying
on it.
Signed-off-by: Jeff King <redacted>
---
I could go either way with the "prefer foo to foo/.git" thing we've been
discussing. I left it as-is here out of a sense of being conservative.
But I have no objection at all to you doing a patch on top that
justifies the change. You'll just need to tweak the "check" line of the
final test.
builtin/clone.c | 4 +-
cache.h | 1 +
path.c | 7 ++-
setup.c | 2 +-
t/t5900-repo-selection.sh | 100 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 109 insertions(+), 5 deletions(-)
create mode 100755 t/t5900-repo-selection.sh
@@ -0,0 +1,100 @@+#!/bin/sh++test_description='selecting remote repo in ambiguous cases'+../test-lib.sh++reset(){+rm-rffoofoo.gitfetchclone+}++make_tree(){+gitinit"$1"&&+(cd"$1"&&test_commit"$1")+}++make_bare(){+gitinit--bare"$1"&&+(cd"$1"&&+tree=`githash-object-w-ttree/dev/null`&&+commit=$(echo"$1"|gitcommit-tree$tree)&&+gitupdate-refHEAD$commit+)+}++get(){+gitinit--barefetch&&+(cdfetch&&gitfetch"../$1")&&+gitclone"$1"clone+}++check(){+echo"$1">expect&&+(cdfetch&&gitlog-1--format=%sFETCH_HEAD)>actual.fetch&&+(cdclone&&gitlog-1--format=%sHEAD)>actual.clone&&+test_cmpexpectactual.fetch&&+test_cmpexpectactual.clone+}++test_expect_success'find .git dir in worktree''+reset&&+make_treefoo&&+getfoo&&+checkfoo+'++test_expect_success'automagically add .git suffix''+reset&&+make_barefoo.git&&+getfoo&&+checkfoo.git+'++test_expect_success'automagically add .git suffix to worktree''+reset&&+make_treefoo.git&&+getfoo&&+checkfoo.git+'++test_expect_success'prefer worktree foo over bare foo.git''+reset&&+make_treefoo&&+make_barefoo.git&&+getfoo&&+checkfoo+'++test_expect_success'prefer bare foo over bare foo.git''+reset&&+make_barefoo&&+make_barefoo.git&&+getfoo&&+checkfoo+'++test_expect_success'disambiguate with full foo.git''+reset&&+make_barefoo&&+make_barefoo.git&&+getfoo.git&&+checkfoo.git+'++test_expect_success'we are not fooled by non-git foo directory''+reset&&+make_barefoo.git&&+mkdirfoo&&+getfoo&&+checkfoo.git+'++test_expect_success'prefer inner .git over outer bare''+reset&&+make_treefoo&&+make_barefoo.git&&+mvfoo/.gitfoo.git&&+getfoo.git&&+checkfoo+'++test_done
From: Richard Purdie <hidden> Date: 2016-06-15 22:52:56
On Thu, 2012-02-02 at 16:59 -0500, Jeff King wrote:
On Tue, Jan 31, 2012 at 04:22:58PM -0600, Jonathan Nieder wrote:
quoted
Anyway, thanks for explaining. Hopefully I can get to this soon and
factor out a common function for get_repo_path and enter_repo to call
so playing with the ordering becomes a little less scary. ;-)
So here's what I think we should apply to fix the particular issue that
Richard mentioned at the start of this thread.
Besides tweaking the ordering, the main contribution is a set of tests
that actually check some of these ambiguous cases (especially checking
the fact that both code paths behave identically!). I didn't factor the
logic into a common function, but doing so should be a little safer on
top of these tests, if you're still interested.
I didn't have much to add to the discussion yesterday but this solution
looks good to me and should resolve the problems I was seeing.
Thanks!
Richard