Subject: Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()

From: Junio C Hamano <[email-masked]>
Date: Wed, 03 May 2017 22:35:30 -0700

Nguyễn Thái Ngọc Duy  <[email-masked]> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>
> ---
>  dir.c             |  3 +--
>  git-compat-util.h |  2 ++
>  wrapper.c         | 10 ++++++++++
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,
>  
>  	fd = open(fname, O_RDONLY);
>  	if (fd < 0 || fstat(fd, &st) < 0) {
> -		if (errno != ENOENT)
> -			warn_on_inaccessible(fname);
> +		warn_on_fopen_errors(fname);

At least in the original, a reader can guess that, because errno
cannot be NOENT if open(2) succeeded and fstat(2) failed, we call
warn_on_inaccessible() only when we fail to open.

This change makes it harder to see why this is OK when the failure
is not in open(2) but in fstat(2).

	fd = open(fname, O_RDONLY);
	if (fd < 0 || fstat(fd, &st) < 0) {
-		if (errno != ENOENT);
-			warn_on_inaccessible(fname);
-		if (0 <= fd)
+		if (fd < 0)
+			warn_on_fopen_errors(fname);
+		else
			close(fd);
		... and the remainder of the original ...

or something like that, perhaps?

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
>  	return ret;
>  }
>  
> +int warn_on_fopen_errors(const char *path)
> +{

Does this need to be returning "int", or should it be "void",
because it always warns when there is an issue, the caller does not
get to choose to give its own warning?

> +	if (errno != ENOENT && errno != ENOTDIR) {
> +		warn_on_inaccessible(path);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  int xmkstemp(char *template)
>  {
>  	int fd;