{"thread":{"id":"24016","subject":"permissions","startedAt":"2010-06-05T09:33:50Z","lastAt":"2010-06-09T13:00:17Z","messageCount":17,"participants":["William Pursell","Andreas Schwab","Alex Riesen","Junio C Hamano","Steven Michalske","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"143028","messageId":"4C0A19FE.1020802@wpursell.net","threadId":"24016","inReplyTo":null,"subject":"permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-05T09:33:50Z","receivedAt":"2010-06-05T09:33:50Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"If .git is not readable, but ../.git is, .git works with\nthe database in ../.git.  Is that the desired behavior?\nIt would seem more appropriate that git fail with a\n\"permission denied\" error.  In particular, if .git\nis not readable and there is no database between\n$PWD and $GIT_CEILING_DIRECTORIES, the current error is:\nfatal: Not a git repository (or any of the parent directories): .git\n\nWouldn't it be better to have the error message be something like:\n.git: permission denied\n\nAfter all, it is a git repository, so \"Not a git repository\"\nis not accurate.\n\n-- \nWilliam Pursell\n"},{"id":"143029","messageId":"m27hmdn704.fsf@igel.home","threadId":"24016","inReplyTo":"4C0A19FE.1020802@wpursell.net","subject":"Re: permissions","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2010-06-05T09:50:03Z","receivedAt":"2010-06-05T09:50:03Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"William Pursell <bill.pursell@gmail.com> writes:\n\n> After all, it is a git repository, so \"Not a git repository\"\n> is not accurate.\n\nA valid git repository requires more than a .git directory.\n\n$ git init\nInitialized empty Git repository in /tmp/x/.git/\n$ rm .git/HEAD \n$ git status\nfatal: Not a git repository (or any of the parent directories): .git\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"143047","messageId":"4C0A9615.4090307@wpursell.net","threadId":"24016","inReplyTo":"m27hmdn704.fsf@igel.home","subject":"Re: permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-05T18:23:17Z","receivedAt":"2010-06-05T18:23:17Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Andreas Schwab wrote:\n> William Pursell <bill.pursell@gmail.com> writes:\n> \n>> After all, it is a git repository, so \"Not a git repository\"\n>> is not accurate.\n> \n> A valid git repository requires more than a .git directory.\n\nTrue, but this doesn't address the issue.  In the example\nyou gave, the error message is accurate.  Consider this:\n\n$ sudo sh -c 'umask 077; git init'\nInitialized empty Git repository in /private/tmp/foo/.git/\n$ git rev-parse --git-dir\nfatal: Not a git repository (or any of the parent directories): .git\n\nThat's just weird.  And if there is a git repository in a\ndirectory above, there may be great confusion, weeping\nand gnashing of teeth.\n\n\n-- \nWilliam Pursell\n"},{"id":"143072","messageId":"AANLkTileRHwUuJpvKJbivRiM9Prn9wJ0zH6abExBgcq0@mail.gmail.com","threadId":"24016","inReplyTo":"4C0A9615.4090307@wpursell.net","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-06T06:45:13Z","receivedAt":"2010-06-06T06:45:13Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:\n> fatal: Not a git repository (or any of the parent directories): .git\n>\n> That's just weird.  And if there is a git repository in a\n> directory above, there may be great confusion, weeping\n> and gnashing of teeth.\n\nHow about just this? (I assume cwd does hold current working directory).\n\ndiff --git a/setup.c b/setup.c\nindex 5a083fa..561f3ab 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -428,7 +428,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t\t*nongit_ok = 1;\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tdie(\"Not a git repository (or any of the parent directories): %s\",\nDEFAULT_GIT_DIR_ENVIRONMENT);\n+\t\t\tdie(\"Not a git repository (or any of the parent directories): %s\n(in %s)\", DEFAULT_GIT_DIR_ENVIRONMENT, cwd);\n \t\t}\n \t\tif (one_filesystem) {\n \t\t\tif (stat(\"..\", &buf)) {\n"},{"id":"143075","messageId":"4C0B6C32.1090700@wpursell.net","threadId":"24016","inReplyTo":"AANLkTileRHwUuJpvKJbivRiM9Prn9wJ0zH6abExBgcq0@mail.gmail.com","subject":"Re: permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-06T09:36:50Z","receivedAt":"2010-06-06T09:36:50Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Alex Riesen wrote:\n> On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:\n>> fatal: Not a git repository (or any of the parent directories): .git\n>>\n>> That's just weird.  And if there is a git repository in a\n>> directory above, there may be great confusion, weeping\n>> and gnashing of teeth.\n> \n> How about just this? (I assume cwd does hold current working directory).\n\n<patch snipped>\n\nThe problem is permissions, not that it's \"not a git repository\".\nThe error message should be \"permission denied\".  The easy solution\nis to abort with \"permission denied\" whenever that is encountered,\nbut the trouble with that is that it breaks the current work flow\nin which a broken dir (or one for which the user lacks\npriveleges) is bypassed and a valid object directory higher\nup in the filesystem tree is used.\n\nConsider the case in which /etc/.git has mode 777 while\n/etc/sysconfig/.git has mode 700, each .git owned by root.\n(Granted, git is for recording development history and\nnot so much for storing history of config files, but\nI believe this is a relevant use case.)  /etc/sysconfig/foo\nis tracked by /etc/sysconfig/.git but not by /etc/.git.\nRegular user in /etc/sysconfig invokes 'git log foo'\nand is told: absolutely nothing.   And when\n'git status' is invoked, the message is that foo is\nuntracked.\n\nNow, if /etc/.git does track /etc/sysconfig/foo,\nthen a regular user in /etc/sysconfig that invokes\n'git log foo' sees the history tracked in /etc/.git,\nbut root in /etc/sysconfig sees the history tracked\nby /etc/sysconfig/.git.  This is confusing.  The regular\nuser in /etc/sysconfig should simply get 'permission denied'\non all invocations of git.\n\nA related question is: does anyone actually prefer (or\nrely on) the current model in which ../.git is\nused in the event that .git is borked or the user\nlacks permission?  It seems to me that if an\nobject directory is discovered which is borked\nor which is unreadable, git must abort with an\nerror message indicating the relevant problem.\n\n-- \nWilliam Pursell\n"},{"id":"143084","messageId":"AANLkTim1-lN6TCD__V5F0BpPkmLbULkXHF03n0CqWTOi@mail.gmail.com","threadId":"24016","inReplyTo":"4C0B6C32.1090700@wpursell.net","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-06T12:45:40Z","receivedAt":"2010-06-06T12:45:40Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Sun, Jun 6, 2010 at 11:36, William Pursell <bill.pursell@gmail.com> wrote:\n> Alex Riesen wrote:\n>> On Sat, Jun 5, 2010 at 20:23, William Pursell <bill.pursell@gmail.com> wrote:\n>>> fatal: Not a git repository (or any of the parent directories): .git\n>>>\n>>> That's just weird.  And if there is a git repository in a\n>>> directory above, there may be great confusion, weeping\n>>> and gnashing of teeth.\n>>\n>> How about just this? (I assume cwd does hold current working directory).\n>\n> <patch snipped>\n>\n> The problem is permissions, not that it's \"not a git repository\".\n> The error message should be \"permission denied\".\n\nWell, isn't it enough to diagnose the problem? (where it lies, at least).\n"},{"id":"143101","messageId":"7vvd9wvswy.fsf@alter.siamese.dyndns.org","threadId":"24016","inReplyTo":"4C0B6C32.1090700@wpursell.net","subject":"Re: permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-06T19:54:05Z","receivedAt":"2010-06-06T19:54:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Pursell <bill.pursell@gmail.com> writes:\n\n> The problem is permissions, not that it's \"not a git repository\".\n> The error message should be \"permission denied\".  The easy solution\n> is to abort with \"permission denied\" whenever that is encountered,\n> but the trouble with that is that it breaks the current work flow\n> in which a broken dir (or one for which the user lacks\n> priveleges) is bypassed and a valid object directory higher\n> up in the filesystem tree is used.\n\nI think it is sane to abort with \"permission denied\", as it is \"not a git\nrepository\" but it is \"we cannot even determine if that .git we see is a\ngit repository, and if it is, then we cannot do any git operation here as\nwe cannot read it\".  As to what you call \"the current work flow\", I think\nit is not like we _support_ such usage, but more like it _happens to_ work\nthat way.\n\n> A related question is: does anyone actually prefer (or rely on) the\n> current model in which ../.git is used in the event that .git is borked\n> or the user lacks permission?\n\nSo my answer to this is \"nobody _should_\", as it is not even \"the current\nmodel\", but \"it happens to behave like that by accident\".  That doesn't\nmean there isn't anybody who already does, though...\n"},{"id":"143221","messageId":"4C0E1AB1.2030702@wpursell.net","threadId":"24016","inReplyTo":"7vvd9wvswy.fsf@alter.siamese.dyndns.org","subject":"Re: permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-08T10:25:53Z","receivedAt":"2010-06-08T10:25:53Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Junio C Hamano wrote:\n\n> I think it is sane to abort with \"permission denied\", as it is \"not a git\n> repository\" but it is \"we cannot even determine if that .git we see is a\n> git repository, and if it is, then we cannot do any git operation here as\n> we cannot read it\".  As to what you call \"the current work flow\", I think\n> it is not like we _support_ such usage, but more like it _happens to_ work\n> that way.\n\nHere's a patch.  This doesn't address the issue of a damaged\nrepository, but just catches access errors and permissions.\n\n>From 8f1c8f4d572fe62a26d1fca47abc976e78942697 Mon Sep 17 00:00:00 2001\nFrom: William Pursell <bill.pursell@gmail.com>\nDate: Tue, 8 Jun 2010 00:16:43 -1000\nSubject: [PATCH] Terminate on access errors\n\nThis changes the way git finds a repository.  Previously, if\naccess is denied to .git (or $GIT_DIR), git will use the object\ndirectory in a higher level directory.  With this patch, git will\ninstead terminate and emit an error message indicating the access\nfailure.  Also, other errors (such as soft-link loops in\nGIT_OBJECT_DIRECTORIES) will cause termination.\n\nSigned-off-by: William Pursell <bill.pursell@gmail.com>\n---\n setup.c |   18 +++++++++++++++---\n 1 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 7e04602..a53331c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -155,6 +155,18 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n }\n\n /*\n+ * Wrapper around access that terminates on\n+ * errors other than ENOENT.\n+ */\n+static int xaccess(const char *path, int amode)\n+{\n+\tint status = access(path, amode);\n+\tif (status && errno != ENOENT)\n+\t\tdie_errno(\"%s\", path);\n+\treturn status;\n+}\n+\n+/*\n  * Test if it looks like we're at a git directory.\n  * We want to see:\n  *\n@@ -172,17 +184,17 @@ static int is_git_directory(const char *suspect)\n\n \tstrcpy(path, suspect);\n \tif (getenv(DB_ENVIRONMENT)) {\n-\t\tif (access(getenv(DB_ENVIRONMENT), X_OK))\n+\t\tif (xaccess(getenv(DB_ENVIRONMENT), X_OK))\n \t\t\treturn 0;\n \t}\n \telse {\n \t\tstrcpy(path + len, \"/objects\");\n-\t\tif (access(path, X_OK))\n+\t\tif (xaccess(path, X_OK))\n \t\t\treturn 0;\n \t}\n\n \tstrcpy(path + len, \"/refs\");\n-\tif (access(path, X_OK))\n+\tif (xaccess(path, X_OK))\n \t\treturn 0;\n\n \tstrcpy(path + len, \"/HEAD\");\n-- \n1.7.1.245.g7c42e.dirty\n\n\n\n-- \nWilliam Pursell\n"},{"id":"143241","messageId":"AANLkTimAmSxq8dC-4bnpLsvN3JabQeTO6pDTh9ds7D0D@mail.gmail.com","threadId":"24016","inReplyTo":"4C0E1AB1.2030702@wpursell.net","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-08T14:52:57Z","receivedAt":"2010-06-08T14:52:57Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:\n> Here's a patch.  This doesn't address the issue of a damaged\n> repository, but just catches access errors and permissions.\n\nThe change looks fishy.\n\nThe patch moves the function is_git_directory at the level of user\ninterface where it wasn't before: it now complains and die.\nNot all callers of the function call it only to die if it fails.\n"},{"id":"143281","messageId":"7vtypds09x.fsf@alter.siamese.dyndns.org","threadId":"24016","inReplyTo":"AANLkTimAmSxq8dC-4bnpLsvN3JabQeTO6pDTh9ds7D0D@mail.gmail.com","subject":"Re: permissions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-08T21:05:30Z","receivedAt":"2010-06-08T21:05:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:\n>> Here's a patch.  This doesn't address the issue of a damaged\n>> repository, but just catches access errors and permissions.\n>\n> The change looks fishy.\n>\n> The patch moves the function is_git_directory at the level of user\n> interface where it wasn't before: it now complains and die.\n> Not all callers of the function call it only to die if it fails.\n\nThanks for shooting it down before I had to look at it ;-)\n"},{"id":"143291","messageId":"4C0EC3D0.6060509@wpursell.net","threadId":"24016","inReplyTo":"7vtypds09x.fsf@alter.siamese.dyndns.org","subject":"Re: permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-08T22:27:28Z","receivedAt":"2010-06-08T22:27:28Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Junio C Hamano wrote:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:\n>>> Here's a patch.  This doesn't address the issue of a damaged\n>>> repository, but just catches access errors and permissions.\n>> The change looks fishy.\n>>\n>> The patch moves the function is_git_directory at the level of user\n>> interface where it wasn't before: it now complains and die.\n>> Not all callers of the function call it only to die if it fails.\n> \n> Thanks for shooting it down before I had to look at it ;-)\n\nThe point of the patch is that it now complains and dies.\nPerhaps I'm being obtuse, but can you describe a situation\nin which this causes git to terminate inappropriately?\n\n\n\n-- \nWilliam Pursell\n"},{"id":"143310","messageId":"AANLkTikGpbeP1ba0y0oUsWGQXsrL8Z-GKjybCB83W_FJ@mail.gmail.com","threadId":"24016","inReplyTo":"4C0EC3D0.6060509@wpursell.net","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-09T07:20:27Z","receivedAt":"2010-06-09T07:20:27Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:\n> Junio C Hamano wrote:\n>> Alex Riesen <raa.lkml@gmail.com> writes:\n>>\n>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:\n>>>> Here's a patch.  This doesn't address the issue of a damaged\n>>>> repository, but just catches access errors and permissions.\n>>> The change looks fishy.\n>>>\n>>> The patch moves the function is_git_directory at the level of user\n>>> interface where it wasn't before: it now complains and die.\n>>> Not all callers of the function call it only to die if it fails.\n>>\n>> Thanks for shooting it down before I had to look at it ;-)\n>\n> The point of the patch is that it now complains and dies.\n\nAt wrong point. Points, actually. There are many callers of the\nfunction you modified. You should have looked at them all.\n\n> Perhaps I'm being obtuse, but can you describe a situation\n> in which this causes git to terminate inappropriately?\n\nMaybe. BTW, can you? (if you try, I mean). But your questions\nmisses the point of my complaint about your patch:\n\nThe patch makes the function you modified act not as one\ncan guess from its other uses. Imagine someone replaced\nopen(2) implementation to kill your program everytime you\ntried to open /etc/passwd. How'd you like that?\n\nThat alone is reason enough to dislike the change and put\nyou personally into a list of persons to be careful with (as\nyou don't seem to care about what happens with the code\nafter you changed it).\n"},{"id":"143322","messageId":"4C0F6CF0.3020603@wpursell.net","threadId":"24016","inReplyTo":"AANLkTikGpbeP1ba0y0oUsWGQXsrL8Z-GKjybCB83W_FJ@mail.gmail.com","subject":"Re: permissions","fromName":"William Pursell","fromEmail":"bill.pursell@gmail.com","sentAt":"2010-06-09T10:29:04Z","receivedAt":"2010-06-09T10:29:04Z","isPatch":false,"sender":{"key":"bill.pursell@gmail.com","avatar":"https://gravatar.com/avatar/3ab4313e5dfdc1fedb65206d829ba33f71f56f26e11326979d1b99d5e1c403c9?d=mp&s=160"},"body":"Alex Riesen wrote:\n> On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:\n>> Junio C Hamano wrote:\n>>> Alex Riesen <raa.lkml@gmail.com> writes:\n>>>\n>>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com> wrote:\n>>>>> Here's a patch.  This doesn't address the issue of a damaged\n>>>>> repository, but just catches access errors and permissions.\n>>>> The change looks fishy.\n>>>>\n>>>> The patch moves the function is_git_directory at the level of user\n>>>> interface where it wasn't before: it now complains and die.\n>>>> Not all callers of the function call it only to die if it fails.\n>>> Thanks for shooting it down before I had to look at it ;-)\n>> The point of the patch is that it now complains and dies.\n> \n> At wrong point. Points, actually. There are many callers of the\n> function you modified. You should have looked at them all.\n\nI did look at all 4 calls, and it seemed to me\nthat localizing the change in one location is a better\ndesign than adding logic to 4 different locations.\n\n>> Perhaps I'm being obtuse, but can you describe a situation\n>> in which this causes git to terminate inappropriately?\n> \n> Maybe. BTW, can you? (if you try, I mean).\n\nNo, I can't.  As far as I can tell, the patch adds\nexactly the functionality that I want it to add.  You\ndo make good points about its problems below, however,\nand you are right that I did miss the point of\nyour criticism.  Thank you for clarifying.\n\n> But your questions\n> misses the point of my complaint about your patch:\n> \n> The patch makes the function you modified act not as one\n> can guess from its other uses. Imagine someone replaced\n> open(2) implementation to kill your program everytime you\n> tried to open /etc/passwd. How'd you like that?\n\nI think there is a substantial difference between changing\na basic library call and changing a statically linked\nfunction called from only 4 locations, but I'll agree\nthat you have a valid point about the function not\nbehaving as expected.  The functionality I've added disagrees\nwith the name of the function, so on that point alone I will\nagree that the patch is no good.\n\n> \n> That alone is reason enough to dislike the change and put\n> you personally into a list of persons to be careful with (as\n> you don't seem to care about what happens with the code\n> after you changed it).\n\nI do care quite a lot actually.  My primary goal\nwas to minimize the changes, and it seemed that\nis_git_directory() was the right place to make\nthe change with minimal impact.  Perhaps the following\npatch would be more to your liking:\n\n\ndiff --git a/setup.c b/setup.c\nindex 7e04602..b25da21 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -303,6 +303,9 @@ const char *read_gitfile_gently(const char *path)\n \t\tbuf = dir;\n \t}\n\n+\tif (access(dir, X_OK))\n+\t\tdie_errno(\"Unable to access %s\", dir);\n+\n \tif (!is_git_directory(dir))\n \t\tdie(\"Not a git repository: %s\", dir);\n \tpath = make_absolute_path(dir);\n@@ -370,6 +373,9 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\t*nongit_ok = 1;\n \t\t\treturn NULL;\n \t\t}\n+\t\tif (access(gitdirenv, X_OK))\n+\t\t\tdie_errno(\"Unable to access %s\", gitdirenv);\n+\n \t\tdie(\"Not a git repository: '%s'\", gitdirenv);\n \t}\n\n@@ -407,6 +413,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t}\n \t\tif (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))\n \t\t\tbreak;\n+\t\tif (access(DEFAULT_GIT_DIR_ENVIRONMENT, X_OK)\n+\t\t\t\t && errno != ENOENT )\n+\t\t\tdie_errno(\"Unable to access %s/%s\",\n+\t\t\t\t cwd, DEFAULT_GIT_DIR_ENVIRONMENT);\n+\n \t\tif (is_git_directory(\".\")) {\n \t\t\tinside_git_dir = 1;\n \t\t\tif (!work_tree_env)\ndiff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh\nindex cb14425..9f6f756 100755\n--- a/t/t0002-gitfile.sh\n+++ b/t/t0002-gitfile.sh\n@@ -46,7 +46,7 @@ test_expect_success 'bad setup: invalid .git file path' '\n \t\techo \"git rev-parse accepted an invalid .git file path\"\n \t\tfalse\n \tfi &&\n-\tif ! grep \"Not a git repository\" .err\n+\tif ! grep \"Unable to access $REAL.not\" .err\n \tthen\n \t\techo \"git rev-parse returned wrong error\"\n \t\tfalse\n\n-- \nWilliam Pursell\n"},{"id":"143323","messageId":"586C928F-9EB5-44C3-A4AE-814AC5E11E91@gmail.com","threadId":"24016","inReplyTo":"AANLkTikGpbeP1ba0y0oUsWGQXsrL8Z-GKjybCB83W_FJ@mail.gmail.com","subject":"Re: permissions","fromName":"Steven Michalske","fromEmail":"smichalske@gmail.com","sentAt":"2010-06-09T10:39:24Z","receivedAt":"2010-06-09T10:39:24Z","isPatch":false,"sender":{"key":"smichalske@gmail.com","avatar":"https://gravatar.com/avatar/721f27456adc9ac84f3bb235f021a70015abb9e09222ae8622fc5579c6a203c1?d=mp&s=160"},"body":"\nOn Jun 9, 2010, at 12:20 AM, Alex Riesen wrote:\n\n> On Wed, Jun 9, 2010 at 00:27, William Pursell  \n> <bill.pursell@gmail.com> wrote:\n>> Junio C Hamano wrote:\n>>> Alex Riesen <raa.lkml@gmail.com> writes:\n>>>\n>>>> On Tue, Jun 8, 2010 at 12:25, William Pursell <bill.pursell@gmail.com \n>>>> > wrote:\n>>>>> Here's a patch.  This doesn't address the issue of a damaged\n>>>>> repository, but just catches access errors and permissions.\n>>>> The change looks fishy.\n>>>>\n>>>> The patch moves the function is_git_directory at the level of user\n>>>> interface where it wasn't before: it now complains and die.\n>>>> Not all callers of the function call it only to die if it fails.\n>>>\n>>> Thanks for shooting it down before I had to look at it ;-)\n>>\n>> The point of the patch is that it now complains and dies.\n>\n> At wrong point. Points, actually. There are many callers of the\n> function you modified. You should have looked at them all.\n>\nShould the other functions not fail in this case?  Looking at the uses  \n3 in setup.c and not exported for use in other code.  Only once case  \nlooked like it could cause an issue would be if the file did not  \nexist, and he excluded that case, lines 408 and 409 of setup.c  Where  \nthe environment variable is passed into the function.\n\nWell that's a good question,  William made a valid assumption that if  \nyou were checking a directory  that is suspected to be a git directory  \nand it couldn't be read you should let the user know that something is  \nfunky.  Now this looks like the right place for catching that access  \nviolation, but it looks like it night not the right place to report  \nthe error.....\n\nSo return another error code for catching it up stream.  But, we can't  \nbecause this function can be true many ways and false only one.  This  \nis where the shell convention that 0 is ok and errors are not 0 really  \nshines, but doesn't work for the name of this function.\n\n\n\n>> Perhaps I'm being obtuse, but can you describe a situation\n>> in which this causes git to terminate inappropriately?\n>\n> Maybe. BTW, can you? (if you try, I mean). But your questions\n> misses the point of my complaint about your patch:\n>\nAnd this point was not clearly explained on your part.... I had to  \nread your complaint a few times to understand what you meant.\n\nSomething mentioned this way might have been more insightful.\nThe patch should pass error up to calling function and not terminate  \nin the function.\n\nAnd offer a suggestion of how you would prefer it to be implemented.\n\n> The patch makes the function you modified act not as one\n> can guess from its other uses. Imagine someone replaced\n> open(2) implementation to kill your program everytime you\n> tried to open /etc/passwd. How'd you like that?\n>\nYour analogy is subtly off.\n\nBecause if you tried to open /etc/passwd and could not open it for  \nreading your application should fail.  That works because open allows  \nfor the return of an error code, but is_git_directory does not, so  \npatching at the level that will correctly detect this erroneous case  \nis difficult to report upstream.\n\n> That alone is reason enough to dislike the change and put\n> you personally into a list of persons to be careful with (as\n> you don't seem to care about what happens with the code\n> after you changed it).\n\nHonestly it is not, he was asking what he did wrong, and probably  \ndidn't follow your line of reasoning.\n\nSteve\n"},{"id":"143324","messageId":"201006091406.50955.trast@student.ethz.ch","threadId":"24016","inReplyTo":"4C0F6CF0.3020603@wpursell.net","subject":"Re: permissions","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-06-09T12:06:50Z","receivedAt":"2010-06-09T12:06:50Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"William Pursell wrote:\n> Alex Riesen wrote:\n> > On Wed, Jun 9, 2010 at 00:27, William Pursell <bill.pursell@gmail.com> wrote:\n> >> Junio C Hamano wrote:\n> >>> Alex Riesen <raa.lkml@gmail.com> writes:\n> >>>\n> >>>> The patch moves the function is_git_directory at the level of user\n> >>>> interface where it wasn't before: it now complains and die.\n> >>>> Not all callers of the function call it only to die if it fails.\n> >>> Thanks for shooting it down before I had to look at it ;-)\n> >> The point of the patch is that it now complains and dies.\n> > \n> > At wrong point. Points, actually. There are many callers of the\n> > function you modified. You should have looked at them all.\n> \n> I did look at all 4 calls, and it seemed to me\n> that localizing the change in one location is a better\n> design than adding logic to 4 different locations.\n> \n> >> Perhaps I'm being obtuse, but can you describe a situation\n> >> in which this causes git to terminate inappropriately?\n> > \n> > Maybe. BTW, can you? (if you try, I mean).\n> \n> No, I can't.  As far as I can tell, the patch adds\n> exactly the functionality that I want it to add.  You\n> do make good points about its problems below, however,\n> and you are right that I did miss the point of\n> your criticism.  Thank you for clarifying.\n\nMaybe I'm missing something, but I think that also apart from any\nmeta-criticism the patch is wrong.  From the use of\nsetup_git_directory_gently() in cmd_apply() [for example; there are\nother commands that are supposed to work both in- and outside of\nrepos], I conclude that the invocation of is_git_directory() must not\ndie() because it is *okay* if the directory is, after all, not a git\nrepo.\n\nAnd I think the same goes for your new patch\n\n> @@ -407,6 +413,11 @@ const char *setup_git_directory_gently(int *nongit_ok)\n>                 }\n>                 if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))\n>                         break;\n> +               if (access(DEFAULT_GIT_DIR_ENVIRONMENT, X_OK)\n> +                                && errno != ENOENT )\n> +                       die_errno(\"Unable to access %s/%s\",\n> +                                cwd, DEFAULT_GIT_DIR_ENVIRONMENT);\n> +\n>                 if (is_git_directory(\".\")) {\n>                         inside_git_dir = 1;\n>                         if (!work_tree_env)\n\n[DEFAULT_GIT_DIR_ENVIRONMENT is \".git\"]\n\nUnless I'm missing something, this effectively prevents git-apply and\nfriends from working outside any repos if your BOFH sysadmin thinks it\nfunny to place an unreadable .git somewhere on the way up to /.\n\nOr maybe we don't care about BOFH ideas?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"143330","messageId":"AANLkTik1FqUkgLYLiz1frA_G0Lws8B4tf7c6Kp3dXcCO@mail.gmail.com","threadId":"24016","inReplyTo":"4C0F6CF0.3020603@wpursell.net","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-09T12:56:43Z","receivedAt":"2010-06-09T12:56:43Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jun 9, 2010 at 12:29, William Pursell <bill.pursell@gmail.com> wrote:\n> I do care quite a lot actually.  My primary goal\n> was to minimize the changes, and it seemed that\n> is_git_directory() was the right place to make\n> the change with minimal impact.\n\nWhile smaller patches are preferred, they are not the goal, per say.\n\n> Perhaps the following patch would be more to your liking:\n\nLooks like a lot of effort just to get a little more information in\nan infrequent (and frankly, obscure) failure case.\nHow about just use errno after failed is_git_directory? It seems\nto make sense even when the function calls down to validate_headref.\nYou may even reset errno to 0 (this will also make obvious\nthe expectation of valid errno from is_git_directory).\n"},{"id":"143331","messageId":"AANLkTimrK5Hf9TBaed69gG330N9z9QpTrUIMY77KiX2_@mail.gmail.com","threadId":"24016","inReplyTo":"201006091406.50955.trast@student.ethz.ch","subject":"Re: permissions","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2010-06-09T13:00:17Z","receivedAt":"2010-06-09T13:00:17Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Wed, Jun 9, 2010 at 14:06, Thomas Rast <trast@student.ethz.ch> wrote:\n> Or maybe we don't care about BOFH ideas?\n\nGit _is_ used in corporate environments. The BOs thrive there,\nnot to mention ideas they get.\n"}]}