Git Bug report

Git Bug report

[Federico Lucifredi]

Hello Git list,
 Found a minor bug in git today - the error message reported is not
correct when trying to access a repo that is not accessible
permission-wise:

> federico@skyplex:/etc$ git log
> fatal: Not a git repository (or any of the parent directories): .git

with correct access permissions, everything works as expected.

> federico@skyplex:/etc$ sudo git log
> commit 10a1d0eefcc100a513a9dff46839cff2c4f9b5a0
> Author: root <root@skyplex>
> Date:   Mon Oct 3 16:53:33 2011 -0400
>
>    saving uncommitted changes in /etc prior to apt run
>
> commit 2abb2b397631c7f48757bbcb029ebc38e37659d6
> Author: federico <federico@skyplex>
> Date:   Mon Oct 3 16:50:16 2011 -0400
>
>    updating firefox packages next
>federico@skyplex:/etc$ 

> drwx------   8 root root      4096 2011-10-03 16:53 .git

That's it... I am not subscribed to the list, CC me in reply as needed.

Best -Federico

-- 

_________________________________________
-- "'Problem' is a bleak word for challenge" - Richard Fish
(Federico L. Lucifredi) - federico at canonical.com - GnuPG 0x4A73884C

Re: Git Bug report

[Johannes Sixt]

Am 10/4/2011 23:24, schrieb Federico Lucifredi:
> Hello Git list,
>  Found a minor bug in git today - the error message reported is not
> correct when trying to access a repo that is not accessible
> permission-wise:
> 
>> federico@skyplex:/etc$ git log
>> fatal: Not a git repository (or any of the parent directories): .git
> 
> with correct access permissions, everything works as expected.

And the correct error message is...?

>> drwx------   8 root root      4096 2011-10-03 16:53 .git

Assuming that you expected something like this:

fatal: .git: permission denied

it is hard to argue that a directory that happens to be named .git, but
was sealed by its owner should be assumed to be a git repository, albeit
one that we do not have access to. "Not a git repository" is an equally
justifyable error message, IMHO.

-- Hannes

Re: Git Bug report

[Fredrik Gustafsson]

On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote:
>  Found a minor bug in git today - the error message reported is not
> correct when trying to access a repo that is not accessible
> permission-wise:
> 
> > federico@skyplex:/etc$ git log
> > fatal: Not a git repository (or any of the parent directories): .git
> 
> with correct access permissions, everything works as expected.

So if:
.git/ is a directory with not enought permissions.
../.git/ is a directory with enought permissions.

