Re: [PATCH 00/15] Handle fopen() errors

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2017, 03:41 UTC
Message-ID: <xmqqk26e5swj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqlgqu7cq8.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 6 quoted lines

> I wonder if it is OK to only special case ENOENT for !fp cases,
> where existing code silently returns.  Perhaps it is trying to read
> an optional file, and it returns silently because lack of it is
> perfectly OK for the purpose of the code.  Are there cases where
> this optional file is inside an optional directory, leading to
> ENOTDIR, instead of ENOENT, observed and reported by your check?

"git grep -B1 warn_on_inaccessible" is enlightening. I wonder if we want to wrap the two lines into a hard to misuse helper function, something along this line. Would having this patch as a preparatory step shrink your series? The patch count would be the same, but you wouldn't be writing "if (errno != ENOENT)" lines yourself.

 attr.c            | 3 +--
 git-compat-util.h | 3 +++
 wrapper.c         | 6 ++++++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 1fcf042b87..f695ded53f 100644
--- a/attr.c
+++ b/attr.c
@@ -373,8 +373,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 	int lineno = 0;
 
 	if (!fp) {
-		if (errno != ENOENT && errno != ENOTDIR)
-			warn_on_inaccessible(path);
+		warn_failure_to_read_open_optional_path(path);
 		return NULL;
 	}
 	res = xcalloc(1, sizeof(*res));
diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..998366c628 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1094,6 +1094,9 @@ int access_or_die(const char *path, int mode, unsigned flag);
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
 
+/* Call the above after fopen/open fails for optional input */
+void warn_failure_to_read_open_optional_path(const char *);
+
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
 struct tm *git_gmtime_r(const time_t *, struct tm *);
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..172cb9fad6 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -576,6 +576,12 @@ int remove_or_warn(unsigned int mode, const char *file)
 	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
 }
 
+void warn_failure_to_read_open_optional_path(const char *path)
+{
+	if (errno != ENOENT && errno != ENOTDIR)
+		warn_on_inaccessible(path);
+}
+
 void warn_on_inaccessible(const char *path)
 {
 	warning_errno(_("unable to access '%s'"), path);