Re: [RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files
On Sat, Oct 13, 2012 at 05:02:10PM -0700, Jonathan Nieder 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 [...] 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 -- 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: [RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files
On Sat, Oct 13, 2012 at 09:55:22PM -0700, Junio C Hamano wrote:
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?
I think that is a reasonable issue to address, but I wonder if we
should be sharing more code between these. If the config side can
be switched to unconditionally attempt to fopen and then deal with
an error when it happens, we can get rid of access_or_{warn,die}
and replace them with fopen_or_{warn,die} and use them from the two
places (attr.c:read_attr_from_file() and the configuration stuff).
I haven't looked to see if that a too intrusive refactoring to be
worth it, though.
Handling the error at the fopen level would eliminate a minor race
condition (e.g., access() reports OK, then the file has its permissions
changed, then we fopen and get EPERM, but ignore it). So it would not
just be a refactoring, but would actually improve the code quality.
-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: [RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files
Junio C Hamano wrote:
If the config side can
be switched to unconditionally attempt to fopen and then deal with
an error when it happens, we can get rid of access_or_{warn,die}
and replace them with fopen_or_{warn,die} and use them from the two
places (attr.c:read_attr_from_file() and the configuration stuff).
I haven't looked to see if that a too intrusive refactoring to be
worth it, though.
That sounds reasonable, but I'm punting on it. The first step would
be tweaking the git_config_from_file() calling convention to convey
missing file errors specially, perhaps by making sure errno is
meaningful when the return value is -1, and that already sounds like
work. ;-)
Thanks,
Jonathan
--
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
[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(-) -- 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: [RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files
Jonathan Nieder jrnie...@gmail.com writes:
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
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?
I think that is a reasonable issue to address, but I wonder if we
should be sharing more code between these. If the config side can
be switched to unconditionally attempt to fopen and then deal with
an error when it happens, we can get rid of access_or_{warn,die}
and replace them with fopen_or_{warn,die} and use them from the two
places (attr.c:read_attr_from_file() and the configuration stuff).
I haven't looked to see if that a too intrusive refactoring to be
worth it, though.
--
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
