Re: receive.denyNonNonFastForwards not denying force update
Just to close the loop on this thread, it did turn out to be a permission problem in our case. It was difficult to track down because it was only a problem on one server in the cluster. Each server had a system git config file at /usr/local/etc/gitconfig. This was a symlink pointing to a single common config file at /etc/gitconfig. This real file had correct content and permissions, and all the machines where eclipse.org allows shell access had correct symlinks. So any tests on the command line always showed that the system config looked fine. However on git.eclipse.org, which is the machine with the central repositories we are pushing to, the symlink was missing o+rx. For security reasons this machine doesn't allow shell access, but our pushes to this machine were failing to honour the system configuration. I gather the patch prepared earlier in this thread will cause an error to be reported when the system config could not be read, which sounds like a good fix to help others track down problems like this. John Arthorne On Fri, Aug 17, 2012 at 12:26 PM, John Arthorne arthorne.ecli...@gmail.com wrote: At eclipse.org we wanted all git repositories to disallow non-fastforward commits by default. So, we set receive.denyNonFastForwards=true as a system configuration setting. However, this does not prevent a non-fastforward force push. If we set the same configuration setting in the local repository configuration then it does prevent non-fastforward pushes. For all the details see this bugzilla, particularly comment #59 where we finally narrowed this down: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 This is on git version 1.7.4.1. The Git book recommends setting this property at the system level: http://git-scm.com/book/ch7-1.html (near the bottom) Can someone confirm if this is intended behaviour or not. Thanks, John Arthorne -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Tue, Aug 21, 2012 at 02:10:59AM -0400, Jeff King wrote:
I think that makes sense. Like this patch?
-- 8 --
Subject: [PATCH] config: warn on inaccessible files
Before reading a config file, we check !access(path, R_OK)
to make sure that the file exists and is readable. If it's
not, then we silently ignore it.
For the case of ENOENT, this is fine, as the presence of the
file is optional. For other cases, though, it may indicate a
configuration error (e.g., not having permissions to read
the file). Let's print a warning in these cases to let the
user know.
And this might be a good follow-on:
-- 8 --
Subject: [PATCH] gitignore: report access errors of exclude files
When we try to access gitignore files, we check for their
existence with a call to access. We silently ignore
missing files. However, if a file is not readable, this may
be a configuration error; let's warn the user.
For $GIT_DIR/info/excludes or core.excludesfile, we can just
use access_or_warn. However, for per-directory files we
actually try to open them, so we must add a custom warning.
Signed-off-by: Jeff King p...@peff.net
---
dir.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 240bf0c..4ee16b5 100644
--- a/dir.c
+++ b/dir.c
@@ -397,6 +397,8 @@ int add_excludes_from_file_to_list(const char *fname,
fd = open(fname, O_RDONLY);
if (fd 0 || fstat(fd, st) 0) {
+ if (errno != ENOENT)
+ warn(_(unable to access '%s': %s), fname,
strerror(errno));
if (0 = fd)
close(fd);
if (!check_index ||
@@ -1311,9 +1313,9 @@ void setup_standard_excludes(struct dir_struct *dir)
home_config_paths(NULL, xdg_path, ignore);
excludes_file = xdg_path;
}
- if (!access(path, R_OK))
+ if (!access_or_warn(path, R_OK))
add_excludes_from_file(dir, path);
- if (excludes_file !access(excludes_file, R_OK))
+ if (excludes_file !access_or_warn(excludes_file, R_OK))
add_excludes_from_file(dir, excludes_file);
}
--
1.7.12.4.g4e9f38f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Tue, Aug 21, 2012 at 02:22:19AM -0400, Jeff King wrote:
And this might be a good follow-on:
-- 8 --
Subject: [PATCH] gitignore: report access errors of exclude files
And if we are going to do that, then we almost certainly want to do
this.
-- 8 --
Subject: [PATCH] attr: warn on inaccessible attribute files
Just like config and gitignore files, we silently ignore
missing or inaccessible attribute files. An existent but
inaccessible file is probably a configuration error, so
let's warn the user.
Signed-off-by: Jeff King p...@peff.net
---
attr.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index b52efb5..cab01b8 100644
--- a/attr.c
+++ b/attr.c
@@ -352,8 +352,11 @@ static struct attr_stack *read_attr_from_file(const char
*path, int macro_ok)
char buf[2048];
int lineno = 0;
- if (!fp)
+ if (!fp) {
+ if (errno != ENOENT)
+ warning(_(unable to access '%s': %s), path,
strerror(errno));
return NULL;
+ }
res = xcalloc(1, sizeof(*res));
while (fgets(buf, sizeof(buf), fp))
handle_attr_line(res, buf, path, ++lineno, macro_ok);
--
1.7.12.4.g4e9f38f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Tue, Aug 21, 2012 at 09:50:27AM -0700, Junio C Hamano wrote: Subject: [PATCH] gitignore: report access errors of exclude files When we try to access gitignore files, we check for their existence with a call to access. We silently ignore missing files. However, if a file is not readable, this may be a configuration error; let's warn the user. For $GIT_DIR/info/excludes or core.excludesfile, we can just use access_or_warn. However, for per-directory files we actually try to open them, so we must add a custom warning. There are a couple of users of add_excludes_from_file() that is outside the per-directory walking in ls-files and unpack-trees; I think both are OK with this change, but the one in ls-files may want to issue a warning or even an error upon ENOENT. Not a regression with this patch; just something we may want to do while we are in the vicinity. The two I see are: 1. unpack-trees:verify_absent This looks like it is reading info/sparse-checkout. But I think it is OK for that file to be missing, no? 2. ls-files:option_parse_exclude_from This handles --exclude-from. I would expect most callers to be converted to --exclude-standard these days, but originally callers did something like: git ls-files \ --exclude-from=$GIT_DIR/info/exclude \ --exclude-per-directory=.gitignore \ ... While it would be friendlier to a user calling ls-files to warn about a missing entry in the first case (since they explicitly typed it, they presumably expect it to work). But for a script calling the ls-files plumbing, that --exclude-from has always meant if it's there, use it, but otherwise, don't worry. Probably no such callers exist anymore, but complaining would be a regression for them. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
Junio C Hamano gits...@pobox.com writes:
Modulo the above you might want to turn the call to warn() to
another helper that can be used from elsewhere, this patch looks
perfect to me.
And that modulo is fairly simple if we wanted to go that route.
attr.c| 2 +-
dir.c | 2 +-
git-compat-util.h | 3 +++
wrapper.c | 7 ++-
4 files changed, 11 insertions(+), 3 deletions(-)
diff --git c/attr.c w/attr.c
index cab01b8..f12c83f 100644
--- c/attr.c
+++ w/attr.c
@@ -354,7 +354,7 @@ static struct attr_stack *read_attr_from_file(const char
*path, int macro_ok)
if (!fp) {
if (errno != ENOENT)
- warning(_(unable to access '%s': %s), path,
strerror(errno));
+ warn_on_inaccessible(path);
return NULL;
}
res = xcalloc(1, sizeof(*res));
diff --git c/dir.c w/dir.c
index ea74048..4868339 100644
--- c/dir.c
+++ w/dir.c
@@ -398,7 +398,7 @@ int add_excludes_from_file_to_list(const char *fname,
fd = open(fname, O_RDONLY);
if (fd 0 || fstat(fd, st) 0) {
if (errno != ENOENT)
- warning(_(unable to access '%s': %s), fname,
strerror(errno));
+ warn_on_inaccessible(fname);
if (0 = fd)
close(fd);
if (!check_index ||
diff --git c/git-compat-util.h w/git-compat-util.h
index 5a520e2..42d 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -607,6 +607,9 @@ int remove_or_warn(unsigned int mode, const char *path);
/* Call access(2), but warn for any error besides ENOENT. */
int access_or_warn(const char *path, int mode);
+/* Warn on an inaccessible file that ought to be accessible */
+void warn_on_inaccessible(const char *path);
+
/* Get the passwd entry for the UID of the current process. */
struct passwd *xgetpwuid_self(void);
diff --git c/wrapper.c w/wrapper.c
index b40c7e7..68739aa 100644
--- c/wrapper.c
+++ w/wrapper.c
@@ -403,11 +403,16 @@ int remove_or_warn(unsigned int mode, const char *file)
return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
}
+void warn_on_inaccessible(const char *path)
+{
+ warning(_(unable to access '%s': %s), path, strerror(errno));
+}
+
int access_or_warn(const char *path, int mode)
{
int ret = access(path, mode);
if (ret errno != ENOENT)
- warning(_(unable to access '%s': %s), path, strerror(errno));
+ warn_on_inaccessible(path);
return ret;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Tue, Aug 21, 2012 at 02:52:02PM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Modulo the above you might want to turn the call to warn() to another helper that can be used from elsewhere, this patch looks perfect to me. And that modulo is fairly simple if we wanted to go that route. attr.c| 2 +- dir.c | 2 +- git-compat-util.h | 3 +++ wrapper.c | 7 ++- 4 files changed, 11 insertions(+), 3 deletions(-) Yeah, that looks fine to me if you want to squash it in. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
John Arthorne arthorne.ecli...@gmail.com writes: For all the details see this bugzilla, particularly comment #59 where we finally narrowed this down: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 What does at the system level in your does *not* work at the system level. exactly mean? A configuration variable in the repository configuration take precedence over user preference $HOME/.gitconfig which in turn take precedence over system wide default /etc/gitconfig (or whereever you or your distro decided to place it). In other words, if you have [receive] denyNonFastForwards in the system wide default, you can say [receive] denyNonFastForwards = false in one particular repository to allow it for that repository. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
Sitaram Chamarty sitar...@gmail.com writes: On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano gits...@pobox.com wrote: John Arthorne arthorne.ecli...@gmail.com writes: For all the details see this bugzilla, particularly comment #59 where we finally narrowed this down: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 What does at the system level in your does *not* work at the system level. exactly mean? git config --system receive.denynonfastforwards true is not honored. At all. (And I checked there was nothing overriding it). --global does work (is honored). Tested on 1.7.11 Thanks, and interesting. Does anybody recall if this is something we did on purpose? After eyeballing the callchain starting from cmd_receive_pack() down to receive_pack_config(), nothing obvious jumps at me. Could this be caused by a chrooted environment not having /etc/gitconfig (now I am just speculating)? A quick strace -f -o /tmp/tr git push ../neigh seems to indicate that at least access() is called on /etc/gitconfig as I expect, which makes me think that near the beginning of git_config_early(), we would read from /etc/gitconfig if the file existed (I do not install any distro git, so there is no /etc/gitconfig on my box). Puzzled. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Mon, Aug 20, 2012 at 6:22 PM, Junio C Hamano gits...@pobox.com wrote: Sitaram Chamarty sitar...@gmail.com writes: On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano gits...@pobox.com wrote: John Arthorne arthorne.ecli...@gmail.com writes: For all the details see this bugzilla, particularly comment #59 where we finally narrowed this down: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 What does at the system level in your does *not* work at the system level. exactly mean? git config --system receive.denynonfastforwards true is not honored. At all. (And I checked there was nothing overriding it). --global does work (is honored). Tested on 1.7.11 Thanks, and interesting. Does anybody recall if this is something we did on purpose? After eyeballing the callchain starting from cmd_receive_pack() down to receive_pack_config(), nothing obvious jumps at me. Could this be caused by a chrooted environment not having /etc/gitconfig (now I am just speculating)? A quick strace -f -o /tmp/tr git push ../neigh seems to indicate that at least access() is called on /etc/gitconfig as I expect, which makes me think that near the beginning of git_config_early(), we would read from /etc/gitconfig if the file existed (I do not install any distro git, so there is no /etc/gitconfig on my box). Puzzled. Seems to work for me. Force push was denied when receive.denyNonFastForwards was set to true in system-level gitconfig. Tested with git installed in my home directory, so my system-level gitconfig was at $HOME/etc/gitconfig. Sitaram and John, are you sure you modified the correct file? Also be sure you're using the git-receive-pack that expects the system gitconfig at the place that you think it is. The system-level gitconfig is hard-coded in the git binary and may not always be at /etc/gitconfig. It is usually set to be relative to the installation directory $prefix in the Makefile. I don't think we expose the path to the system-level gitconfig file anywhere in the ui. One way to figure out where it should be is to use 'git config' to edit it like this: git config --system -e Hopefully your editor exposes the path that it is editing even if you don't have permission to modify it. I'm thinking that the git-receive-pack binary that you guys used expects the system gitconfig to be in a different location than the one you modified. -Brandon -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote: git config --system receive.denynonfastforwards true is not honored. At all. (And I checked there was nothing overriding it). --global does work (is honored). Tested on 1.7.11 Thanks, and interesting. Does anybody recall if this is something we did on purpose? After eyeballing the callchain starting from cmd_receive_pack() down to receive_pack_config(), nothing obvious jumps at me. No, I do not think it was on purpose. And it would be very hard to do so, anyway; config callbacks are not given any information about the source of the config variable, and cannot distinguish between repo, global, and system-level config variables. Could this be caused by a chrooted environment not having /etc/gitconfig (now I am just speculating)? That seems far more likely to me. Another possibility is that the file is not readable by the user running receive-pack. A quick strace -f -o /tmp/tr git push ../neigh seems to indicate that at least access() is called on /etc/gitconfig as I expect, which makes me think that near the beginning of git_config_early(), we would read from /etc/gitconfig if the file existed (I do not install any distro git, so there is no /etc/gitconfig on my box). I just did a few quick tests both across local repos and across an ssh session. receive.denynonfastforwards worked just fine in my /etc/gitconfig in both cases. So the likely cause would be that git cannot access that file for some reason (chroot or permissions). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Tue, Aug 21, 2012 at 6:52 AM, Junio C Hamano gits...@pobox.com wrote: Sitaram Chamarty sitar...@gmail.com writes: On Mon, Aug 20, 2012 at 10:35 PM, Junio C Hamano gits...@pobox.com wrote: John Arthorne arthorne.ecli...@gmail.com writes: For all the details see this bugzilla, particularly comment #59 where we finally narrowed this down: https://bugs.eclipse.org/bugs/show_bug.cgi?id=343150 What does at the system level in your does *not* work at the system level. exactly mean? git config --system receive.denynonfastforwards true is not honored. At all. (And I checked there was nothing overriding it). --global does work (is honored). Tested on 1.7.11 Thanks, and interesting. Uggh. My fault this one. I had a very tight umask on root, and running 'git config --system' created an /etc/gitconfig that was not readable by a normal user. Running strace clued me in... John: maybe it's as simple as that in your case too. Junio/Brandon/Jeff: sorry for the false corroboration of John's report! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey draf...@gmail.com wrote: git config --system -e Hopefully your editor exposes the path that it is editing even if you don't have permission to modify it. GIT_EDITOR=echo git config --system -e works for me. j. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
Jay Soffian jaysoff...@gmail.com writes: On Mon, Aug 20, 2012 at 9:53 PM, Brandon Casey draf...@gmail.com wrote: git config --system -e Hopefully your editor exposes the path that it is editing even if you don't have permission to modify it. GIT_EDITOR=echo git config --system -e works for me. Clever ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: receive.denyNonNonFastForwards not denying force update
Jeff King p...@peff.net writes: On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote: Does anybody recall if this is something we did on purpose? After eyeballing the callchain starting from cmd_receive_pack() down to receive_pack_config(), nothing obvious jumps at me. No, I do not think it was on purpose. And it would be very hard to do so, anyway; config callbacks are not given any information about the source of the config variable, and cannot distinguish between repo, global, and system-level config variables. I was looking for setenv() to refuse system wide defaults; that actually is fairly simple. Could this be caused by a chrooted environment not having /etc/gitconfig (now I am just speculating)? That seems far more likely to me. Another possibility is that the file is not readable by the user running receive-pack. Good point. We explicitly use access(R_OK) and pretend as if a path that is known to exist but not readable is missing; perhaps we may want to diagnose this as a misconfiguration and issue a warning? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