git would today use ../.git. You suggest that git instead would die
because a .git/ exists? (I'm not saying this is wrong or right).

-- 
Med vänliga hälsningar
Fredrik Gustafsson

E-post: iveqy@iveqy.com
Tel. nr.: 0733 60 82 74

[PATCH] Report errors related to .git access during repository discovery

[Nguyễn Thái Ngọc Duy]

If $GIT_DIR is not given, we go up step by step and look for potential
repository directory, may see .git directory but for some reasons we
decide to skip and move on.

It's probably better to report along the line, so users can stop
wondering "hey, but I have .git directory _there_".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Wed, Oct 5, 2011 at 5:11 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
 > Am 10/4/2011 23:24, schrieb Federico Lucifredi:
 >> Hello Git list,
 >>  Found a minor bug in git today - the error message reported is not
 >> correct when trying to access a repo that is not accessible
 >> permission-wise:
 >>
 >>> federico@skyplex:/etc$ git log
 >>> fatal: Not a git repository (or any of the parent directories): .git
 >>
 >> with correct access permissions, everything works as expected.
 >
 > And the correct error message is...?

 That's a correct message. But it'd be even better if we help diagnose
 why. Even when you have proper access to .git dir, git can still
 refuse to accept the directory as a repository, because "HEAD" is
 invalid for example.

 I think a patch like this is an improvement. There may be many
 situations git refuses a directory but I don't cover here. Well, we
 may when users report them

 setup.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 27c1d47..b6028e5 100644
--- a/setup.c
+++ b/setup.c
@@ -269,6 +269,19 @@ const char *pathspec_prefix(const char *prefix, const char **pathspec)
 }
 
 /*
+ * This function is used during .git detection phase. If .git does not
+ * exist, it's OK not to report because that happens a lot if you stay
+ * inside a subdirectory and git checks every level back to topdir.
+ */
+static int access_and_warn(const char *path, int perm)
+{
+	int ret = access(path, perm);
+	if (ret && errno != ENOENT)
+		error("%s: %s", absolute_path(path), strerror(errno));
+	return ret;
+}
+
+/*
  * Test if it looks like we're at a git directory.
  * We want to see:
  *
@@ -288,22 +301,24 @@ static int is_git_directory(const char *suspect)
 		die("Too long path: %.*s", 60, suspect);
 	strcpy(path, suspect);
 	if (getenv(DB_ENVIRONMENT)) {
-		if (access(getenv(DB_ENVIRONMENT), X_OK))
+		if (access_and_warn(getenv(DB_ENVIRONMENT), X_OK))
 			return 0;
 	}
 	else {
 		strcpy(path + len, "/objects");
-		if (access(path, X_OK))
+		if (access_and_warn(path, X_OK))
 			return 0;
 	}
 
 	strcpy(path + len, "/refs");
-	if (access(path, X_OK))
+	if (access_and_warn(path, X_OK))
 		return 0;
 
 	strcpy(path + len, "/HEAD");
-	if (validate_headref(path))
+	if (validate_headref(path)) {
+		error("invalid HEAD at %s", absolute_path(path));
 		return 0;
+	}
 
 	return 1;
 }
-- 
1.7.3.1.256.g2539c.dirty

Re: Git Bug report

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote:
>>  Found a minor bug in git today - the error message reported is not
>> correct when trying to access a repo that is not accessible
>> permission-wise:
>> 
>> > federico@skyplex:/etc$ git log
>> > fatal: Not a git repository (or any of the parent directories): .git
>> 
>> with correct access permissions, everything works as expected.
>
> So if:
> .git/ is a directory with not enought permissions.
> ../.git/ is a directory with enought permissions.
>
> git would today use ../.git. You suggest that git instead would die
> because a .git/ exists? (I'm not saying this is wrong or right).

For that matter, if you have .git/ that is a directory but is not a
repository, and ../.git/ that is, the same situation would arise. I do not
think we should die because .git/ is not a git repository. I do not know
if we should even warn about it.

Re: Git Bug report

[Federico Lucifredi]

On Wed, 2011-10-05 at 08:11 +0200, Johannes Sixt wrote:
> >> federico@skyplex:/etc$ git log
> >> fatal: Not a git repository (or any of the parent directories): .git
> > 
> > with correct access permissions, everything works as expected.
> 
> And the correct error message is...?

".git: permission denied" -- no need to be fatal (there could
be ../.git, etc - fatal comes only if those don't exist). 

It could fail silently if one of the parents exists, but in the fatal
scenario, I should be told that it was by permission denied. Currently,
I am told there is no git repo where I know there to be one, which means
"what happened to my repo" is the next question in the user's head.
"it's there but it is broke" is the subtext one gets from this error. it
should be "it is there, I cannot get to it".

> >> drwx------   8 root root      4096 2011-10-03 16:53 .git
> 
> Assuming that you expected something like this:
> 
> fatal: .git: permission denied
> 
> it is hard to argue that a directory that happens to be named .git, but
> was sealed by its owner should be assumed to be a git repository, albeit
> one that we do not have access to. "Not a git repository" is an equally
> justifyable error message, IMHO.

An error message should help resolve the error in question, not grade
the user's smarts. Here I as a user had an expectation that there was a
git repository for /etc ("I set up one!") and figured the permission
issue with my own wits (well, I did not need to because it was /etc, but
in the general perm-denied case I would have had to), by looking at the
dir because the message gave me no useful information on the cause of
the problem.

Thanks -Federico
-- 

_________________________________________
-- "'Problem' is a bleak word for challenge" - Richard Fish
(Federico L. Lucifredi) - federico at canonical.com - GnuPG 0x4A73884C

Re: Git Bug report

[Nguyen Thai Ngoc Duy]

On Thu, Oct 6, 2011 at 3:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> So if:
>> .git/ is a directory with not enough permissions.
>> ../.git/ is a directory with enough permissions.
>>
>> git would today use ../.git. You suggest that git instead would die
>> because a .git/ exists? (I'm not saying this is wrong or right).
>
> For that matter, if you have .git/ that is a directory but is not a
> repository, and ../.git/ that is, the same situation would arise. I do not
> think we should die because .git/ is not a git repository. I do not know
> if we should even warn about it.

Probably not. On the other hand we should show user how we ignored
.git if we find no good repository in the end. So maybe it's a good
idea to queue up warnings and only print before git calls die("Not a
repository").
-- 
Duy

Re: Git Bug report

[SZEDER Gábor]

On Wed, Oct 05, 2011 at 09:49:00AM -0700, Junio C Hamano wrote:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
> > On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote:
> >>  Found a minor bug in git today - the error message reported is not
> >> correct when trying to access a repo that is not accessible
> >> permission-wise:
> >> 
> >> > federico@skyplex:/etc$ git log
> >> > fatal: Not a git repository (or any of the parent directories): .git
> >> 
> >> with correct access permissions, everything works as expected.
> >
> > So if:
> > .git/ is a directory with not enought permissions.
> > ../.git/ is a directory with enought permissions.
> >
> > git would today use ../.git. You suggest that git instead would die
> > because a .git/ exists? (I'm not saying this is wrong or right).
> 
> For that matter, if you have .git/ that is a directory but is not a
> repository, and ../.git/ that is, the same situation would arise. I do not
> think we should die because .git/ is not a git repository. I do not know
> if we should even warn about it.

And what about unreadable .git files?

~/tmp/git/outside$ git init
Initialized empty Git repository in /home/szeder/tmp/git/outside/.git/
~/tmp/git/outside$ mkdir inside repo
~/tmp/git/outside$ cd inside/
~/tmp/git/outside/inside$ git init --separate-git-dir=../repo
Initialized empty Git repository in /home/szeder/tmp/git/outside/repo/
~/tmp/git/outside/inside$ git rev-parse --git-dir
/home/szeder/tmp/git/outside/repo
~/tmp/git/outside/inside$ chmod a-r .git
~/tmp/git/outside/inside$ git rev-parse --git-dir
fatal: Error opening '.git': Permission denied

Or a non-gitfile .git file?

~/tmp/git/outside/inside$ chmod a+r .git
~/tmp/git/outside/inside$ echo foo >.git
~/tmp/git/outside/inside$ git rev-parse --git-dir
fatal: Invalid gitfile format: .git

Shouldn't these also be ignored?


Best,
Gábor

Re: Git Bug report

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> And what about unreadable .git files?

Having then inside a working tree is so sick that I do not think it
deserves consideration.

Please don't troll immediately after a big release.

Re: Git Bug report

[SZEDER Gábor]

On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > And what about unreadable .git files?
> 
> Having then inside a working tree is so sick that I do not think it
> deserves consideration.

I'm not sure why is this any different than having a .git directory
that is not a repository inside a working tree.

> Please don't troll immediately after a big release.

I didn't mean to troll; it just happened that I came across this issue
this weekend while trying to optimize the bash completion code...

Re: Git Bug report

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> On Oct 5, 2011 9:14 PM, "SZEDER Gábor" <szeder@ira.uka.de> wrote:
>>
>> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
>> > SZEDER Gábor <szeder@ira.uka.de> writes:
>> >
>> > > And what about unreadable .git files?
>> >
>> > Having then inside a working tree is so sick that I do not think it
>> > deserves consideration.
>>
>> I'm not sure why is this any different than having a .git directory
>> that is not a repository inside a working tree.
>
> What should happen here? Ignore it and keep searching? Or fail?
>
> I just added some common gitfile detection code and I noticed that the
> oddball case now is the one that dies on error rather than continuing to
> search for alternate explanations.  I left the oddball behavior assuming it
> is desireable, but now you have me rethinking it.

Yeah, after thinking about it a bit more, whenever we see ".git" during
the upward discovery process, we should always warn if we know it is _not_
a GIT_DIR before looking for another ".git" at higher levels, as anything
in that directory cannot be added. If we cannot tell if it is or is not
a GIT_DIR, we should error out---the reason we cannot tell most likely is
because we cannot read it, and such a file, if it is not a GIT_DIR, cannot
be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR,
we cannot use it to record updates or inspect existing history.

How's that sound as a guideline?

Re: Git Bug report

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>  If we cannot tell if it is or is not
> a GIT_DIR, we should error out---the reason we cannot tell most likely is
> because we cannot read it, and such a file, if it is not a GIT_DIR, cannot
> be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR,
> we cannot use it to record updates or inspect existing history.

Plus, the user may have removed the permission on the .git directory by
mistake, and it would be very surprising behavior if git ran without
complaining using a higher level GIT_DIR (i.e. a more or less arbitrary
repo as far as the user is concerned ...)

> How's that sound as a guideline?

Sounds reasonable, yes.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Git Bug report

[Phil Hord]

On 10/5/11, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
>> SZEDER Gábor <szeder@ira.uka.de> writes:
>>
>> > And what about unreadable .git files?
>>
>> Having then inside a working tree is so sick that I do not think it
>> deserves consideration.
>
> I'm not sure why is this any different than having a .git directory
> that is not a repository inside a working tree.

What should happen here? Ignore it and keep searching? Or fail?

I just added some common gitfile detection code and I noticed that the
oddball case now is the one that dies on error rather than continuing
to search for alternate explanations. I left the oddball behavior
assuming it is desireable, but now you have me rethinking it.

Phil

Re: Git Bug report

[Phil Hord]

On Thu, Oct 6, 2011 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Oct 5, 2011 9:14 PM, "SZEDER Gábor" <szeder@ira.uka.de> wrote:
>>>
>>> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
>>> > SZEDER Gábor <szeder@ira.uka.de> writes:
>>> >
>>> > > And what about unreadable .git files?
>>> >
>>> > Having then inside a working tree is so sick that I do not think it
>>> > deserves consideration.
>>>
>>> I'm not sure why is this any different than having a .git directory
>>> that is not a repository inside a working tree.
>>
>> What should happen here? Ignore it and keep searching? Or fail?
>>
>> I just added some common gitfile detection code and I noticed that the
>> oddball case now is the one that dies on error rather than continuing to
>> search for alternate explanations.  I left the oddball behavior assuming it
>> is desireable, but now you have me rethinking it.
>
> Yeah, after thinking about it a bit more, whenever we see ".git" during
> the upward discovery process, we should always warn if we know it is _not_
> a GIT_DIR before looking for another ".git" at higher levels, as anything
> in that directory cannot be added. If we cannot tell if it is or is not
> a GIT_DIR, we should error out---the reason we cannot tell most likely is
> because we cannot read it, and such a file, if it is not a GIT_DIR, cannot
> be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR,
> we cannot use it to record updates or inspect existing history.
>
> How's that sound as a guideline?

Ok.  Three cases, then:

if .git is valid, we use it.
If .git is bogus, we warn about it and keep searching.
If .git is unverifiable (permissions, IO-fail, etc.), we die.

Phil

Re: Git Bug report

[Aaron Schrab]

At 09:22 -0700 06 Oct 2011, Junio C Hamano <gitster@pobox.com> wrote:
>Yeah, after thinking about it a bit more, whenever we see ".git" during
>the upward discovery process, we should always warn if we know it is _not_
>a GIT_DIR before looking for another ".git" at higher levels, as anything
>in that directory cannot be added. If we cannot tell if it is or is not
>a GIT_DIR, we should error out---the reason we cannot tell most likely is
>because we cannot read it, and such a file, if it is not a GIT_DIR, cannot
>be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR,
>we cannot use it to record updates or inspect existing history.

Yes, I think that sounds like a good idea.  That should also solve a 
related problem that I noticed while checking out the current behaviour.

Currently if the .git directory of a submodule is inaccessible running 
`git status` from anywhere in the parent repository (including within 
the submodule) will cause git to recursively call itself until enough 
resources are used to prevent further forking.  I then tried this with 
the patch from earlier in the thread applied, but with the call to 
error() changed to call die() instead.  With that change it quickly 
failed with a useful error message.