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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  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, ) < 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, ) < 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;

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

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy 

This commit message is a bit empty. How about:

In many places, Git warns about an inaccessible file after a
fopen() failed. To discern these cases from other cases where we
want to warn about inaccessible files, introduce a new helper
specifically to test whether fopen() failed because the current
user lacks the permission to open file in question.

> 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, ) < 0) {
> - if (errno != ENOENT)
> - warn_on_inaccessible(fname);
> + warn_on_fopen_errors(fname);
>   if (0 <= fd)
>   close(fd);

I see you already converted one location. Why not simply fold all of them
into this patch? It's not like it would be easier to review those changes
if you separate patches addressing this class of problems.

Ciao,
Dscho

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 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, ) < 0) {
-   if (errno != ENOENT)
-   warn_on_inaccessible(fname);
+   warn_on_fopen_errors(fname);
if (0 <= fd)
close(fd);
if (!check_index ||
diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564a69..c5b59c23e8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1099,6 +1099,8 @@ 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);
+/* Warn on an inaccessible file if errno indicates this is an error */
+int warn_on_fopen_errors(const char *path);
 
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
diff --git a/wrapper.c b/wrapper.c
index d837417709..20c25e7e65 100644
--- 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)
+{
+   if (errno != ENOENT && errno != ENOTDIR) {
+   warn_on_inaccessible(path);
+   return -1;
+   }
+
+   return 0;
+}
+
 int xmkstemp(char *template)
 {
int fd;
-- 
2.11.0.157.gd943d85