{"thread":{"id":"31293","subject":"receive.denyNonNonFastForwards not denying force update","startedAt":"2012-08-20T13:33:29Z","lastAt":"2012-09-10T13:24:20Z","messageCount":20,"participants":["John Arthorne","Junio C Hamano","Sitaram Chamarty","Brandon Casey","Jeff King","Jay Soffian"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"197405","messageId":"CAHgXSooFj2PJtcOWqsVNHUzMBQnH0cYzPjfs1CkzVuufwRVrog@mail.gmail.com","threadId":"31293","inReplyTo":"CAHgXSop42qWcAEGn6=og8Pistv_Jrwhgcnv3B_ORVtSMi1fCHA@mail.gmail.com","subject":"receive.denyNonNonFastForwards not denying force update","fromName":"John Arthorne","fromEmail":"arthorne.eclipse@gmail.com","sentAt":"2012-08-20T13:33:29Z","receivedAt":"2012-08-20T13:33:29Z","isPatch":false,"sender":{"key":"arthorne.eclipse@gmail.com","avatar":null},"body":"At eclipse.org we wanted all git repositories to disallow non-fastforward\ncommits by default. So, we set receive.denyNonFastForwards=true as a system\nconfiguration setting. However, this does not prevent a non-fastforward\nforce push. If we set the same configuration setting in the local repository\nconfiguration then it does prevent non-fastforward pushes.\n\nFor all the details see this bugzilla, particularly comment #59 where we\nfinally narrowed this down:\n\nhttps://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n\nThis is on git version 1.7.4.1.\n\nThe Git book recommends setting this property at the system level:\n\nhttp://git-scm.com/book/ch7-1.html (near the bottom)\n\nCan someone confirm if this is intended behaviour or not. We ended up\nusing a script to set a local config property in each repository, but\nwith several hundred git repositories it would be much easier if the\nsystem setting was honoured.\n\nThanks,\nJohn Arthorne\n"},{"id":"197423","messageId":"7vzk5pjxy3.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"CAHgXSooFj2PJtcOWqsVNHUzMBQnH0cYzPjfs1CkzVuufwRVrog@mail.gmail.com","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T17:05:40Z","receivedAt":"2012-08-20T17:05:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Arthorne writes:\n\n> For all the details see this bugzilla, particularly comment #59 where we\n> finally narrowed this down:\n>\n> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n\nWhat does \"at the system level\" in your \"does *not* work at the\nsystem level.\" exactly mean?\n\nA configuration variable in the repository configuration take\nprecedence over user preference $HOME/.gitconfig which in turn take\nprecedence over system wide default /etc/gitconfig (or whereever you\nor your distro decided to place it). In other words, if you have\n\"[receive] denyNonFastForwards\" in the system wide default, you can\nsay \"[receive] denyNonFastForwards = false\" in one particular\nrepository to allow it for that repository.\n"},{"id":"197476","messageId":"CAMK1S_hMTGhiKDow3x-UZ7eNnTtpLd2=QUf6-YoQF1-O1ywi2w@mail.gmail.com","threadId":"31293","inReplyTo":"7vzk5pjxy3.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2012-08-21T00:52:36Z","receivedAt":"2012-08-21T00:52:36Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano wrote:\n> John Arthorne writes:\n>\n>> For all the details see this bugzilla, particularly comment #59 where we\n>> finally narrowed this down:\n>>\n>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n>\n> What does \"at the system level\" in your \"does *not* work at the\n> system level.\" exactly mean?\n\n\"git config --system receive.denynonfastforwards true\" is not honored.\n At all. (And I checked there was nothing overriding it).\n\n\"--global\" does work (is honored).\n\nTested on 1.7.11\n"},{"id":"197477","messageId":"7v628dght9.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"CAMK1S_hMTGhiKDow3x-UZ7eNnTtpLd2=QUf6-YoQF1-O1ywi2w@mail.gmail.com","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T01:22:26Z","receivedAt":"2012-08-21T01:22:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sitaram Chamarty writes:\n\n> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano wrote:\n>> John Arthorne writes:\n>>\n>>> For all the details see this bugzilla, particularly comment #59 where we\n>>> finally narrowed this down:\n>>>\n>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n>>\n>> What does \"at the system level\" in your \"does *not* work at the\n>> system level.\" exactly mean?\n>\n> \"git config --system receive.denynonfastforwards true\" is not honored.\n> At all. (And I checked there was nothing overriding it).\n>\n> \"--global\" does work (is honored).\n>\n> Tested on 1.7.11\n\nThanks, and interesting.\n\nDoes anybody recall if this is something we did on purpose? After\neyeballing the callchain starting from cmd_receive_pack() down to\nreceive_pack_config(), nothing obvious jumps at me.\n\nCould this be caused by a chrooted environment not having\n/etc/gitconfig (now I am just speculating)?\n\nA quick \"strace -f -o /tmp/tr git push ../neigh\" seems to indicate\nthat at least access() is called on \"/etc/gitconfig\" as I expect,\nwhich makes me think that near the beginning of git_config_early(),\nwe would read from /etc/gitconfig if the file existed (I do not\ninstall any distro \"git\", so there is no /etc/gitconfig on my box).\n\nPuzzled.\n"},{"id":"197479","messageId":"CA+sFfMexCWLza65bVp2uXoqo3+yY5MPBBcGugoEA6UCEwAv6Ow@mail.gmail.com","threadId":"31293","inReplyTo":"7v628dght9.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-08-21T01:53:53Z","receivedAt":"2012-08-21T01:53:53Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Aug 20, 2012 at 6:22 PM, Junio C Hamano wrote:\n> Sitaram Chamarty writes:\n>\n>> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano wrote:\n>>> John Arthorne writes:\n>>>\n>>>> For all the details see this bugzilla, particularly comment #59 where we\n>>>> finally narrowed this down:\n>>>>\n>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n>>>\n>>> What does \"at the system level\" in your \"does *not* work at the\n>>> system level.\" exactly mean?\n>>\n>> \"git config --system receive.denynonfastforwards true\" is not honored.\n>> At all. (And I checked there was nothing overriding it).\n>>\n>> \"--global\" does work (is honored).\n>>\n>> Tested on 1.7.11\n>\n> Thanks, and interesting.\n>\n> Does anybody recall if this is something we did on purpose? After\n> eyeballing the callchain starting from cmd_receive_pack() down to\n> receive_pack_config(), nothing obvious jumps at me.\n>\n> Could this be caused by a chrooted environment not having\n> /etc/gitconfig (now I am just speculating)?\n>\n> A quick \"strace -f -o /tmp/tr git push ../neigh\" seems to indicate\n> that at least access() is called on \"/etc/gitconfig\" as I expect,\n> which makes me think that near the beginning of git_config_early(),\n> we would read from /etc/gitconfig if the file existed (I do not\n> install any distro \"git\", so there is no /etc/gitconfig on my box).\n>\n> Puzzled.\n\nSeems to work for me. Force push was denied when\nreceive.denyNonFastForwards was set to true in system-level gitconfig.\n Tested with git installed in my home directory, so my system-level\ngitconfig was at $HOME/etc/gitconfig.\n\nSitaram and John, are you sure you modified the correct file? Also be\nsure you're using the git-receive-pack that expects the system\ngitconfig at the place that you think it is.\n\nThe system-level gitconfig is hard-coded in the git binary and may not\nalways be at /etc/gitconfig. It is usually set to be relative to the\ninstallation directory \"$prefix\" in the Makefile. I don't think we\nexpose the path to the system-level gitconfig file anywhere in the ui.\n One way to figure out where it should be is to use 'git config' to\nedit it like this:\n\n git config --system -e\n\nHopefully your editor exposes the path that it is editing even if you\ndon't have permission to modify it.\n\nI'm thinking that the git-receive-pack binary that you guys used\nexpects the system gitconfig to be in a different location than the\none you modified.\n\n-Brandon\n"},{"id":"197481","messageId":"20120821015738.GA20271@sigill.intra.peff.net","threadId":"31293","inReplyTo":"7v628dght9.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T01:57:38Z","receivedAt":"2012-08-21T01:57:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote:\n\n> > \"git config --system receive.denynonfastforwards true\" is not honored.\n> > At all. (And I checked there was nothing overriding it).\n> >\n> > \"--global\" does work (is honored).\n> >\n> > Tested on 1.7.11\n> \n> Thanks, and interesting.\n> \n> Does anybody recall if this is something we did on purpose? After\n> eyeballing the callchain starting from cmd_receive_pack() down to\n> receive_pack_config(), nothing obvious jumps at me.\n\nNo, I do not think it was on purpose. And it would be very hard to do\nso, anyway; config callbacks are not given any information about the\nsource of the config variable, and cannot distinguish between repo,\nglobal, and system-level config variables.\n\n> Could this be caused by a chrooted environment not having\n> /etc/gitconfig (now I am just speculating)?\n\nThat seems far more likely to me. Another possibility is that the file\nis not readable by the user running receive-pack.\n\n> A quick \"strace -f -o /tmp/tr git push ../neigh\" seems to indicate\n> that at least access() is called on \"/etc/gitconfig\" as I expect,\n> which makes me think that near the beginning of git_config_early(),\n> we would read from /etc/gitconfig if the file existed (I do not\n> install any distro \"git\", so there is no /etc/gitconfig on my box).\n\nI just did a few quick tests both across local repos and across an ssh\nsession. receive.denynonfastforwards worked just fine in my\n/etc/gitconfig in both cases. So the likely cause would be that git\ncannot access that file for some reason (chroot or permissions).\n\n-Peff\n"},{"id":"197483","messageId":"CAMK1S_gKWYxSBi5EpBJ3JZhPnqaGD=UoozVzUAKfjBG0CycvQg@mail.gmail.com","threadId":"31293","inReplyTo":"7v628dght9.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2012-08-21T02:08:41Z","receivedAt":"2012-08-21T02:08:41Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Tue, Aug 21, 2012 at 6:52 AM, Junio C Hamano wrote:\n> Sitaram Chamarty writes:\n>\n>> On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano wrote:\n>>> John Arthorne writes:\n>>>\n>>>> For all the details see this bugzilla, particularly comment #59 where we\n>>>> finally narrowed this down:\n>>>>\n>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n>>>\n>>> What does \"at the system level\" in your \"does *not* work at the\n>>> system level.\" exactly mean?\n>>\n>> \"git config --system receive.denynonfastforwards true\" is not honored.\n>> At all. (And I checked there was nothing overriding it).\n>>\n>> \"--global\" does work (is honored).\n>>\n>> Tested on 1.7.11\n>\n> Thanks, and interesting.\n\nUggh. My fault this one.\n\nI had a very tight umask on root, and running 'git config --system'\ncreated an /etc/gitconfig that was not readable by a normal user.\n\nRunning strace clued me in...\n\nJohn: maybe it's as simple as that in your case too.\n\nJunio/Brandon/Jeff: sorry for the false corroboration of John's report!\n"},{"id":"197484","messageId":"CAG+J_Dz3SHyNSUBuFcHu-x8gkE+wj5wJGLOfopNQw0dBThtSuA@mail.gmail.com","threadId":"31293","inReplyTo":"CA+sFfMexCWLza65bVp2uXoqo3+yY5MPBBcGugoEA6UCEwAv6Ow@mail.gmail.com","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2012-08-21T02:16:56Z","receivedAt":"2012-08-21T02:16:56Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey wrote:\n> git config --system -e\n>\n> Hopefully your editor exposes the path that it is editing even if you\n> don't have permission to modify it.\n\n GIT_EDITOR=echo git config --system -e\n\nworks for me.\n\nj.\n"},{"id":"197488","messageId":"7vtxvwgb5j.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"CAG+J_Dz3SHyNSUBuFcHu-x8gkE+wj5wJGLOfopNQw0dBThtSuA@mail.gmail.com","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T03:46:16Z","receivedAt":"2012-08-21T03:46:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey wrote:\n>> git config --system -e\n>>\n>> Hopefully your editor exposes the path that it is editing even if you\n>> don't have permission to modify it.\n>\n> GIT_EDITOR=echo git config --system -e\n>\n> works for me.\n\nClever ;-)\n"},{"id":"197489","messageId":"7vpq6kgazt.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"20120821015738.GA20271@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T03:49:42Z","receivedAt":"2012-08-21T03:49:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote:\n>\n>> Does anybody recall if this is something we did on purpose? After\n>> eyeballing the callchain starting from cmd_receive_pack() down to\n>> receive_pack_config(), nothing obvious jumps at me.\n>\n> No, I do not think it was on purpose. And it would be very hard to do\n> so, anyway; config callbacks are not given any information about the\n> source of the config variable, and cannot distinguish between repo,\n> global, and system-level config variables.\n\nI was looking for setenv() to refuse system wide defaults; that\nactually is fairly simple.\n\n>> Could this be caused by a chrooted environment not having\n>> /etc/gitconfig (now I am just speculating)?\n>\n> That seems far more likely to me. Another possibility is that the file\n> is not readable by the user running receive-pack.\n\nGood point. We explicitly use access(R_OK) and pretend as if a path\nthat is known to exist but not readable is missing; perhaps we may\nwant to diagnose this as a misconfiguration and issue a warning?\n"},{"id":"197504","messageId":"20120821061059.GA26516@sigill.intra.peff.net","threadId":"31293","inReplyTo":"7vpq6kgazt.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T06:10:59Z","receivedAt":"2012-08-21T06:10:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 20, 2012 at 08:49:42PM -0700, Junio C Hamano wrote:\n\n> > No, I do not think it was on purpose. And it would be very hard to do\n> > so, anyway; config callbacks are not given any information about the\n> > source of the config variable, and cannot distinguish between repo,\n> > global, and system-level config variables.\n> \n> I was looking for setenv() to refuse system wide defaults; that\n> actually is fairly simple.\n\nAh. I was thinking we had ripped those out (since they were primarily\nabout the test suite, and we found other ways of working around them),\nbut we do indeed still have GIT_CONFIG_NOSYSTEM. So yet another\npossibility is that the OP has that environment variable set for some\nodd reason.\n\n> > That seems far more likely to me. Another possibility is that the\n> > file is not readable by the user running receive-pack.\n> \n> Good point. We explicitly use access(R_OK) and pretend as if a path\n> that is known to exist but not readable is missing; perhaps we may\n> want to diagnose this as a misconfiguration and issue a warning?\n\nI think that makes sense. Like this patch?\n\n-- >8 --\nSubject: [PATCH] config: warn on inaccessible files\n\nBefore reading a config file, we check \"!access(path, R_OK)\"\nto make sure that the file exists and is readable. If it's\nnot, then we silently ignore it.\n\nFor the case of ENOENT, this is fine, as the presence of the\nfile is optional. For other cases, though, it may indicate a\nconfiguration error (e.g., not having permissions to read\nthe file). Let's print a warning in these cases to let the\nuser know.\n\nSigned-off-by: Jeff King \n---\nThis catches the common code path of git itself trying to read the\nconfig file. The \"git config foo.bar\" lookup path does not warn, as it\njust tries to fopen each file (and silently bails if a file cannot be\nopened). However, since before doing its actual lookup, it would run\ngit_config() anyway, you will already have seen the warning.\n\nYou can get multiple warnings from this, as some programs read the\nconfig multiple times. I don't think it's really worth caring about, as\nyou would want to fix such a misconfiguration quickly anyway.\n\nA bigger question is whether people are stuck living with such a\nmisconfiguration (e.g., inaccessible directories made by a clueless\nadmin), and would be annoyed at having no way to turn this feature off.\n\n builtin/config.c | 4 ++--\n config.c | 10 +++++-----\n git-compat-util.h | 3 +++\n wrapper.c | 8 ++++++++\n 4 files changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8cd08da..b0394ef 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -396,8 +396,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t */\n \t\t\tdie(\"$HOME not set\");\n \n-\t\tif (access(user_config, R_OK) &&\n-\t\t xdg_config && !access(xdg_config, R_OK))\n+\t\tif (access_or_warn(user_config, R_OK) &&\n+\t\t xdg_config && !access_or_warn(xdg_config, R_OK))\n \t\t\tgiven_config_file = xdg_config;\n \t\telse\n \t\t\tgiven_config_file = user_config;\ndiff --git a/config.c b/config.c\nindex 2b706ea..08e47e2 100644\n--- a/config.c\n+++ b/config.c\n@@ -60,7 +60,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n \n-\tif (!access(path, R_OK)) {\n+\tif (!access_or_warn(path, R_OK)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n \t\t\t cf && cf->name ? cf->name : \"the command line\");\n@@ -939,23 +939,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \thome_config_paths(&user_config, &xdg_config, \"config\");\n \n-\tif (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {\n+\tif (git_config_system() && !access_or_warn(git_etc_gitconfig(), R_OK)) {\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n \t\t\t\t\t data);\n \t\tfound += 1;\n \t}\n \n-\tif (xdg_config && !access(xdg_config, R_OK)) {\n+\tif (xdg_config && !access_or_warn(xdg_config, R_OK)) {\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (user_config && !access(user_config, R_OK)) {\n+\tif (user_config && !access_or_warn(user_config, R_OK)) {\n \t\tret += git_config_from_file(fn, user_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (repo_config && !access(repo_config, R_OK)) {\n+\tif (repo_config && !access_or_warn(repo_config, R_OK)) {\n \t\tret += git_config_from_file(fn, repo_config, data);\n \t\tfound += 1;\n \t}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 35b095e..5a520e2 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -604,6 +604,9 @@ int rmdir_or_warn(const char *path);\n */\n int remove_or_warn(unsigned int mode, const char *path);\n \n+/* Call access(2), but warn for any error besides ENOENT. */\n+int access_or_warn(const char *path, int mode);\n+\n /* Get the passwd entry for the UID of the current process. */\n struct passwd *xgetpwuid_self(void);\n \ndiff --git a/wrapper.c b/wrapper.c\nindex b5e33e4..b40c7e7 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -403,6 +403,14 @@ int remove_or_warn(unsigned int mode, const char *file)\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n \n+int access_or_warn(const char *path, int mode)\n+{\n+\tint ret = access(path, mode);\n+\tif (ret && errno != ENOENT)\n+\t\twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n+\treturn ret;\n+}\n+\n struct passwd *xgetpwuid_self(void)\n {\n \tstruct passwd *pw;\n-- \n1.7.12.4.g4e9f38f\n"},{"id":"197505","messageId":"20120821062219.GB26516@sigill.intra.peff.net","threadId":"31293","inReplyTo":"20120821061059.GA26516@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T06:22:19Z","receivedAt":"2012-08-21T06:22:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2012 at 02:10:59AM -0400, Jeff King wrote:\n\n> I think that makes sense. Like this patch?\n> \n> -- >8 --\n> Subject: [PATCH] config: warn on inaccessible files\n> \n> Before reading a config file, we check \"!access(path, R_OK)\"\n> to make sure that the file exists and is readable. If it's\n> not, then we silently ignore it.\n> \n> For the case of ENOENT, this is fine, as the presence of the\n> file is optional. For other cases, though, it may indicate a\n> configuration error (e.g., not having permissions to read\n> the file). Let's print a warning in these cases to let the\n> user know.\n\nAnd this might be a good follow-on:\n\n-- >8 --\nSubject: [PATCH] gitignore: report access errors of exclude files\n\nWhen we try to access gitignore files, we check for their\nexistence with a call to \"access\". We silently ignore\nmissing files. However, if a file is not readable, this may\nbe a configuration error; let's warn the user.\n\nFor $GIT_DIR/info/excludes or core.excludesfile, we can just\nuse access_or_warn. However, for per-directory files we\nactually try to open them, so we must add a custom warning.\n\nSigned-off-by: Jeff King \n---\n dir.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 240bf0c..4ee16b5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,\n \n \tfd = open(fname, O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn(_(\"unable to access '%s': %s\"), fname, strerror(errno));\n \t\tif (0 <= fd)\n \t\t\tclose(fd);\n \t\tif (!check_index ||\n@@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n \t\texcludes_file = xdg_path;\n \t}\n-\tif (!access(path, R_OK))\n+\tif (!access_or_warn(path, R_OK))\n \t\tadd_excludes_from_file(dir, path);\n-\tif (excludes_file && !access(excludes_file, R_OK))\n+\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n \n-- \n1.7.12.4.g4e9f38f\n"},{"id":"197506","messageId":"20120821062607.GC26516@sigill.intra.peff.net","threadId":"31293","inReplyTo":"20120821062219.GB26516@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T06:26:07Z","receivedAt":"2012-08-21T06:26:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2012 at 02:22:19AM -0400, Jeff King wrote:\n\n> And this might be a good follow-on:\n> \n> -- >8 --\n> Subject: [PATCH] gitignore: report access errors of exclude files\n\n...and it would probably help if I gave you the version that actually\ncompiled.\n\n-- >8 --\nSubject: [PATCH] gitignore: report access errors of exclude files\n\nWhen we try to access gitignore files, we check for their\nexistence with a call to \"access\". We silently ignore\nmissing files. However, if a file is not readable, this may\nbe a configuration error; let's warn the user.\n\nFor $GIT_DIR/info/excludes or core.excludesfile, we can just\nuse access_or_warn. However, for per-directory files we\nactually try to open them, so we must add a custom warning.\n\nSigned-off-by: Jeff King \n---\n dir.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 240bf0c..ea74048 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,\n \n \tfd = open(fname, O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarning(_(\"unable to access '%s': %s\"), fname, strerror(errno));\n \t\tif (0 <= fd)\n \t\t\tclose(fd);\n \t\tif (!check_index ||\n@@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n \t\texcludes_file = xdg_path;\n \t}\n-\tif (!access(path, R_OK))\n+\tif (!access_or_warn(path, R_OK))\n \t\tadd_excludes_from_file(dir, path);\n-\tif (excludes_file && !access(excludes_file, R_OK))\n+\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n \t\tadd_excludes_from_file(dir, excludes_file);\n }\n \n-- \n1.7.12.4.g4e9f38f\n"},{"id":"197507","messageId":"20120821063152.GD26516@sigill.intra.peff.net","threadId":"31293","inReplyTo":"20120821062219.GB26516@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T06:31:52Z","receivedAt":"2012-08-21T06:31:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2012 at 02:22:19AM -0400, Jeff King wrote:\n\n> And this might be a good follow-on:\n> \n> -- >8 --\n> Subject: [PATCH] gitignore: report access errors of exclude files\n\nAnd if we are going to do that, then we almost certainly want to do\nthis.\n\n-- >8 --\nSubject: [PATCH] attr: warn on inaccessible attribute files\n\nJust like config and gitignore files, we silently ignore\nmissing or inaccessible attribute files. An existent but\ninaccessible file is probably a configuration error, so\nlet's warn the user.\n\nSigned-off-by: Jeff King \n---\n attr.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex b52efb5..cab01b8 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -352,8 +352,11 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \tchar buf[2048];\n \tint lineno = 0;\n \n-\tif (!fp)\n+\tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n \t\treturn NULL;\n+\t}\n \tres = xcalloc(1, sizeof(*res));\n \twhile (fgets(buf, sizeof(buf), fp))\n \t\thandle_attr_line(res, buf, path, ++lineno, macro_ok);\n-- \n1.7.12.4.g4e9f38f\n"},{"id":"197535","messageId":"7v628cfb6h.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"20120821061059.GA26516@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T16:43:18Z","receivedAt":"2012-08-21T16:43:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You can get multiple warnings from this, as some programs read the\n> config multiple times. I don't think it's really worth caring about, as\n> you would want to fix such a misconfiguration quickly anyway.\n\nI agree that we wouldn't care too much about the multiple warnings,\nand even if we did, it would be easy to correct. Instead of having a\ncall to warning(_(\"unable to access...\"), path, strerror(errno))\ndirectly in acceess_or_warn(), make that a helper function that is\ncalled from there and other places you warn in your other patches,\nand maintain a small table of already-warned-for paths in the helper,\nand we are done.\n\n> A bigger question is whether people are stuck living with such a\n> misconfiguration (e.g., inaccessible directories made by a clueless\n> admin), and would be annoyed at having no way to turn this feature off.\n\nYes, /etc/gitconfig would certainly have that issue; exclude and\nattr you deal with your other patches are safe, though.\n\nModulo the above \"you might want to turn the call to warn() to\nanother helper that can be used from elsewhere\", this patch looks\nperfect to me.\n\nThanks.\n"},{"id":"197536","messageId":"7v1uj0fauk.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"20120821062219.GB26516@sigill.intra.peff.net","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T16:50:27Z","receivedAt":"2012-08-21T16:50:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 21, 2012 at 02:10:59AM -0400, Jeff King wrote:\n>\n>> I think that makes sense. Like this patch?\n>> \n>> -- >8 --\n>> Subject: [PATCH] config: warn on inaccessible files\n>> \n>> Before reading a config file, we check \"!access(path, R_OK)\"\n>> to make sure that the file exists and is readable. If it's\n>> not, then we silently ignore it.\n>> \n>> For the case of ENOENT, this is fine, as the presence of the\n>> file is optional. For other cases, though, it may indicate a\n>> configuration error (e.g., not having permissions to read\n>> the file). Let's print a warning in these cases to let the\n>> user know.\n>\n> And this might be a good follow-on:\n>\n> -- >8 --\n> Subject: [PATCH] gitignore: report access errors of exclude files\n>\n> When we try to access gitignore files, we check for their\n> existence with a call to \"access\". We silently ignore\n> missing files. However, if a file is not readable, this may\n> be a configuration error; let's warn the user.\n>\n> For $GIT_DIR/info/excludes or core.excludesfile, we can just\n> use access_or_warn. However, for per-directory files we\n> actually try to open them, so we must add a custom warning.\n\nThere are a couple of users of add_excludes_from_file() that is\noutside the per-directory walking in ls-files and unpack-trees; I\nthink both are OK with this change, but the one in ls-files may want\nto issue a warning or even an error upon ENOENT.\n\nNot a regression with this patch; just something we may want to do\nwhile we are in the vicinity.\n\n> Signed-off-by: Jeff King \n> ---\n> dir.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 240bf0c..4ee16b5 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,\n> \n> \tfd = open(fname, O_RDONLY);\n> \tif (fd < 0 || fstat(fd, &st) < 0) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\twarn(_(\"unable to access '%s': %s\"), fname, strerror(errno));\n> \t\tif (0 <= fd)\n> \t\t\tclose(fd);\n> \t\tif (!check_index ||\n> @@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)\n> \t\thome_config_paths(NULL, &xdg_path, \"ignore\");\n> \t\texcludes_file = xdg_path;\n> \t}\n> -\tif (!access(path, R_OK))\n> +\tif (!access_or_warn(path, R_OK))\n> \t\tadd_excludes_from_file(dir, path);\n> -\tif (excludes_file && !access(excludes_file, R_OK))\n> +\tif (excludes_file && !access_or_warn(excludes_file, R_OK))\n> \t\tadd_excludes_from_file(dir, excludes_file);\n> }\n"},{"id":"197552","messageId":"20120821193331.GA15667@sigill.intra.peff.net","threadId":"31293","inReplyTo":"7v1uj0fauk.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T19:33:31Z","receivedAt":"2012-08-21T19:33:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2012 at 09:50:27AM -0700, Junio C Hamano wrote:\n\n> > Subject: [PATCH] gitignore: report access errors of exclude files\n> >\n> > When we try to access gitignore files, we check for their\n> > existence with a call to \"access\". We silently ignore\n> > missing files. However, if a file is not readable, this may\n> > be a configuration error; let's warn the user.\n> >\n> > For $GIT_DIR/info/excludes or core.excludesfile, we can just\n> > use access_or_warn. However, for per-directory files we\n> > actually try to open them, so we must add a custom warning.\n> \n> There are a couple of users of add_excludes_from_file() that is\n> outside the per-directory walking in ls-files and unpack-trees; I\n> think both are OK with this change, but the one in ls-files may want\n> to issue a warning or even an error upon ENOENT.\n> \n> Not a regression with this patch; just something we may want to do\n> while we are in the vicinity.\n\nThe two I see are:\n\n 1. unpack-trees:verify_absent\n\n This looks like it is reading info/sparse-checkout. But I think it\n is OK for that file to be missing, no?\n\n 2. ls-files:option_parse_exclude_from\n\n This handles --exclude-from. I would expect most callers to be\n converted to --exclude-standard these days, but originally callers\n did something like:\n\n git ls-files \\\n --exclude-from=$GIT_DIR/info/exclude \\\n --exclude-per-directory=.gitignore \\\n ...\n\n While it would be friendlier to a user calling ls-files to warn\n about a missing entry in the first case (since they explicitly\n typed it, they presumably expect it to work). But for a script\n calling the ls-files plumbing, that --exclude-from has always\n meant \"if it's there, use it, but otherwise, don't worry\".\n\n Probably no such callers exist anymore, but complaining would be\n a regression for them.\n\n-Peff\n"},{"id":"197559","messageId":"7va9xndibh.fsf@alter.siamese.dyndns.org","threadId":"31293","inReplyTo":"7v628cfb6h.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-21T21:52:02Z","receivedAt":"2012-08-21T21:52:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Modulo the above \"you might want to turn the call to warn() to\n> another helper that can be used from elsewhere\", this patch looks\n> perfect to me.\n\nAnd that \"modulo\" is fairly simple if we wanted to go that route.\n\n attr.c | 2 +-\n dir.c | 2 +-\n git-compat-util.h | 3 +++\n wrapper.c | 7 ++++++-\n 4 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git c/attr.c w/attr.c\nindex cab01b8..f12c83f 100644\n--- c/attr.c\n+++ w/attr.c\n@@ -354,7 +354,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \n \tif (!fp) {\n \t\tif (errno != ENOENT)\n-\t\t\twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n+\t\t\twarn_on_inaccessible(path);\n \t\treturn NULL;\n \t}\n \tres = xcalloc(1, sizeof(*res));\ndiff --git c/dir.c w/dir.c\nindex ea74048..4868339 100644\n--- c/dir.c\n+++ w/dir.c\n@@ -398,7 +398,7 @@ int add_excludes_from_file_to_list(const char *fname,\n \tfd = open(fname, O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n \t\tif (errno != ENOENT)\n-\t\t\twarning(_(\"unable to access '%s': %s\"), fname, strerror(errno));\n+\t\t\twarn_on_inaccessible(fname);\n \t\tif (0 <= fd)\n \t\t\tclose(fd);\n \t\tif (!check_index ||\ndiff --git c/git-compat-util.h w/git-compat-util.h\nindex 5a520e2..000042d 100644\n--- c/git-compat-util.h\n+++ w/git-compat-util.h\n@@ -607,6 +607,9 @@ int remove_or_warn(unsigned int mode, const char *path);\n /* Call access(2), but warn for any error besides ENOENT. */\n int access_or_warn(const char *path, int mode);\n \n+/* Warn on an inaccessible file that ought to be accessible */\n+void warn_on_inaccessible(const char *path);\n+\n /* Get the passwd entry for the UID of the current process. */\n struct passwd *xgetpwuid_self(void);\n \ndiff --git c/wrapper.c w/wrapper.c\nindex b40c7e7..68739aa 100644\n--- c/wrapper.c\n+++ w/wrapper.c\n@@ -403,11 +403,16 @@ int remove_or_warn(unsigned int mode, const char *file)\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n \n+void warn_on_inaccessible(const char *path)\n+{\n+\twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n+}\n+\n int access_or_warn(const char *path, int mode)\n {\n \tint ret = access(path, mode);\n \tif (ret && errno != ENOENT)\n-\t\twarning(_(\"unable to access '%s': %s\"), path, strerror(errno));\n+\t\twarn_on_inaccessible(path);\n \treturn ret;\n }\n \n"},{"id":"197560","messageId":"20120821215350.GA22215@sigill.intra.peff.net","threadId":"31293","inReplyTo":"7va9xndibh.fsf@alter.siamese.dyndns.org","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-21T21:53:50Z","receivedAt":"2012-08-21T21:53:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 21, 2012 at 02:52:02PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Modulo the above \"you might want to turn the call to warn() to\n> > another helper that can be used from elsewhere\", this patch looks\n> > perfect to me.\n> \n> And that \"modulo\" is fairly simple if we wanted to go that route.\n> \n> attr.c | 2 +-\n> dir.c | 2 +-\n> git-compat-util.h | 3 +++\n> wrapper.c | 7 ++++++-\n> 4 files changed, 11 insertions(+), 3 deletions(-)\n\nYeah, that looks fine to me if you want to squash it in.\n\n-Peff\n"},{"id":"198671","messageId":"CAHgXSoqZMPC8uawL7f+7iq-L=Ns+G2w4kh3_oV3DB=WXnTg+Ug@mail.gmail.com","threadId":"31293","inReplyTo":"CAHgXSop42qWcAEGn6=og8Pistv_Jrwhgcnv3B_ORVtSMi1fCHA@mail.gmail.com","subject":"Re: receive.denyNonNonFastForwards not denying force update","fromName":"John Arthorne","fromEmail":"arthorne.eclipse@gmail.com","sentAt":"2012-09-10T13:24:20Z","receivedAt":"2012-09-10T13:24:20Z","isPatch":false,"sender":{"key":"arthorne.eclipse@gmail.com","avatar":null},"body":"Just to close the loop on this thread, it did turn out to be a\npermission problem in our case. It was difficult to track down because\nit was only a problem on one server in the cluster. Each server had a\nsystem git config file at /usr/local/etc/gitconfig. This was a symlink\npointing to a single common config file at /etc/gitconfig. This real\nfile had correct content and permissions, and all the machines where\neclipse.org allows shell access had correct symlinks. So any tests on\nthe command line always showed that the system config looked fine.\nHowever on git.eclipse.org, which is the machine with the central\nrepositories we are pushing to, the symlink was missing o+rx. For\nsecurity reasons this machine doesn't allow shell access, but our\npushes to this machine were failing to honour the system\nconfiguration. I gather the patch prepared earlier in this thread will\ncause an error to be reported when the system config could not be\nread, which sounds like a good fix to help others track down problems\nlike this.\n\nJohn Arthorne\n\n\nOn Fri, Aug 17, 2012 at 12:26 PM, John Arthorne\n wrote:\n> At eclipse.org we wanted all git repositories to disallow non-fastforward\n> commits by default. So, we set receive.denyNonFastForwards=true as a system\n> configuration setting. However, this does not prevent a non-fastforward\n> force push. If we set the same configuration setting in the local repository\n> configuration then it does prevent non-fastforward pushes.\n>\n> For all the details see this bugzilla, particularly comment #59 where we\n> finally narrowed this down:\n>\n> https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150\n>\n> This is on git version 1.7.4.1.\n>\n> The Git book recommends setting this property at the system level:\n>\n> http://git-scm.com/book/ch7-1.html (near the bottom)\n>\n> Can someone confirm if this is intended behaviour or not.\n>\n> Thanks,\n> John Arthorne\n"}]}