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

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2017, 04:57 UTC
Message-ID: <xmqqwp9eu5h2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170517132839.fx6oebgqt5t65tia@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 29 quoted lines

> On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:
>
>> The old code ignored any errors encountered when trying to fopen the
>> "packed-refs" file, treating all such failures as if the file didn't
>> exist. But it could be that there is some other error opening the
>> file (e.g., permissions problems), and we don't want to silently
>> ignore such problems. So report any failures that are not due to
>> ENOENT.
>
> I think there are may be other "OK" errno values here, like ENOTDIR.
> That's probably pretty unlikely in practice, though, as we're opening a
> file at the root of the $GIT_DIR here (so somebody would had to have
> racily changed our paths). It's probably fine to just err on the side of
> safety.
>
>> +	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;
	}