git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, Michael Haggerty <mhagger@alum.mit.edu>
Subject: Re: [PATCH 2/2] read_info_alternates: warn on non-trivial errors
Date: Tue, 19 Sep 2017 01:15:05 -0400	[thread overview]
Message-ID: <20170919051505.qdfycovqlcqa6n5c@sigill.intra.peff.net> (raw)
In-Reply-To: <20170919024623.GD175206@aiede.mtv.corp.google.com>

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

> 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

  reply	other threads:[~2017-09-19  5:15 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-18 15:51 [PATCH 0/2] fix read past end of array in alternates files Jeff King
2017-09-18 15:54 ` [PATCH 1/2] read_info_alternates: read contents into strbuf Jeff King
2017-09-19  0:08   ` Junio C Hamano
2017-09-19  2:42   ` Jonathan Nieder
2017-09-19  5:03     ` Jeff King
2017-09-18 15:55 ` [PATCH 2/2] read_info_alternates: warn on non-trivial errors Jeff King
2017-09-19  2:46   ` Jonathan Nieder
2017-09-19  5:15     ` Jeff King [this message]
2017-09-19  2:36 ` [PATCH 0/2] fix read past end of array in alternates files Jonathan Nieder
2017-09-19  4:57   ` Jeff King
2017-09-19 19:40 ` [PATCH v2 " Jeff King
2017-09-19 19:41   ` [PATCH v2 1/2] read_info_alternates: read contents into strbuf Jeff King
2017-09-19 19:41   ` [PATCH v2 2/2] read_info_alternates: warn on non-trivial errors Jeff King
2017-09-20  2:44   ` [PATCH v2 0/2] fix read past end of array in alternates files Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170919051505.qdfycovqlcqa6n5c@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=mhagger@alum.mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).