From: Jeff King <peff@peff.net>
Date: Sun, 14 Oct 2012 06:26:50 GMT
Subject: Re: [RFC/PATCH 0/2] Re: [PATCH] config: warn on inaccessible files
Message-ID: <20121014062650.GC13477@sigill.intra.peff.net>
In-Reply-To: <7vhapx1wlh.fsf@alter.siamese.dyndns.org>

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