Re: receive.denyNonNonFastForwards not denying force update

Jeff King [off-list ref] writes:

On Mon, Aug 20, 2012 at 06:22:26PM -0700, Junio C Hamano wrote:

quoted

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.

quoted

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?

On Mon, Aug 20, 2012 at 08:49:42PM -0700, Junio C Hamano wrote:

quoted

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.

Ah. I was thinking we had ripped those out (since they were primarily
about the test suite, and we found other ways of working around them),
but we do indeed still have GIT_CONFIG_NOSYSTEM.  So yet another
possibility is that the OP has that environment variable set for some
odd reason.

quoted

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?

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.

Signed-off-by: Jeff King <redacted>
---
This catches the common code path of git itself trying to read the
config file.  The "git config foo.bar" lookup path does not warn, as it
just tries to fopen each file (and silently bails if a file cannot be
opened).  However, since before doing its actual lookup, it would run
git_config() anyway, you will already have seen the warning.

You can get multiple warnings from this, as some programs read the
config multiple times. I don't think it's really worth caring about, as
you would want to fix such a misconfiguration quickly anyway.

A bigger question is whether people are stuck living with such a
misconfiguration (e.g., inaccessible directories made by a clueless
admin), and would be annoyed at having no way to turn this feature off.

 builtin/config.c  |  4 ++--
 config.c          | 10 +++++-----
 git-compat-util.h |  3 +++
 wrapper.c         |  8 ++++++++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..b0394ef 100644
--- a/builtin/config.c
+++ b/builtin/config.c

@@ -396,8 +396,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			 */
 			die("$HOME not set");
 
-		if (access(user_config, R_OK) &&
-		    xdg_config && !access(xdg_config, R_OK))
+		if (access_or_warn(user_config, R_OK) &&
+		    xdg_config && !access_or_warn(xdg_config, R_OK))
 			given_config_file = xdg_config;
 		else
 			given_config_file = user_config;

diff --git a/config.c b/config.c
index 2b706ea..08e47e2 100644
--- a/config.c
+++ b/config.c

@@ -60,7 +60,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access(path, R_OK)) {
+	if (!access_or_warn(path, R_OK)) {
 		if (++inc->depth > MAX_INCLUDE_DEPTH)
 			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
 			    cf && cf->name ? cf->name : "the command line");

@@ -939,23 +939,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
 	home_config_paths(&user_config, &xdg_config, "config");
 
-	if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
+	if (git_config_system() && !access_or_warn(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
 		found += 1;
 	}
 
-	if (xdg_config && !access(xdg_config, R_OK)) {
+	if (xdg_config && !access_or_warn(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access(user_config, R_OK)) {
+	if (user_config && !access_or_warn(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}
 
-	if (repo_config && !access(repo_config, R_OK)) {
+	if (repo_config && !access_or_warn(repo_config, R_OK)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}

diff --git a/git-compat-util.h b/git-compat-util.h
index 35b095e..5a520e2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -604,6 +604,9 @@ int rmdir_or_warn(const char *path);
  */
 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);
+
 /* Get the passwd entry for the UID of the current process. */
 struct passwd *xgetpwuid_self(void);
 

diff --git a/wrapper.c b/wrapper.c
index b5e33e4..b40c7e7 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -403,6 +403,14 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
+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));
+	return ret;
+}
+
 struct passwd *xgetpwuid_self(void)
 {
 	struct passwd *pw;

-- 
1.7.12.4.g4e9f38f

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 <redacted>
---
 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

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 it would probably help if I gave you the version that actually
compiled.

-- >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 <redacted>
---
 dir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 240bf0c..ea74048 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)
+			warning(_("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

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 <redacted>
---
 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