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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 14, 2012, 04:55 UTC
Message-ID: <7vhapx1wlh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20121014000210.GA19094@elie.Belkin>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 33 quoted lines

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