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

Jeff King [off-list ref] writes:

On Wed, May 17, 2017 at 02:05:42PM +0200, Michael Haggerty wrote:

quoted

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.

quoted

+	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;
	}

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

quoted

quoted

+	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