[RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files

Hi Jeff,

In August, Jeff King wrote:

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.

git became noisy:

 $ git fetch --all
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 Fetching origin
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 Fetching charon
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 [...]

On this machine, ~/.config/git has been a regular file for a while,
with ~/.gitconfig a symlink to it.  Probably ENOTDIR should be ignored
just like ENOENT is.  Except for the noise, the behavior is fine, but
something still feels wrong.  

When ~/.gitconfig is unreadable (EPERM), the messages are a symptom of
an older issue: the config file is being ignored.  Shouldn't git error
out instead so the permissions can be fixed?  E.g., if the sysadmin
has set "[branch] autoSetupRebase" to true in /etc/gitconfig and I
have set it to false in my own ~/.gitconfig, I'd rather see git error
out because ~/.gitconfig has become unreadable in a chmod gone wrong
than have a branch set up with the wrong settings and have to learn to
fix it up myself.

In other words, how about something like this?

Jonathan Nieder (2):
  config, gitignore: failure to access with ENOTDIR is ok
  config: treat user and xdg config permission problems as errors

 config.c          |  4 ++--
 git-compat-util.h |  6 +++++-
 wrapper.c         | 10 +++++++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

The access_or_warn() function is used to check for optional
configuration files like .gitconfig and .gitignore and warn when they
are not accessible due to a configuration issue (e.g., bad
permissions).  It is not supposed to complain when a file is simply
missing.

Noticed on a system where ~/.config/git was a file --- when the new
XDG_CONFIG_HOME support looks for ~/.config/git/config it should
ignore ~/.config/git instead of printing irritating warnings:

 $ git status -s
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
 warning: unable to access '/home/jrn/.config/git/config': Not a directory

Compare v1.7.12.1~2^2 (attr:failure to open a .gitattributes file
is OK with ENOTDIR, 2012-09-13).

Signed-off-by: Jonathan Nieder <redacted>
---
 git-compat-util.h | 5 ++++-
 wrapper.c         | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 2fbf1fd8..f567767f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -635,7 +635,10 @@ 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. */
+/*
+ * Call access(2), but warn for any error except "missing file"
+ * (ENOENT or ENOTDIR).
+ */
 int access_or_warn(const char *path, int mode);
 
 /* Warn on an inaccessible file that ought to be accessible */

diff --git a/wrapper.c b/wrapper.c
index 68739aaa..c1b919f3 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -411,7 +411,7 @@ void warn_on_inaccessible(const char *path)
 int access_or_warn(const char *path, int mode)
 {
 	int ret = access(path, mode);
-	if (ret && errno != ENOENT)
+	if (ret && errno != ENOENT && errno != ENOTDIR)
 		warn_on_inaccessible(path);
 	return ret;
 }

-- 
1.8.0.rc2

Git reads multiple configuration files: settings come first from the
system config file (typically /etc/gitconfig), then the xdg config
file (typically ~/.config/git/config), then the user's dotfile
(~/.gitconfig), then the repository configuration (.git/config).

Git has always used access(2) to decide whether to use each file; as
an unfortunate side effect, that means that if one of these files is
unreadable (e.g., EPERM or EIO), git skips it.  So if I use
~/.gitconfig to override some settings but make a mistake and give it
the wrong permissions then I am subject to the settings the sysadmin
chose for /etc/gitconfig.

Better to error out and ask the user to correct the problem.

This only affects the user and xdg config files, since the user
presumably has enough access to fix their permissions.  If the system
config file is unreadable, the best we can do is to warn about it so
the user knows to notify someone and get on with work in the meantime.

Signed-off-by: Jonathan Nieder <redacted>
---
 config.c          | 4 ++--
 git-compat-util.h | 1 +
 wrapper.c         | 8 ++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 08e47e2e..e8875b8a 100644
--- a/config.c
+++ b/config.c

@@ -945,12 +945,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	if (xdg_config && !access_or_warn(xdg_config, R_OK)) {
+	if (xdg_config && !access_or_die(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (user_config && !access_or_warn(user_config, R_OK)) {
+	if (user_config && !access_or_die(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}

diff --git a/git-compat-util.h b/git-compat-util.h
index f567767f..dfe2e318 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -640,6 +640,7 @@ int remove_or_warn(unsigned int mode, const char *path);
  * (ENOENT or ENOTDIR).
  */
 int access_or_warn(const char *path, int mode);
+int access_or_die(const char *path, int mode);
 
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);

diff --git a/wrapper.c b/wrapper.c
index c1b919f3..7cbe96a0 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -416,6 +416,14 @@ int access_or_warn(const char *path, int mode)
 	return ret;
 }
 
+int access_or_die(const char *path, int mode)
+{
+	int ret = access(path, mode);
+	if (ret && errno != ENOENT && errno != ENOTDIR)
+		die_errno(_("unable to access '%s'"), path);
+	return ret;
+}
+
 struct passwd *xgetpwuid_self(void)
 {
 	struct passwd *pw;

-- 
1.8.0.rc2

On Sat, Oct 13, 2012 at 05:02:10PM -0700, Jonathan Nieder wrote:

quoted

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.

git became noisy:

 $ git fetch --all
 warning: unable to access '/home/jrn/.config/git/config': Not a directory
[...]

I somehow thought that we had dealt with this ENOTDIR already, but I see
that 8e950da only dealt with .gitattributes, which may look for
arbitrary path names that are not reflected in the current working tree.

We didn't ignore ENOTDIR for config files at the same time, because it
is not obvious that such a bogus config path is not something the user
would want to know about.

On this machine, ~/.config/git has been a regular file for a while,
with ~/.gitconfig a symlink to it.  Probably ENOTDIR should be ignored
just like ENOENT is.  Except for the noise, the behavior is fine, but
something still feels wrong.

Hmm. Your use of ~/.config/git is interesting. Recent versions of git
will look in ~/.config (or $XDG_CONFIG_HOME), but they want to find
"git/config" there, and your single file is in conflict with that. So
this has nothing to do with ~/.gitconfig, or the fact that it is
symlinked. This is the XDG lookup code kicking in, because you happened
to put your file in the same place, and then afterwards git learned to
look there (albeit with a slightly different format).

So on the one hand, this ENOTDIR is uninteresting, because it is not
really about an error with the file we are trying to open at all, but
simply another way of saying "the file does not exist". And therefore it
should be ignored.

On the other hand, it is actually alerting you to an unusual situation
that you might want to fix (you are putting stuff in the XDG config
directory, but it is not in the format git wants).

I don't have a strong preference about what should happen, but I would
lean towards your first patch. ENOTDIR really is just another way of
saying ENOENT (it just gives more information about the leading paths).
It did find a configuration oddity you might want to fix, but that
oddity was not actually hurting anything.

When ~/.gitconfig is unreadable (EPERM), the messages are a symptom of
an older issue: the config file is being ignored.  Shouldn't git error
out instead so the permissions can be fixed?  E.g., if the sysadmin
has set "[branch] autoSetupRebase" to true in /etc/gitconfig and I
have set it to false in my own ~/.gitconfig, I'd rather see git error
out because ~/.gitconfig has become unreadable in a chmod gone wrong
than have a branch set up with the wrong settings and have to learn to
fix it up myself.

This is a separate issue from above. I tend to agree that dying would be
better in most cases, because an operation may not do what you want if
opening the config fails (for an even worse example, considering
something like receive-pack trying to figure out if receive.denyDeletes
is set).

I considered doing this when I wrote the original patch, but was mainly
worried about regressions in weird situations. The two I can think of
are:

  1. You are inspecting somebody else's repo, but you do not have access
     to their .git/config file. But then, I think that is probably a
     sane time to die anyway, since we cannot read core.repositoryFormatVersion.

  2. You have used sudo or some other tool to switch uid, but your
     environment still points git at your original user's global config,
     which may not be readable.

Those are unusual situations, though. It probably makes more sense for
us to be conservative in the common case and die. Case 1 is pretty
insane and should probably involve dying anyway. Case 2 people may be
inconvenienced (they would rather see the harmless warning and continue
the operation), but they can work around it by setting up their
environment properly after switching uids.

In other words, how about something like this?

Jonathan Nieder (2):
  config, gitignore: failure to access with ENOTDIR is ok
  config: treat user and xdg config permission problems as errors

Yeah, those look sane, modulo a question about the second one (I'll
reply directly).

-Peff

On Sat, Oct 13, 2012 at 05:04:02PM -0700, Jonathan Nieder wrote:

Better to error out and ask the user to correct the problem.

This only affects the user and xdg config files, since the user
presumably has enough access to fix their permissions.  If the system
config file is unreadable, the best we can do is to warn about it so
the user knows to notify someone and get on with work in the meantime.

I'm on the fence about treating the systme config specially. On the one
hand, I see the convenience if somebody has a bogus /etc/gitconfig and
gets EPERM but can't fix it. On the other hand, if we get EIO, isn't
that a good indication that we would want to die?

For example, servers may depend on /etc/gitconfig to enforce security
policy (e.g., setting transfer.fsckObjects or receive.deny*). Perhaps
our default should be safe, and people can use GIT_CONFIG_NOSYSTEM to
work around a broken machine.

-Peff

Jeff King wrote:

For example, servers may depend on /etc/gitconfig to enforce security
policy (e.g., setting transfer.fsckObjects or receive.deny*). Perhaps
our default should be safe, and people can use GIT_CONFIG_NOSYSTEM to
work around a broken machine.

Very good point.  How about these patches on top?

Jonathan Nieder (2):
  config doc: advertise GIT_CONFIG_NOSYSTEM
  config: exit on error accessing any config file

 Documentation/git-config.txt | 8 ++++++++
 config.c                     | 6 +++---
 2 files changed, 11 insertions(+), 3 deletions(-)

When a syntax error or other problem renders /etc/gitconfig buggy on a
multiuser system where mortals do not have write access to /etc, the
GIT_CONFIG_NOSYSTEM variable is the best tool we have to keep getting
work done until the sysadmin sorts the problem out.

Noticed while experimenting with teaching git to error out when
/etc/gitconfig is unreadable.

Signed-off-by: Jonathan Nieder <redacted>
---
 Documentation/git-config.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index eaea0791..907a1fd5 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -240,6 +240,14 @@ GIT_CONFIG::
 	Using the "--global" option forces this to ~/.gitconfig. Using the
 	"--system" option forces this to $(prefix)/etc/gitconfig.
 
+GIT_CONFIG_NOSYSTEM::
+	Whether to skip reading settings from the system-wide
+	$(prefix)/etc/gitconfig file.  This environment variable can
+	be used along with HOME and XDG_CONFIG_HOME to create a
+	predictable environment for a picky script, or you can set it
+	temporarily to avoid using a buggy /etc/gitconfig file while
+	waiting for someone with sufficient permissions to fix it.
+
 See also <<FILES>>.
 
 

-- 
1.8.0.rc2

There is convenience in warning and moving on when somebody has a
bogus permissions on /etc/gitconfig and cannot do anything about it.
But the cost in predictability and security is too high --- when
unreadable config files are skipped, it means an I/O error or
permissions problem causes important configuration to be bypassed.

For example, servers may depend on /etc/gitconfig to enforce security
policy (setting transfer.fsckObjects or receive.deny*).  Best to
always error out when encountering trouble accessing a config file.

This may add inconvenience in some cases:

  1. You are inspecting somebody else's repo, and you do not have
     access to their .git/config file.  Git typically dies in this
     case already since we cannot read core.repositoryFormatVersion,
     so the change should not be too noticeable.

  2. You have used "sudo -u" or a similar tool to switch uid, and your
     environment still points Git at your original user's global
     config, which is not readable.  In this case people really would
     be inconvenienced (they would rather see the harmless warning and
     continue the operation) but they can work around it by setting
     HOME appropriately after switching uids.

  3. You do not have access to /etc/gitconfig due to a broken setup.
     In this case, erroring out is a good way to put pressure on the
     sysadmin to fix the setup.  While they wait for a reply, users
     can set GIT_CONFIG_NOSYSTEM to true to keep Git working without
     complaint.

After this patch, errors accessing the repository-local and systemwide
config files and files requested in include directives cause Git to
exit, just like errors accessing ~/.gitconfig.

Explained-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 config.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index e8875b8a..a4d153f6 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_or_warn(path, R_OK)) {
+	if (!access_or_die(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,7 +939,7 @@ 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_or_warn(git_etc_gitconfig(), R_OK)) {
+	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK)) {
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
 		found += 1;

@@ -955,7 +955,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	if (repo_config && !access_or_warn(repo_config, R_OK)) {
+	if (repo_config && !access_or_die(repo_config, R_OK)) {
 		ret += git_config_from_file(fn, repo_config, data);
 		found += 1;
 	}

-- 
1.8.0.rc2

On a multiuser system where mortals do not have write access to /etc,
the GIT_CONFIG_NOSYSTEM variable is the best tool we have to keep
getting work done when a syntax error or other problem renders
/etc/gitconfig buggy, until the sysadmin sorts the problem out.

Noticed while experimenting with teaching git to error out when
/etc/gitconfig is unreadable.

Signed-off-by: Jonathan Nieder <redacted>
---
Jonathan Nieder wrote:

Hm, unlike GIT_CONFIG this applies to all git commands (not just "git
config"), so it is misleading to document them in the same place.
Here's a better patch.

 Documentation/git-config.txt | 4 ++++
 Documentation/git.txt        | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index eaea0791..9ae2508f 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -240,6 +240,10 @@ GIT_CONFIG::
 	Using the "--global" option forces this to ~/.gitconfig. Using the
 	"--system" option forces this to $(prefix)/etc/gitconfig.
 
+GIT_CONFIG_NOSYSTEM::
+	Whether to skip reading settings from the system-wide
+	$(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
+
 See also <<FILES>>.
 
 

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d1d227a3..ae1f833a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -757,6 +757,14 @@ for further details.
 	and read the password from its STDOUT. See also the 'core.askpass'
 	option in linkgit:git-config[1].
 
+'GIT_CONFIG_NOSYSTEM'::
+	Whether to skip reading settings from the system-wide
+	`$(prefix)/etc/gitconfig` file.  This environment variable can
+	be used along with `$HOME` and `$XDG_CONFIG_HOME` to create a
+	predictable environment for a picky script, or you can set it
+	temporarily to avoid using a buggy `/etc/gitconfig` file while
+	waiting for someone with sufficient permissions to fix it.
+
 'GIT_FLUSH'::
 	If this environment variable is set to "1", then commands such
 	as 'git blame' (in incremental mode), 'git rev-list', 'git log',

-- 
1.8.0.rc2

On Sun, Oct 14, 2012 at 01:42:44AM -0700, Jonathan Nieder wrote:

Jeff King wrote:

quoted

For example, servers may depend on /etc/gitconfig to enforce security
policy (e.g., setting transfer.fsckObjects or receive.deny*). Perhaps
our default should be safe, and people can use GIT_CONFIG_NOSYSTEM to
work around a broken machine.

Very good point.  How about these patches on top?

Jonathan Nieder (2):
  config doc: advertise GIT_CONFIG_NOSYSTEM
  config: exit on error accessing any config file

 Documentation/git-config.txt | 8 ++++++++
 config.c                     | 6 +++---
 2 files changed, 11 insertions(+), 3 deletions(-)

This is my absolute favorite type of reply: the kind that you can apply
with "git am".

The direction and the patches themselves look good to me. I agree with
your reasoning in v2 of 3/2; it makes much more sense than v1.

Thanks.

-Peff