Re: [PATCH 00/15] Handle fopen() errors
Jeff Kingwrites: > 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
On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote: > On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamanowrote: > > 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
On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamanowrote: > 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
On Fri, Apr 21, 2017 at 8:04 PM, Duy Nguyenwrote: > 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
On Fri, Apr 21, 2017 at 1:29 PM, Jeff Kingwrote: > 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
On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote: > Junio C Hamanowrites: > > > 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 Hamanowrites: > 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
Nguyễn Thái Ngọc Duywrites: > 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?
[PATCH 00/15] Handle fopen() errors
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. There are still a few iffy fopen() calls in sequencer.c though. I only fixed the easy ones in there. The last patch may fail on some platforms since I want to make sure that fopen() == NULL is an expected behavior, even though I could only test FreeBSD and Linux (and know Windows behaves the same). At least when people shout up, we could start adding FREAD_READS_DIRECTORIES on those platforms. That's the goal. Nguyễn Thái Ngọc Duy (15): config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD bisect: report on fopen() error blame: report error on open if graft_file is a directory clone: use xfopen() instead of fopen() log: report errno on failure to fopen() a file commit.c: report error on failure to fopen() the graft file remote.c: report error on failure to fopen() rerere.c: report error on failure to fopen() rerere.c: report correct errno sequencer.c: report error on failure to fopen() server-info: report error on failure to fopen() wt-status.c: report error on failure to fopen() xdiff-interface.c: report errno on failure to stat() or fopen() config.c: handle error on failing to fopen() t1308: add a test case on open a config directory bisect.c | 5 - builtin/blame.c | 5 - builtin/clone.c | 2 +- builtin/log.c | 3 ++- commit.c | 5 - config.c | 8 +++- config.mak.uname | 3 +++ remote.c | 12 ++-- rerere.c | 10 +++--- sequencer.c | 5 - server-info.c | 5 - t/t1308-config-set.sh | 13 - wt-status.c | 2 ++ xdiff-interface.c | 4 ++-- 14 files changed, 66 insertions(+), 16 deletions(-) -- 2.11.0.157.gd943d85