permissions

permissions

[William Pursell]

If .git is not readable, but ../.git is, .git works with
the database in ../.git.  Is that the desired behavior?
It would seem more appropriate that git fail with a
"permission denied" error.  In particular, if .git
is not readable and there is no database between
$PWD and $GIT_CEILING_DIRECTORIES, the current error is:
fatal: Not a git repository (or any of the parent directories): .git

Wouldn't it be better to have the error message be something like:
.git: permission denied

After all, it is a git repository, so "Not a git repository"
is not accurate.

-- 
William Pursell

Re: permissions

[Andreas Schwab]

William Pursell <bill.pursell@gmail.com> writes:

> After all, it is a git repository, so "Not a git repository"
> is not accurate.

A valid git repository requires more than a .git directory.

$ git init
Initialized empty Git repository in /tmp/x/.git/
$ rm .git/HEAD 
$ git status
fatal: Not a git repository (or any of the parent directories): .git

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: permissions

[William Pursell]

Andreas Schwab wrote:
> William Pursell <bill.pursell@gmail.com> writes:
> 
>> After all, it is a git repository, so "Not a git repository"
>> is not accurate.
> 
> A valid git repository requires more than a .git directory.

True, but this doesn't address the issue.  In the example
you gave, the error message is accurate.  Consider this:

$ sudo sh -c 'umask 077; git init'
Initialized empty Git repository in /private/tmp/foo/.git/
$ git rev-parse --git-dir
fatal: Not a git repository (or any of the parent directories): .git

That's just weird.  And if there is a git repository in a
directory above, there may be great confusion, weeping
and gnashing of teeth.


-- 
William Pursell

Re: permissions

[Alex Riesen]

On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:
> fatal: Not a git repository (or any of the parent directories): .git
>
> That's just weird.  And if there is a git repository in a
> directory above, there may be great confusion, weeping
> and gnashing of teeth.

How about just this? (I assume cwd does hold current working directory).

diff --git a/setup.c b/setup.c
index 5a083fa..561f3ab 100644
--- a/setup.c
+++ b/setup.c
@@ -428,7 +428,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				*nongit_ok = 1;
 				return NULL;
 			}
