Re: [PATCH 19/23] read_packed_refs(): report unexpected fopen() failures

From: Jeff King <peff@peff.net>
Date: May 18, 2017, 05:08 UTC
Message-ID: <20170518050849.kopykuewhfq3psav@sigill.intra.peff.net>
In-Reply-To: <xmqqwp9eu5h2.fsf@gitster.mtv.corp.google.com>

On Thu, May 18, 2017 at 01:57:45PM +0900, Junio C Hamano wrote:

Show 25 quoted lines

> >> +	if (!f) {
> >> +		if (errno == ENOENT) {
> >> +			/*
> >> +			 * This is OK; it just means that no
> >> +			 * "packed-refs" file has been written yet,
> >> +			 * which is equivalent to it being empty.
> >> +			 */
> >> +			return packed_refs;
> >> +		} else {
> >> +			die("couldn't read %s: %s",
> >> +			    packed_refs_file, strerror(errno));
> >> +		}
> >
> > I think this could be die_errno().
> 
> I wonder what the endgame shape of this code should be, when it and
> nd/fopen-errors topic both graduate.  We cannot use fopen_or_warn(),
> as we not just want to warn but want to die, e.g.
> 
> 	f = fopen(...);
> 	if (!f) {
>         	if (warn_on_fopen_errors(...))
>                 	die_erno(...);
> 		return packed_refs;
> 	}

If we go that route I think the die becomes just:
 
   if (warn_on_fopen_errors(...))
	die("unable to open packed refs");

or something. If we want a single die, perhaps the cleanest thing is to put the logic into its own function:

  static int missing_file_errno(int err)
  {
	return errno == ENOENT || errno == ENOTDIR;
  }
  ...
  if (!missing_file_errno(errno))
	die_errno("couldn't read %s", packed_refs_file);

It feels a little silly to make such a trivial helper, but the point is to encapsulate that logic. And I think it would be used inside fopen_or_warn(), and possibly other places. The name might need some work; that was the best I could come up with.

-Peff