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

[Junio C Hamano]

Jeff King  writes:

> On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:
>
>> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano  wrote:
>> > Yes, but (1) we'd need to be careful about --quiet
>> 
>> Yeah. It's a real pain point for making changes like this. At some
>> point we should just have a global (maybe multi-level) quiet flag.
>
> I don't think it's too bad here. Isn't it just:
>
>   FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);
>
> It is a little annoying to have to repeat "x", though (which is
> sometimes a git_path() invocation).

Sure, but you could do

fopen_or_warn(quiet, x, y)

if it is a problem ;-)

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

[Jeff King]

On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:

> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano  wrote:
> > Yes, but (1) we'd need to be careful about --quiet
> 
> Yeah. It's a real pain point for making changes like this. At some
> point we should just have a global (maybe multi-level) quiet flag.

I don't think it's too bad here. Isn't it just:

  FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);

It is a little annoying to have to repeat "x", though (which is
sometimes a git_path() invocation).

-Peff

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

[Duy Nguyen]

On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano  wrote:
> Yes, but (1) we'd need to be careful about --quiet

Yeah. It's a real pain point for making changes like this. At some
point we should just have a global (maybe multi-level) quiet flag.
-- 
Duy

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

[Junio C Hamano]

On Fri, Apr 21, 2017 at 8:04 PM, Duy Nguyen  wrote:
> On Fri, Apr 21, 2017 at 1:29 PM, Jeff King  wrote:
>>
>> I had a similar thought while reading through it. I think it would be
>> shorter still with:
>>
>>   FILE *fopen_or_warn(const char *path, const char *mode)
>>   {
>> FILE *fh = fopen(path, mode);
>> if (!fh)
>> warn_failure_to_read_open_optional_path(path);
>> return fh;
>>   }
>>
>> And then quite a few of the patches could just be
>> s/fopen/fopen_or_warn/.
>
> Jeff.. oh Jeff.. you have made it _way_ too convenient that after a
> quick grep at fopen( again, I found a couple more places that I would
> have just ignored last time (too much work), but now all I need to do
> is Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)

Yes, but (1) we'd need to be careful about --quiet, and (2) we would also need
a wrapper for open(2).

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

[Duy Nguyen]

On Fri, Apr 21, 2017 at 1:29 PM, Jeff King  wrote:
> On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano  writes:
>>
>> > 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.
>
> I had a similar thought while reading through it. I think it would be
> shorter still with:
>
>   FILE *fopen_or_warn(const char *path, const char *mode)
>   {
> FILE *fh = fopen(path, mode);
> if (!fh)
> warn_failure_to_read_open_optional_path(path);
> return fh;
>   }
>
> And then quite a few of the patches could just be
> s/fopen/fopen_or_warn/.

Jeff.. oh Jeff.. you have made it _way_ too convenient that after a
quick grep at fopen( again, I found a couple more places that I would
have just ignored last time (too much work), but now all I need to do
is Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)
-- 
Duy

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

[Jeff King]

On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > 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.

I had a similar thought while reading through it. I think it would be
shorter still with:

  FILE *fopen_or_warn(const char *path, const char *mode)
  {
FILE *fh = fopen(path, mode);
if (!fh)
warn_failure_to_read_open_optional_path(path);
return fh;
  }

And then quite a few of the patches could just be
s/fopen/fopen_or_warn/.

-Peff

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

[Junio C Hamano]

Junio C Hamano  writes:

> 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);

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

[Junio C Hamano]

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

> Some of you may recall a while back, nd/conditional-config-include
> failed on Windows because I accidentally fopen()'d a directory in a
> test, but it's not considered an serious error unless it's on Windows,
> where fopen() returns NULL.
>
> A couple of suggestions were thrown back and forth, but I was a bit
> busy to follow up. Now that I have time, I have audited all fopen()
> calls and try to fix them up for good. There 15 patches, but they only
> change one or two lines each. I split them anyway so you can pause
> between patches and see if it really makes sense, as changes are all
> over the places.

Nicely done.  

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?