threads / bug / 28577

Git Bug report

Subject: Git Bug report

## tl;dr

15 messages between Oct 4, 2011 and Oct 6, 2011.

replies: 14people: 9as markdown or json

Federico Lucifredi· Oct 4, 2011, 21:24 UTC · lore
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.
Show 13 quoted lines
> 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
Johannes Sixt· Oct 5, 2011, 06:11 UTC · re: Federico Lucifredi · lore

Re: Git Bug report

Am 10/4/2011 23:24, schrieb Federico Lucifredi:
Show 9 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Oct 5, 2011, 08:17 UTC · re: Johannes Sixt · lore

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

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
Federico Lucifredi· Oct 5, 2011, 18:32 UTC · re: Johannes Sixt · lore

Re: Git Bug report

On Wed, 2011-10-05 at 08:11 +0200, Johannes Sixt wrote:
Show 6 quoted lines
> >> 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".

Show 10 quoted lines
> >> 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
Fredrik Gustafsson· Oct 5, 2011, 07:22 UTC · re: Federico Lucifredi · lore

Re: Git Bug report

On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote:
Show 8 quoted lines
>  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
Junio C Hamano· Oct 5, 2011, 16:49 UTC · re: Fredrik Gustafsson · lore

Re: Git Bug report

Fredrik Gustafsson <iveqy@iveqy.com> writes:
Show 16 quoted lines
> 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.

Nguyen Thai Ngoc Duy· Oct 5, 2011, 21:56 UTC · re: Junio C Hamano · lore

Re: Git Bug report

On Thu, Oct 6, 2011 at 3:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>> 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
SZEDER Gábor· Oct 6, 2011, 00:33 UTC · re: Junio C Hamano · lore

Re: Git Bug report

On Wed, Oct 05, 2011 at 09:49:00AM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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

Junio C Hamano· Oct 6, 2011, 00:44 UTC · re: SZEDER Gábor · lore

Re: Git Bug report

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.
SZEDER Gábor· Oct 6, 2011, 01:09 UTC · re: Junio C Hamano · lore

Re: Git Bug report

On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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...

Phil Hord· Oct 6, 2011, 16:48 UTC · re: SZEDER Gábor · lore

Re: Git Bug report

On 10/5/11, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Oct 6, 2011, 16:22 UTC · lore

Re: Git Bug report

Phil Hord <phil.hord@gmail.com> writes:
Show 19 quoted lines
> 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?
Matthieu Moy· Oct 6, 2011, 16:26 UTC · re: Junio C Hamano · lore

Re: Git Bug report

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
>  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/
Phil Hord· Oct 6, 2011, 16:54 UTC · re: Junio C Hamano · lore

Re: Git Bug report

On Thu, Oct 6, 2011 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> 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
Aaron Schrab· Oct 6, 2011, 22:57 UTC · re: Junio C Hamano · lore

Re: Git Bug report

At 09:22 -0700 06 Oct 2011, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
>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.

← back to recent threads