-			die("Not a git repository (or any of the parent directories): %s",
DEFAULT_GIT_DIR_ENVIRONMENT);
+			die("Not a git repository (or any of the parent directories): %s
(in %s)", DEFAULT_GIT_DIR_ENVIRONMENT, cwd);
 		}
 		if (one_filesystem) {
 			if (stat("..", &buf)) {

Re: permissions

[William Pursell]

Alex Riesen wrote:
> On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:
>> fatal: Not a git repository (or any of the parent directories): .git
>>
>> That's just weird.  And if there is a git repository in a
>> directory above, there may be great confusion, weeping
>> and gnashing of teeth.
> 
> How about just this? (I assume cwd does hold current working directory).

<patch snipped>

The problem is permissions, not that it's "not a git repository".
The error message should be "permission denied".  The easy solution
is to abort with "permission denied" whenever that is encountered,
but the trouble with that is that it breaks the current work flow
in which a broken dir (or one for which the user lacks
priveleges) is bypassed and a valid object directory higher
up in the filesystem tree is used.

Consider the case in which /etc/.git has mode 777 while
/etc/sysconfig/.git has mode 700, each .git owned by root.
(Granted, git is for recording development history and
not so much for storing history of config files, but
I believe this is a relevant use case.)  /etc/sysconfig/foo
is tracked by /etc/sysconfig/.git but not by /etc/.git.
Regular user in /etc/sysconfig invokes 'git log foo'
and is told: absolutely nothing.   And when
'git status' is invoked, the message is that foo is
untracked.

Now, if /etc/.git does track /etc/sysconfig/foo,
then a regular user in /etc/sysconfig that invokes
'git log foo' sees the history tracked in /etc/.git,
but root in /etc/sysconfig sees the history tracked
by /etc/sysconfig/.git.  This is confusing.  The regular
user in /etc/sysconfig should simply get 'permission denied'
on all invocations of git.

A related question is: does anyone actually prefer (or
rely on) the current model in which ../.git is
used in the event that .git is borked or the user
lacks permission?  It seems to me that if an
object directory is discovered which is borked
or which is unreadable, git must abort with an
error message indicating the relevant problem.

-- 
William Pursell

Re: permissions

[Alex Riesen]

On Sun, Jun 6, 2010 at 11:36, William Pursell <bill.pursell@gmail.com> wrote:
> Alex Riesen wrote:
>> On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:
>>> fatal: Not a git repository (or any of the parent directories): .git
>>>
>>> That's just weird.  And if there is a git repository in a
>>> directory above, there may be great confusion, weeping
>>> and gnashing of teeth.
>>
>> How about just this? (I assume cwd does hold current working directory).
>
> <patch snipped>
>
> The problem is permissions, not that it's "not a git repository".
> The error message should be "permission denied".

Well, isn't it enough to diagnose the problem? (where it lies, at least).

Re: permissions

[Junio C Hamano]

William Pursell <bill.pursell@gmail.com> writes:

> The problem is permissions, not that it's "not a git repository".
> The error message should be "permission denied".  The easy solution
> is to abort with "permission denied" whenever that is encountered,
> but the trouble with that is that it breaks the current work flow
> in which a broken dir (or one for which the user lacks
> priveleges) is bypassed and a valid object directory higher
> up in the filesystem tree is used.

I think it is sane to abort with "permission denied", as it is "not a git
repository" but it is "we cannot even determine if that .git we see is a
git repository, and if it is, then we cannot do any git operation here as
we cannot read it".  As to what you call "the current work flow", I think
it is not like we _support_ such usage, but more like it _happens to_ work
that way.

> A related question is: does anyone actually prefer (or rely on) the
> current model in which ../.git is used in the event that .git is borked
> or the user lacks permission?

So my answer to this is "nobody _should_", as it is not even "the current
model", but "it happens to behave like that by accident".  That doesn't
mean there isn't anybody who already does, though...

Re: permissions

[William Pursell]

Junio C Hamano wrote:

> I think it is sane to abort with "permission denied", as it is "not a git
> repository" but it is "we cannot even determine if that .git we see is a
> git repository, and if it is, then we cannot do any git operation here as
> we cannot read it".  As to what you call "the current work flow", I think
> it is not like we _support_ such usage, but more like it _happens to_ work
> that way.

Here's a patch.  This doesn't address the issue of a damaged
repository, but just catches access errors and permissions.

>From 8f1c8f4d572fe62a26d1fca47abc976e78942697 Mon Sep 17 00:00:00 2001
From: William Pursell <bill.pursell@gmail.com>
Date: Tue, 8 Jun 2010 00:16:43 -1000
Subject: [PATCH] Terminate on access errors

This changes the way git finds a repository.  Previously, if
access is denied to .git (or $GIT_DIR), git will use the object
directory in a higher level directory.  With this patch, git will
instead terminate and emit an error message indicating the access
failure.  Also, other errors (such as soft-link loops in
GIT_OBJECT_DIRECTORIES) will cause termination.

Signed-off-by: William Pursell <bill.pursell@gmail.com>
---
 setup.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/setup.c b/setup.c
index 7e04602..a53331c 100644
--- a/setup.c
+++ b/setup.c
@@ -155,6 +155,18 @@ const char **get_pathspec(const char *prefix, const char **pathspec)
 }

 /*
+ * Wrapper around access that terminates on
+ * errors other than ENOENT.
+ */
+static int xaccess(const char *path, int amode)
+{
+	int status = access(path, amode);
+	if (status && errno != ENOENT)
+		die_errno("%s", path);
+	return status;
+}
+
+/*
  * Test if it looks like we're at a git directory.
  * We want to see:
  *
@@ -172,17 +184,17 @@ static int is_git_directory(const char *suspect)

 	strcpy(path, suspect);
 	if (getenv(DB_ENVIRONMENT)) {
-		if (access(getenv(DB_ENVIRONMENT), X_OK))
+		if (xaccess(getenv(DB_ENVIRONMENT), X_OK))
 			return 0;
 	}
 	else {
 		strcpy(path + len, "/objects");
-		if (access(path, X_OK))
+		if (xaccess(path, X_OK))
 			return 0;
 	}

 	strcpy(path + len, "/refs");
-	if (access(path, X_OK))
+	if (xaccess(path, X_OK))
 		return 0;

 	strcpy(path + len, "/HEAD");
-- 
1.7.1.245.g7c42e.dirty



-- 
William Pursell

Re: permissions

[Alex Riesen]

On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:
> Here's a patch.  This doesn't address the issue of a damaged
> repository, but just catches access errors and permissions.

The change looks fishy.

The patch moves the function is_git_directory at the level of user
interface where it wasn't before: it now complains and die.
Not all callers of the function call it only to die if it fails.

Re: permissions

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:
>> Here's a patch.  This doesn't address the issue of a damaged
>> repository, but just catches access errors and permissions.
>
> The change looks fishy.
>
> The patch moves the function is_git_directory at the level of user
> interface where it wasn't before: it now complains and die.
> Not all callers of the function call it only to die if it fails.

Thanks for shooting it down before I had to look at it ;-)

Re: permissions

[William Pursell]

Junio C Hamano wrote:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:
>>> Here's a patch.  This doesn't address the issue of a damaged
>>> repository, but just catches access errors and permissions.
>> The change looks fishy.
>>
>> The patch moves the function is_git_directory at the level of user
>> interface where it wasn't before: it now complains and die.
>> Not all callers of the function call it only to die if it fails.
> 
> Thanks for shooting it down before I had to look at it ;-)

The point of the patch is that it now complains and dies.
Perhaps I'm being obtuse, but can you describe a situation
in which this causes git to terminate inappropriately?



-- 
William Pursell

Re: permissions

[Alex Riesen]

On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:
> Junio C Hamano wrote:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>>
>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:
>>>> Here's a patch.  This doesn't address the issue of a damaged
>>>> repository, but just catches access errors and permissions.
>>> The change looks fishy.
>>>
>>> The patch moves the function is_git_directory at the level of user
>>> interface where it wasn't before: it now complains and die.
>>> Not all callers of the function call it only to die if it fails.
>>
>> Thanks for shooting it down before I had to look at it ;-)
>
> The point of the patch is that it now complains and dies.

At wrong point. Points, actually. There are many callers of the
function you modified. You should have looked at them all.

> Perhaps I'm being obtuse, but can you describe a situation
> in which this causes git to terminate inappropriately?

Maybe. BTW, can you? (if you try, I mean). But your questions
misses the point of my complaint about your patch:

The patch makes the function you modified act not as one
can guess from its other uses. Imagine someone replaced
open(2) implementation to kill your program everytime you
tried to open /etc/passwd. How'd you like that?

That alone is reason enough to dislike the change and put
you personally into a list of persons to be careful with (as
you don't seem to care about what happens with the code
after you changed it).

Re: permissions

[William Pursell]

Alex Riesen wrote:
> On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:
>> Junio C Hamano wrote:
>>> Alex Riesen <raa.lkml@gmail.com> writes:
>>>
>>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:
>>>>> Here's a patch.  This doesn't address the issue of a damaged
>>>>> repository, but just catches access errors and permissions.
>>>> The change looks fishy.
>>>>
>>>> The patch moves the function is_git_directory at the level of user
>>>> interface where it wasn't before: it now complains and die.
>>>> Not all callers of the function call it only to die if it fails.
>>> Thanks for shooting it down before I had to look at it ;-)
>> The point of the patch is that it now complains and dies.
> 
> At wrong point. Points, actually. There are many callers of the
> function you modified. You should have looked at them all.

I did look at all 4 calls, and it seemed to me
that localizing the change in one location is a better
design than adding logic to 4 different locations.

>> Perhaps I'm being obtuse, but can you describe a situation
>> in which this causes git to terminate inappropriately?
> 
> Maybe. BTW, can you? (if you try, I mean).

No, I can't.  As far as I can tell, the patch adds
exactly the functionality that I want it to add.  You
do make good points about its problems below, however,
and you are right that I did miss the point of
your criticism.  Thank you for clarifying.

> But your questions
> misses the point of my complaint about your patch:
> 
> The patch makes the function you modified act not as one
> can guess from its other uses. Imagine someone replaced
> open(2) implementation to kill your program everytime you
> tried to open /etc/passwd. How'd you like that?

I think there is a substantial difference between changing
a basic library call and changing a statically linked
function called from only 4 locations, but I'll agree
that you have a valid point about the function not
behaving as expected.  The functionality I've added disagrees
with the name of the function, so on that point alone I will
agree that the patch is no good.

> 
> That alone is reason enough to dislike the change and put
> you personally into a list of persons to be careful with (as
> you don't seem to care about what happens with the code
> after you changed it).

I do care quite a lot actually.  My primary goal
was to minimize the changes, and it seemed that
is_git_directory() was the right place to make
the change with minimal impact.  Perhaps the following
patch would be more to your liking:


diff --git a/setup.c b/setup.c
index 7e04602..b25da21 100644
--- a/setup.c
+++ b/setup.c
@@ -303,6 +303,9 @@ const char *read_gitfile_gently(const char *path)
 		buf = dir;
 	}

+	if (access(dir, X_OK))
+		die_errno("Unable to access %s", dir);
+
 	if (!is_git_directory(dir))
 		die("Not a git repository: %s", dir);
 	path = make_absolute_path(dir);
@@ -370,6 +373,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			*nongit_ok = 1;
 			return NULL;
 		}
+		if (access(gitdirenv, X_OK))
+			die_errno("Unable to access %s", gitdirenv);
+
 		die("Not a git repository: '%s'", gitdirenv);
 	}

@@ -407,6 +413,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		}
 		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
 			break;
+		if (access(DEFAULT_GIT_DIR_ENVIRONMENT, X_OK)
+				 && errno != ENOENT )
+			die_errno("Unable to access %s/%s",
+				 cwd, DEFAULT_GIT_DIR_ENVIRONMENT);
+
 		if (is_git_directory(".")) {
 			inside_git_dir = 1;
 			if (!work_tree_env)
diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index cb14425..9f6f756 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -46,7 +46,7 @@ test_expect_success 'bad setup: invalid .git file path' '
 		echo "git rev-parse accepted an invalid .git file path"
 		false
 	fi &&
-	if ! grep "Not a git repository" .err
+	if ! grep "Unable to access $REAL.not" .err
 	then
 		echo "git rev-parse returned wrong error"
 		false

-- 
William Pursell

Re: permissions

[Steven Michalske]

On Jun 9, 2010, at 12:20 AM, Alex Riesen wrote:

> On Wed, Jun 9, 2010 at 00:27, William Pursell  
> <bill.pursell@gmail.com> wrote:
>> Junio C Hamano wrote:
>>> Alex Riesen <raa.lkml@gmail.com> writes:
>>>
>>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com 
>>>> > wrote:
>>>>> Here's a patch.  This doesn't address the issue of a damaged
>>>>> repository, but just catches access errors and permissions.
>>>> The change looks fishy.
>>>>
>>>> The patch moves the function is_git_directory at the level of user
>>>> interface where it wasn't before: it now complains and die.
>>>> Not all callers of the function call it only to die if it fails.
>>>
>>> Thanks for shooting it down before I had to look at it ;-)
>>
>> The point of the patch is that it now complains and dies.
>
> At wrong point. Points, actually. There are many callers of the
> function you modified. You should have looked at them all.
>
Should the other functions not fail in this case?  Looking at the uses  
3 in setup.c and not exported for use in other code.  Only once case  
looked like it could cause an issue would be if the file did not  
exist, and he excluded that case, lines 408 and 409 of setup.c  Where  
the environment variable is passed into the function.

Well that's a good question,  William made a valid assumption that if  
you were checking a directory  that is suspected to be a git directory  
and it couldn't be read you should let the user know that something is  
funky.  Now this looks like the right place for catching that access  
violation, but it looks like it night not the right place to report  
the error.....

So return another error code for catching it up stream.  But, we can't  
because this function can be true many ways and false only one.  This  
is where the shell convention that 0 is ok and errors are not 0 really  
shines, but doesn't work for the name of this function.



>> Perhaps I'm being obtuse, but can you describe a situation
>> in which this causes git to terminate inappropriately?
>
> Maybe. BTW, can you? (if you try, I mean). But your questions
> misses the point of my complaint about your patch:
>
And this point was not clearly explained on your part.... I had to  
read your complaint a few times to understand what you meant.

Something mentioned this way might have been more insightful.
The patch should pass error up to calling function and not terminate  
in the function.

And offer a suggestion of how you would prefer it to be implemented.

> The patch makes the function you modified act not as one
> can guess from its other uses. Imagine someone replaced
> open(2) implementation to kill your program everytime you
> tried to open /etc/passwd. How'd you like that?
>
Your analogy is subtly off.

Because if you tried to open /etc/passwd and could not open it for  
reading your application should fail.  That works because open allows  
for the return of an error code, but is_git_directory does not, so  
patching at the level that will correctly detect this erroneous case  
is difficult to report upstream.

> That alone is reason enough to dislike the change and put
> you personally into a list of persons to be careful with (as
> you don't seem to care about what happens with the code
> after you changed it).

Honestly it is not, he was asking what he did wrong, and probably  
didn't follow your line of reasoning.

Steve

Re: permissions

[Thomas Rast]

William Pursell wrote:
> Alex Riesen wrote:
> > On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:
> >> Junio C Hamano wrote:
> >>> Alex Riesen <raa.lkml@gmail.com> writes:
> >>>
> >>>> The patch moves the function is_git_directory at the level of user
> >>>> interface where it wasn't before: it now complains and die.
> >>>> Not all callers of the function call it only to die if it fails.
> >>> Thanks for shooting it down before I had to look at it ;-)
> >> The point of the patch is that it now complains and dies.
> > 
> > At wrong point. Points, actually. There are many callers of the
> > function you modified. You should have looked at them all.
> 
> I did look at all 4 calls, and it seemed to me
> that localizing the change in one location is a better
> design than adding logic to 4 different locations.
> 
> >> Perhaps I'm being obtuse, but can you describe a situation
> >> in which this causes git to terminate inappropriately?
> > 
> > Maybe. BTW, can you? (if you try, I mean).
> 
> No, I can't.  As far as I can tell, the patch adds
> exactly the functionality that I want it to add.  You
> do make good points about its problems below, however,
> and you are right that I did miss the point of
> your criticism.  Thank you for clarifying.

Maybe I'm missing something, but I think that also apart from any
meta-criticism the patch is wrong.  From the use of
setup_git_directory_gently() in cmd_apply() [for example; there are
other commands that are supposed to work both in- and outside of
repos], I conclude that the invocation of is_git_directory() must not
die() because it is *okay* if the directory is, after all, not a git
repo.

And I think the same goes for your new patch

> @@ -407,6 +413,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                 }
>                 if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
>                         break;
> +               if (access(DEFAULT_GIT_DIR_ENVIRONMENT, X_OK)
> +                                && errno != ENOENT )
> +                       die_errno("Unable to access %s/%s",
> +                                cwd, DEFAULT_GIT_DIR_ENVIRONMENT);
> +
>                 if (is_git_directory(".")) {
>                         inside_git_dir = 1;
>                         if (!work_tree_env)

[DEFAULT_GIT_DIR_ENVIRONMENT is ".git"]

Unless I'm missing something, this effectively prevents git-apply and
friends from working outside any repos if your BOFH sysadmin thinks it
funny to place an unreadable .git somewhere on the way up to /.

Or maybe we don't care about BOFH ideas?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: permissions

[Alex Riesen]

On Wed, Jun 9, 2010 at 12:29, William Pursell <bill.pursell@gmail.com> wrote:
> I do care quite a lot actually.  My primary goal
> was to minimize the changes, and it seemed that
> is_git_directory() was the right place to make
> the change with minimal impact.

While smaller patches are preferred, they are not the goal, per say.

> Perhaps the following patch would be more to your liking:

Looks like a lot of effort just to get a little more information in
an infrequent (and frankly, obscure) failure case.
How about just use errno after failed is_git_directory? It seems
to make sense even when the function calls down to validate_headref.
You may even reset errno to 0 (this will also make obvious
the expectation of valid errno from is_git_directory).

Re: permissions

[Alex Riesen]

On Wed, Jun 9, 2010 at 14:06, Thomas Rast <trast@student.ethz.ch> wrote:
> Or maybe we don't care about BOFH ideas?

Git _is_ used in corporate environments. The BOs thrive there,
not to mention ideas they get.