Re: [PATCH 2/2] read_info_alternates: warn on non-trivial errors

From: Jeff King <peff@peff.net>
Date: Sep 19, 2017, 05:15 UTC
Message-ID: <20170919051505.qdfycovqlcqa6n5c@sigill.intra.peff.net>
In-Reply-To: <20170919024623.GD175206@aiede.mtv.corp.google.com>

On Mon, Sep 18, 2017 at 07:46:24PM -0700, Jonathan Nieder wrote:

Show 24 quoted lines

> Jeff King wrote:
> 
> > When we fail to open $GIT_DIR/info/alternates, we silently
> > assume there are no alternates. This is the right thing to
> > do for ENOENT, but not for other errors.
> >
> > A hard error is probably overkill here. If we fail to read
> > an alternates file then either we'll complete our operation
> > anyway, or we'll fail to find some needed object. Either
> > way, a warning is good idea. And we already have a helper
> > function to handle this pattern; let's just call
> > warn_on_fopen_error().
> 
> I think I prefer a hard error.  What kind of cases are you imagining
> where it would be better to warn?
> 
> E.g. for EIO, erroring out so that the user can try again seems better
> than hoping that the application will be able to cope with the more
> subtle error that comes from discovering some objects are missing.
> 
> For EACCES, I can see how it makes sense to warn and move on, but no
> other errors like that are occuring to me.
> 
> Thoughts?

Yes, EACCES is exactly the example that came to mind. But most importantly, we don't know at this point whether the alternate is even relevant to the current operation. So calling die() here would make some cases fail that would otherwise succeed. That's usually not a big deal, but we've had cases where being lazy about dying helps people use git itself to help repair the case.

Admittedly most of those chicken-and-egg problems are centered around git-config (e.g., using "git config --edit" to repair broken config), so it's perhaps less important here. But it seems like a reasonable principle to follow in general.

If there's a compelling reason to die hard, I'd reconsider it. But I can't really think of one (if we were _writing_ a new alternates file and encountered a read error of the old data we're copying, then I think it would be very important to bail before claiming a successful write. But that's not what's happening here).

-Peff