Handle fopen() errors
cover 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(<dir>) 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(<directory>) == 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(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH 1/15 config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
This variable is added [1] with the assumption that on a sane system, fopen(<dir>, "r") should return NULL. Linux and FreeBSD do not meet this expectation while at least Windows and AIX do. Let's make sure they behave the same way.
I only tested one version on Linux (4.7.0 with glibc 2.22) and FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace, I'm pretty sure it shares the same problem.
[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open
directory - 2008-02-08)
--- config.mak.uname | 3 +++ 1 file changed, 3 insertions(+)
config.mak.uname +3 -0
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux) NEEDS_LIBRT = YesPlease HAVE_GETDELIM = YesPlease SANE_TEXT_GREP=-a + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease
@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD) HAVE_PATHS_H = YesPlease DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease LIBC_CONTAINS_LIBINTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),UnixWare) CC = cc
@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD) GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes HAVE_BSD_SYSCTL = YesPlease PAGER_ENV = LESS=FRX LV=-c MORE=FRX + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease
Signature · 1 line
2.11.0.157.gd943d85
PATCH 2/15 bisect: report on fopen() error
In reply to Nguyễn Thái Ngọc DuyThe main thing to catch here is when fopen() is called on a directory. It's safe even without this change because a few lines earlier we do check if "filename" is a regular file.
Regardless, let's stay on the safe side in case somebody changes those lines. Unconditionally printing to stderr by warn_on_inaccessible() should be fine, because the caller does unconditional fprintf(stderr, too, no checking for quiet option or anything.
--- bisect.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
bisect.c +4 -1
@@ -669,8 +669,11 @@ static int is_expected_rev(const struct object_id *oid)return 0 ; fp =fopen (filename,"r" ); -if (!fp) +if (!fp) { +if (errno !=ENOENT ) +warn_on_inaccessible (filename);return 0 ; + }if (strbuf_getline_lf (&str, fp) !=EOF ) res = !strcmp (str.buf,oid_to_hex (oid));
Signature · 1 line
2.11.0.157.gd943d85
PATCH 3/15 blame: report error on open if graft_file is a directory
In reply to Nguyễn Thái Ngọc Duy--- builtin/blame.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
builtin/blame.c +4 -1
@@ -2073,8 +2073,11 @@ static int read_ancestry(const char *graft_file) {FILE *fp =fopen (graft_file,"r" );struct strbuf buf =STRBUF_INIT ; -if (!fp) +if (!fp) { +if (errno !=ENOENT ) +warn_on_inaccessible (graft_file);return -1 ; + }while (!strbuf_getwholeline (&buf, fp,'\n' )) {/* The format is just "Commit Parent1 Parent2 ...\n" */ struct commit_graft *graft =read_graft_line (buf.buf, buf.len);
Signature · 1 line
2.11.0.157.gd943d85
PATCH 4/15 clone: use xfopen() instead of fopen()
In reply to Nguyễn Thái Ngọc DuyThis code uses the result FILE pointer right away without the NULL check. Let's use xfopen() and die if we could not open the file. That's still better than crashing,
--- builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/clone.c +1 -1
@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,* to turn entries with paths relative to the original * absolute, so that they can be used in the new repository. */ -FILE *in =fopen (src->buf,"r" ); +FILE *in =xfopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ;while (strbuf_getline (&line, in) !=EOF ) {
Signature · 1 line
2.11.0.157.gd943d85
PATCH 5/15 log: report errno on failure to fopen() a file
In reply to Nguyễn Thái Ngọc Duy--- builtin/log.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/log.c +2 -1
@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,printf ("%s\n" , filename.buf + outdir_offset);if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) -return error (_ ("Cannot open patch file %s" ), filename.buf); +return error_errno (_ ("Cannot open patch file %s" ), + filename.buf);strbuf_release (&filename);return 0 ;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH 05/15] log: report errno on failure to fopen() a file
Quoted 10 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- builtin/log.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)builtin/log.c +0
-0
builtin/log.c static int open_next_file(struct commit *commit, const char *subject, · line 865
Quoted 9 lines
builtin/log.c +2
-1@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,printf ("%s\n" , filename.buf + outdir_offset);if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) -return error (_ ("Cannot open patch file %s" ), filename.buf); +return error_errno (_ ("Cannot open patch file %s" ), + filename.buf);strbuf_release (&filename);return 0 ;
Not a new problem with your patch, but just looking at the context it seems clear that "filename" is leaked in the error case.
-Peff
PATCH 6/15 commit.c: report error on failure to fopen() the graft file
In reply to Nguyễn Thái Ngọc DuySuppressing the error (because the command requires --quiet) is not a concern because we already call error() just a couple lines down.
--- commit.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
commit.c +4 -1
@@ -169,8 +169,11 @@ static int read_graft_file(const char *graft_file) {FILE *fp =fopen (graft_file,"r" );struct strbuf buf =STRBUF_INIT ; -if (!fp) +if (!fp) { +if (errno !=ENOENT ) +warn_on_inaccessible (graft_file);return -1 ; + }while (!strbuf_getwholeline (&buf, fp,'\n' )) {/* The format is just "Commit Parent1 Parent2 ...\n" */ struct commit_graft *graft =read_graft_line (buf.buf, buf.len);
Signature · 1 line
2.11.0.157.gd943d85
PATCH 7/15 remote.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc DuyThere's plenty of error() in this code to safely assume --quiet is not a concern.
--- remote.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
remote.c +10 -2
@@ -253,8 +253,12 @@ static void read_remotes_file(struct remote *remote)struct strbuf buf =STRBUF_INIT ;FILE *f =fopen (git_path ("remotes/%s" , remote->name),"r" ); -if (!f) +if (!f) { +if (errno !=ENOENT ) +warn_on_inaccessible (git_path ("remotes/%s" , + remote->name));return ; + } remote->configured_in_repo =1 ; remote->origin =REMOTE_REMOTES ;while (strbuf_getline (&buf, f) !=EOF ) {
@@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote)struct strbuf buf =STRBUF_INIT ;FILE *f =fopen (git_path ("branches/%s" , remote->name),"r" ); -if (!f) +if (!f) { +if (errno !=ENOENT ) +warn_on_inaccessible (git_path ("branches/%s" , + remote->name));return ; + }strbuf_getline_lf (&buf, f);fclose (f);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH 07/15] remote.c: report error on failure to fopen()
Am 20.04.2017 um 13:26 schrieb Nguyễn Thái Ngọc Duy:
Quoted 4 lines
(quoted diff) +0
-0@@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote) struct strbuf buf = STRBUF_INIT; FILE *f = fopen(git_path("branches/%s", remote->name), "r");
Review static void read_branches_file(struct remote *remote) · line 294
Quoted 10 lines
(quoted diff) +5
-1- if (!f) + if (!f) { + if (errno != ENOENT) + warn_on_inaccessible(git_path("branches/%s", + remote->name)); return; + } strbuf_getline_lf(&buf, f); fclose(f);
This hunk causes a new failure in t5512.10 'confuses pattern as remote when no remote specified' on Windows:
+++ diff -u exp actual
actual Wed Apr 26 08:16:10 2017 renamed from exp Wed Apr 26 08:16:10 2017 +5 -0
@@ -1,5 +1,19 @@ +++ case "$1" in +++ _test_ok= +++ git ls-remote 'refs*master' +warning: unable to access '.git/branches/refs*master': Invalid argument fatal: 'refs*master' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. +++ exit_code=128
On Windows, it is not allowed to pass a file name with an asterisk to the OS. The test case contains this comment:
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.
So, can we replace the name with a simple "master", our would this miss the goal of the test case? Should we make it conditional on the MINGW prerequisite?
-- Hannes
Re: [PATCH 07/15] remote.c: report error on failure to fopen()
Quoted 12 lines · Johannes Sixt
+++ git ls-remote 'refs*master'
+warning: unable to access '.git/branches/refs*master': Invalid argument
fatal: 'refs*master' does not appear to be a git repository
fatal: Could not read from remote repository.Please make sure you have the correct access rights and the repository exists.
+++ exit_code=128On Windows, it is not allowed to pass a file name with an asterisk to the OS. The test case contains this comment:
Quoted 6 lines
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.So, can we replace the name with a simple "master", our would this miss the goal of the test case? Should we make it conditional on the MINGW prerequisite?
I would actually be more worried about the real-life impact of this change. Those who did "git ls-remote 'refs*master'" merely got "it does not appear to be a git repository" and that was entirely sensible response from the command. Can Windows folks live with an extra warning before it, or do they object to see that new warning?
Re: [PATCH 07/15] remote.c: report error on failure to fopen()
Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
Quoted 17 lines
Quoted 15 lines · Johannes Sixt
+++ git ls-remote 'refs*master'
+warning: unable to access '.git/branches/refs*master': Invalid argument
fatal: 'refs*master' does not appear to be a git repository
fatal: Could not read from remote repository.Please make sure you have the correct access rights and the repository exists.
+++ exit_code=128On Windows, it is not allowed to pass a file name with an asterisk to the OS. The test case contains this comment:
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.
Quoted 9 lines
Quoted 3 lines
So, can we replace the name with a simple "master", our would this miss the goal of the test case? Should we make it conditional on the MINGW prerequisite?
I would actually be more worried about the real-life impact of this change. Those who did "git ls-remote 'refs*master'" merely got "it does not appear to be a git repository" and that was entirely sensible response from the command. Can Windows folks live with an extra warning before it, or do they object to see that new warning?
I was also worried that the new warning may be irritating. However, I expect that it is seen in practice only after a typo. My gut feeling is that this is bearable, because the reason for the warning should be obvious.
Unless a use-case turns up where the pattern occurs routinely. We'll have to keep the eyes open. Until then it is better to keep the change, IMO.
-- Hannes
Re: [PATCH 07/15] remote.c: report error on failure to fopen()
Quoted 31 lines · Johannes Sixt
Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
Quoted 18 lines
Quoted 15 lines · Johannes Sixt
+++ git ls-remote 'refs*master'
+warning: unable to access '.git/branches/refs*master': Invalid argument
fatal: 'refs*master' does not appear to be a git repository
fatal: Could not read from remote repository.Please make sure you have the correct access rights and the repository exists.
+++ exit_code=128On Windows, it is not allowed to pass a file name with an asterisk to the OS. The test case contains this comment:
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.Quoted 10 lines
Quoted 3 lines
So, can we replace the name with a simple "master", our would this miss the goal of the test case? Should we make it conditional on the MINGW prerequisite?
I would actually be more worried about the real-life impact of this change. Those who did "git ls-remote 'refs*master'" merely got "it does not appear to be a git repository" and that was entirely sensible response from the command. Can Windows folks live with an extra warning before it, or do they object to see that new warning?
Quoted 6 lines
I was also worried that the new warning may be irritating. However, I expect that it is seen in practice only after a typo. My gut feeling is that this is bearable, because the reason for the warning should be obvious.
Unless a use-case turns up where the pattern occurs routinely. We'll have to keep the eyes open. Until then it is better to keep the change, IMO.
OK I'll just add MINGW to the test then. Windows folks can improve warn_on_inaccessible() to suppress certain class of error messages if needed.
Signature · 1 line
Duy
Re: [PATCH 07/15] remote.c: report error on failure to fopen()
Am 27.04.2017 um 11:14 schrieb Duy Nguyen:
Quoted 20 lines · Johannes Sixt
Quoted 19 lines
Am 27.04.2017 um 02:57 schrieb Junio C Hamano:
Quoted 18 lines
Quoted 15 lines · Johannes Sixt
+++ git ls-remote 'refs*master'
+warning: unable to access '.git/branches/refs*master': Invalid argument
fatal: 'refs*master' does not appear to be a git repository
fatal: Could not read from remote repository.Please make sure you have the correct access rights and the repository exists.
+++ exit_code=128On Windows, it is not allowed to pass a file name with an asterisk to the OS. The test case contains this comment:
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So, can we replace the name with a simple "master", our would this miss the goal of the test case? Should we make it conditional on the MINGW prerequisite?
OK I'll just add MINGW to the test then. Windows folks can improve
warn_on_inaccessible()to suppress certain class of error messages if needed.
I actually meant something like this:
if test_have_prereq MINGW then does_not_exist=master else does_not_exist="refs*master" fi test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
-- Hannes
PATCH 8/15 rerere.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- rerere.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
rerere.c +4 -1
@@ -202,8 +202,11 @@ static void read_rr(struct string_list *rr)struct strbuf buf =STRBUF_INIT ;FILE *in =fopen (git_path_merge_rr (),"r" ); -if (!in) +if (!in) { +if (errno !=ENOENT ) +warn_on_inaccessible (git_path_merge_rr ());return ; + }while (!strbuf_getwholeline (&buf, in,'\0' )) {char *path;unsigned char sha1[20 ];
Signature · 1 line
2.11.0.157.gd943d85
PATCH 9/15 rerere.c: report correct errno
In reply to Nguyễn Thái Ngọc DuyIn the first case, we should have reported the reason fopen() fails. In the second case, fclose() might change errno but we want to report fopen() failure.
--- rerere.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
rerere.c +3 -2
@@ -487,13 +487,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output io.input =fopen (path,"r" ); io.io.wrerror =0 ;if (!io.input) -return error ("Could not open %s" , path); +return error_errno ("Could not open %s" , path);if (output) { io.io.output =fopen (output,"w" );if (!io.io.output) { +error_errno ("Could not write %s" , output);fclose (io.input); -return error ("Could not write %s" , output); +return -1 ; } }
Signature · 1 line
2.11.0.157.gd943d85
PATCH 10/15 sequencer.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- sequencer.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
sequencer.c +4 -1
@@ -919,8 +919,11 @@ static void record_in_rewritten(struct object_id *oid, enum todo_command next_command) {FILE *out =fopen (rebase_path_rewritten_pending (),"a" ); -if (!out) +if (!out) { +if (errno !=ENOENT ) +warn_on_inaccessible (rebase_path_rewritten_pending ());return ; + }fprintf (out,"%s\n" ,oid_to_hex (oid));fclose (out);
Signature · 1 line
2.11.0.157.gd943d85
PATCH 11/15 server-info: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- server-info.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
server-info.c +4 -1
@@ -132,8 +132,11 @@ static int read_pack_info_file(const char *infofile)int old_cnt =0 ; fp =fopen (infofile,"r" ); -if (!fp) +if (!fp) { +if (errno !=ENOENT ) +warn_on_inaccessible (infofile);return 1 ;/* nonexistent is not an error. */ + }while (fgets (line,sizeof (line), fp)) {int len =strlen (line);
Signature · 1 line
2.11.0.157.gd943d85
PATCH 12/15 wt-status.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 2 ++ 1 file changed, 2 insertions(+)
wt-status.c +2 -0
@@ -1067,6 +1067,8 @@ static char *read_line_from_git_path(const char *filename)struct strbuf buf =STRBUF_INIT ;FILE *fp =fopen (git_path ("%s" , filename),"r" );if (!fp) { +if (errno !=ENOENT ) +warn_on_inaccessible (git_path ("%s" , filename));strbuf_release (&buf);return NULL ; }
Signature · 1 line
2.11.0.157.gd943d85
PATCH 13/15 xdiff-interface.c: report errno on failure to stat() or fopen()
In reply to Nguyễn Thái Ngọc Duy--- xdiff-interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
xdiff-interface.c +2 -2
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)size_t sz;if (stat (filename, &st)) -return error ("Could not stat %s" , filename); +return error_errno ("Could not stat %s" , filename);if ((f =fopen (filename,"rb" )) ==NULL ) -return error ("Could not open %s" , filename); +return error_errno ("Could not open %s" , filename); sz =xsize_t (st.st_size); ptr->ptr =xmalloc (sz ? sz :1 );if (sz &&fread (ptr->ptr, sz,1 , f) !=1 ) {
Signature · 1 line
2.11.0.157.gd943d85
PATCH 14/15 config.c: handle error on failing to fopen()
In reply to Nguyễn Thái Ngọc DuyIn the first case, we already correctly return -1 if fopen() fails to open. But we should report something so people know what's wrong.
In the second case, config_file == NULL does not necessarily mean "no config file". Bail out if needed.
--- config.c | 8 +++++++- t/t1308-config-set.sh | 4 +++- 2 files changed, 10 insertions(+), 2 deletions(-)
config.c +7 -1
@@ -1401,7 +1401,8 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) ret =do_config_from_file (fn,CONFIG_ORIGIN_FILE , filename, filename, f, data);funlockfile (f);fclose (f); - } + }else if (errno !=ENOENT ) +warn_on_inaccessible (filename);return ret; }
@@ -2601,6 +2602,11 @@ int git_config_rename_section_in_file(const char *config_filename, }if (!(config_file =fopen (config_filename,"rb" ))) { +if (errno !=ENOENT ) { +warn_on_inaccessible (config_filename); + ret =-1 ; + goto out; + }/* no config file means nothing to rename, no error */ goto commit_and_out; }
t/t1308-config-set.sh +3 -1
@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" && echo "Error (-1) reading configuration file .git/config." >expect && - test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual && + test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output && + grep "^warning:" output && + grep "^Error" output >actual && test_cmp expect actual '
Signature · 1 line
2.11.0.157.gd943d85
PATCH 15/15 t1308: add a test case on open a config directory
In reply to Nguyễn Thái Ngọc DuyWe don't support config-as-a-directory (maybe someday we will?). Make sure we consistently fail in this case, which should happen on platforms where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is defined.
--- t/t1308-config-set.sh | 9 +++++++++ 1 file changed, 9 insertions(+)
t/t1308-config-set.sh +9 -0
@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' ' test_cmp expect actual ' +test_expect_success 'proper error on directory "files"' ' + echo "Error (-1) reading configuration file a-directory." >expect && + mkdir a-directory && + test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output && + grep "^warning:" output && + grep "^Error" output >actual && + test_cmp expect actual +' + test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" &&
Signature · 1 line
2.11.0.157.gd943d85
Quoted 11 lines · Nguyễn Thái Ngọc Duy
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(<dir>) 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?
Quoted 6 lines · Junio C Hamano
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(-)
attr.c +1 -2
@@ -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));
git-compat-util.h +3 -0
@@ -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 *);
wrapper.c +6 -0
@@ -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);
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
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?
Quoted 5 lines
"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
Quoted 28 lines · Jeff King
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
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?
Quoted 5 lines
"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; }
Quoted 2 lines
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.. :)
Signature · 1 line
Duy
Quoted 13 lines · Duy Nguyen
Quoted 12 lines · Jeff King
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; }
Quoted 7 lines
Quoted 2 lines
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).
Quoted 1 line · Junio C Hamano
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.
Signature · 1 line
Duy
Quoted 5 lines · Duy Nguyen
Quoted 1 line · Junio C Hamano
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
Quoted 8 lines · Jeff King
Quoted 5 lines · Duy Nguyen
Quoted 1 line · Junio C Hamano
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.
Quoted 6 lines
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 ;-)
v2 cover [PATCH v2 00/21]
In reply to Nguyễn Thái Ngọc DuyChanges since v1:
- fopen_or_warn() and warn_on_fopen_errors() are introduced. The latter's name is probably not great...
- A new patch (first one) to convert a bunch to using xfopen()
- Fix test failure on Windows, found by Johannes Sixt
- Fix the memory leak in log.c, found by Jeff
There are still a couple of fopen() remained, but I'm getting slow again so let's get these in first and worry about the rest when somebody has time.
Nguyễn Thái Ngọc Duy (21):
Use xfopen() in more places
clone: use xfopen() instead of fopen()
config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD wrapper.c: add warn_on_fopen_errors()
wrapper.c: add fopen_or_warn()
attr.c: use fopen_or_warn()
ident.c: use fopen_or_warn()
bisect: report on fopen() error
blame: report error on open if graft_file is a directory
log: report errno on failure to fopen() a file
log: fix memory leak in open_next_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
attr.c | 7 ++----- bisect.c | 7 ++----- builtin/am.c | 8 ++------ builtin/blame.c | 4 +++- builtin/clone.c | 2 +- builtin/commit.c | 5 +---- builtin/fast-export.c | 4 +--- builtin/fsck.c | 3 +-- builtin/log.c | 11 ++++++++--- builtin/merge.c | 4 +--- builtin/pull.c | 3 +-- commit.c | 4 +++- config.c | 5 ++++- config.mak.uname | 3 +++ diff.c | 8 ++------ dir.c | 3 +-- fast-import.c | 4 +--- git-compat-util.h | 3 +++ ident.c | 8 +++----- remote-testsvn.c | 8 ++------ remote.c | 4 ++-- rerere.c | 7 ++++--- sequencer.c | 8 ++++---- server-info.c | 2 +- t/t1308-config-set.sh | 13 ++++++++++++- t/t5512-ls-remote.sh | 13 ++++++++++--- wrapper.c | 21 +++++++++++++++++++++ wt-status.c | 3 ++- xdiff-interface.c | 4 ++-- 29 files changed, 103 insertions(+), 76 deletions(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 1/21 Use xfopen() in more places
xfopen()
- provides error details
- explains error on reading, or writing, or whatever operation
- has l10n support
- prints file name in the error
Some of these are missing in the places that are replaced with xfopen(), which is a clear win. In some other places, it's just less code (not as clearly a win as the previous case but still is).
The only slight regresssion is in remote-testsvn, where we don't report the file class (marks files) in the error messages anymore. But since this is a _test_ svn remote transport, I'm not too concerned.
--- bisect.c | 5 +---- builtin/am.c | 8 ++------ builtin/commit.c | 5 +---- builtin/fast-export.c | 4 +--- builtin/fsck.c | 3 +-- builtin/merge.c | 4 +--- builtin/pull.c | 3 +-- diff.c | 8 ++------ fast-import.c | 4 +--- remote-testsvn.c | 8 ++------ 10 files changed, 13 insertions(+), 39 deletions(-)
bisect.c +1 -4
@@ -438,10 +438,7 @@ static void read_bisect_paths(struct argv_array *array) {struct strbuf str =STRBUF_INIT ;const char *filename =git_path_bisect_names (); -FILE *fp =fopen (filename,"r" ); - -if (!fp) -die_errno (_ ("Could not open file '%s'" ), filename); +FILE *fp =xfopen (filename,"r" );while (strbuf_getline_lf (&str, fp) !=EOF ) {strbuf_trim (&str);
builtin/am.c +2 -6
@@ -1275,12 +1275,8 @@ static int parse_mail(struct am_state *state, const char *mail)die ("BUG: invalid value for state->scissors" ); } - mi.input =fopen (mail,"r" ); -if (!mi.input) -die ("could not open input" ); - mi.output =fopen (am_path (state,"info" ),"w" ); -if (!mi.output) -die ("could not open output 'info'" ); + mi.input =xfopen (mail,"r" ); + mi.output =xfopen (am_path (state,"info" ),"w" );if (mailinfo (&mi,am_path (state,"msg" ),am_path (state,"patch" )))die ("could not parse patch" );
builtin/commit.c +1 -4
@@ -1695,10 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)if (!reflog_msg) reflog_msg ="commit (merge)" ; pptr =commit_list_append (current_head, pptr); - fp =fopen (git_path_merge_head (),"r" ); -if (fp ==NULL ) -die_errno (_ ("could not open '%s' for reading" ), -git_path_merge_head ()); + fp =xfopen (git_path_merge_head (),"r" );while (strbuf_getline_lf (&m, fp) !=EOF ) {struct commit *parent;
builtin/fast-export.c +1 -3
@@ -905,9 +905,7 @@ static void export_marks(char *file)static void import_marks (char *input_file) {char line[512 ]; -FILE *f =fopen (input_file,"r" ); -if (!f) -die_errno ("cannot read '%s'" , input_file); +FILE *f =xfopen (input_file,"r" );while (fgets (line,sizeof (line), f)) {uint32_t mark;
builtin/fsck.c +1 -2
@@ -280,8 +280,7 @@ static void check_unreachable_object(struct object *obj)free (filename);return ; } -if (!(f =fopen (filename,"w" ))) -die_errno ("Could not open '%s'" , filename); + f =xfopen (filename,"w" );if (obj->type ==OBJ_BLOB ) {if (stream_blob_to_fd (fileno (f), &obj->oid,NULL ,1 ))die_errno ("Could not write '%s'" , filename);
builtin/merge.c +1 -3
@@ -839,9 +839,7 @@ static int suggest_conflicts(void)struct strbuf msgbuf =STRBUF_INIT ; filename =git_path_merge_msg (); - fp =fopen (filename,"a" ); -if (!fp) -die_errno (_ ("Could not open '%s' for writing" ), filename); + fp =xfopen (filename,"a" );append_conflicts_hint (&msgbuf);fputs (msgbuf.buf, fp);
builtin/pull.c +1 -2
@@ -337,8 +337,7 @@ static void get_merge_heads(struct oid_array *merge_heads)struct strbuf sb =STRBUF_INIT ;struct object_id oid; -if (!(fp =fopen (filename,"r" ))) -die_errno (_ ("could not open '%s' for reading" ), filename); + fp =xfopen (filename,"r" );while (strbuf_getline_lf (&sb, fp) !=EOF ) {if (get_oid_hex (sb.buf, &oid))continue ;/* invalid line: does not start with SHA1 */
diff.c +2 -6
@@ -4071,9 +4071,7 @@ int diff_opt_parse(struct diff_options *options,DIFF_OPT_CLR (options,FUNCCONTEXT ); else if ((argcount = parse_long_opt("output", av, &optarg))) { char *path = prefix_filename(prefix, optarg); - options->file =fopen (path,"w" ); -if (!options->file) -die_errno ("Could not open '%s'" , path); + options->file =xfopen (path,"w" ); options->close_file =1 ;if (options->use_color !=GIT_COLOR_ALWAYS ) options->use_color =GIT_COLOR_NEVER ;
@@ -4807,9 +4805,7 @@ void diff_flush(struct diff_options *options)*/ if (options->close_file)fclose (options->file); - options->file =fopen ("/dev/null" ,"w" ); -if (!options->file) -die_errno ("Could not open /dev/null" ); + options->file =xfopen ("/dev/null" ,"w" ); options->close_file =1 ;for (i =0 ; i < q->nr; i++) {struct diff_filepair *p = q->queue[i];
fast-import.c +1 -3
@@ -3274,9 +3274,7 @@ static void option_export_pack_edges(const char *edges) {if (pack_edges)fclose (pack_edges); - pack_edges =fopen (edges,"a" ); -if (!pack_edges) -die_errno ("Cannot open '%s'" , edges); + pack_edges =xfopen (edges,"a" ); }static int parse_one_option (const char *option)
remote-testsvn.c +2 -6
@@ -124,10 +124,8 @@ static int note2mark_cb(const unsigned char *object_sha1,static void regenerate_marks (void ) {int ret; -FILE *marksfile =fopen (marksfilename,"w+" ); +FILE *marksfile =xfopen (marksfilename,"w+" ); -if (!marksfile) -die_errno ("Couldn't create mark file %s." , marksfilename); ret =for_each_note (NULL ,0 , note2mark_cb, marksfile);if (ret)die ("Regeneration of marks failed, returned %d." , ret);
@@ -148,9 +146,7 @@ static void check_or_regenerate_marks(int latestrev) marksfile =fopen (marksfilename,"r" );if (!marksfile) {regenerate_marks (); - marksfile =fopen (marksfilename,"r" ); -if (!marksfile) -die_errno ("cannot read marks file %s!" , marksfilename); + marksfile =xfopen (marksfilename,"r" );fclose (marksfile); }else {strbuf_addf (&sb,":%d " , latestrev);
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 2/21 clone: use xfopen() instead of fopen()
In reply to Nguyễn Thái Ngọc DuyThis code uses the result FILE pointer right away without the NULL check. Let's use xfopen() and die if we could not open the file. That's still better than crashing,
--- builtin/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/clone.c +1 -1
@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,* to turn entries with paths relative to the original * absolute, so that they can be used in the new repository. */ -FILE *in =fopen (src->buf,"r" ); +FILE *in =xfopen (src->buf,"r" );struct strbuf line =STRBUF_INIT ;while (strbuf_getline (&line, in) !=EOF ) {
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 02/21] clone: use xfopen() instead of fopen()
Hi Duy,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
This code uses the result FILE pointer right away without the NULL check. Let's use
xfopen()and die if we could not open the file. That's still better than crashing,Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
While funny to read, maybe it would be better to spell it out clearly:
copy_alternates() called fopen() without handling errors. By switching to xfopen(), this bug is fixed.
Ciao,
Dscho
PATCH v2 3/21 config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
In reply to Nguyễn Thái Ngọc DuyThis variable is added [1] with the assumption that on a sane system, fopen(<dir>, "r") should return NULL. Linux and FreeBSD do not meet this expectation while at least Windows and AIX do. Let's make sure they behave the same way.
I only tested one version on Linux (4.7.0 with glibc 2.22) and FreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace, I'm pretty sure it shares the same problem.
[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open
directory - 2008-02-08)
--- config.mak.uname | 3 +++ 1 file changed, 3 insertions(+)
config.mak.uname +3 -0
@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux) NEEDS_LIBRT = YesPlease HAVE_GETDELIM = YesPlease SANE_TEXT_GREP=-a + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),GNU/kFreeBSD) HAVE_ALLOCA_H = YesPlease
@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD) HAVE_PATHS_H = YesPlease DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease LIBC_CONTAINS_LIBINTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),UnixWare) CC = cc
@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD) GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes HAVE_BSD_SYSCTL = YesPlease PAGER_ENV = LESS=FRX LV=-c MORE=FRX + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 4/21 wrapper.c: add warn_on_fopen_errors()
In reply to 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(-)
dir.c +1 -2
@@ -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);if (0 <= fd)close (fd); if (!check_index ||
git-compat-util.h +2 -0
@@ -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 *);
wrapper.c +10 -0
@@ -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;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
Hi Duy,
Quoted 1 line · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
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.
dir.c static int add_excludes(const char *fname, const char *base, int baselen, · line 750
Quoted 13 lines
dir.c +1
-2@@ -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);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
Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()
In reply to Nguyễn Thái Ngọc DuyQuoted 12 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- dir.c | 3 +-- git-compat-util.h | 2 ++ wrapper.c | 10 ++++++++++ 3 files changed, 13 insertions(+), 2 deletions(-)dir.c +0
-0
dir.c static int add_excludes(const char *fname, const char *base, int baselen, · line 748
Quoted 6 lines
dir.c +1
-2@@ -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?
wrapper.c FILE *fopen_for_writing(const char *path) · line 422
Quoted 8 lines
wrapper.c +2
-0@@ -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?
Quoted 11 lines
wrapper.c +8
-0+if (errno !=ENOENT && errno !=ENOTDIR ) { +warn_on_inaccessible (path); +return -1 ; + } + +return 0 ; +} +int xmkstemp (char *template) {int fd;
PATCH v2 5/21 wrapper.c: add fopen_or_warn()
In reply to Nguyễn Thái Ngọc DuyWhen fopen() returns NULL, it could be because the given path does not exist, but it could also be some other errors and the caller has to check. Add a wrapper so we don't have to repeat the same error check everywhere.
--- git-compat-util.h | 1 + wrapper.c | 11 +++++++++++ 2 files changed, 12 insertions(+)
git-compat-util.h +1 -0
@@ -800,6 +800,7 @@ extern int xmkstemp(char *template);extern int xmkstemp_mode (char *template,int mode);extern char *xgetcwd (void );extern FILE *fopen_for_writing (const char *path); +extern FILE *fopen_or_warn (const char *path,const char *mode);#define ALLOC_ARRAY (x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))#define REALLOC_ARRAY (x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
wrapper.c +11 -0
@@ -428,6 +428,17 @@ int warn_on_fopen_errors(const char *path)return 0 ; } +FILE *fopen_or_warn (const char *path,const char *mode) +{ +FILE *fp =fopen (path, mode); + +if (fp) +return fp; + +warn_on_fopen_errors (path); +return NULL ; +} +int xmkstemp (char *template) {int fd;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 6/21 attr.c: use fopen_or_warn()
In reply to Nguyễn Thái Ngọc Duy--- attr.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
attr.c +2 -5
@@ -720,16 +720,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction,static struct attr_stack *read_attr_from_file (const char *path,int macro_ok) { -FILE *fp =fopen (path,"r" ); +FILE *fp =fopen_or_warn (path,"r" );struct attr_stack *res;char buf[2048 ];int lineno =0 ; -if (!fp) { -if (errno !=ENOENT && errno !=ENOTDIR ) -warn_on_inaccessible (path); +if (!fp)return NULL ; - } res =xcalloc (1 ,sizeof (*res));while (fgets (buf,sizeof (buf), fp)) {char *bufp = buf;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 06/21] attr.c: use fopen_or_warn()
Hi Duy,
Quoted 1 line · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
As commented about warn_on_fopen_errors(), I do not see a benefit in separating the introduction of this small function from the individual files' changes to use it. Let's bring it all together.
Ciao,
Dscho
PATCH v2 7/21 ident.c: use fopen_or_warn()
In reply to Nguyễn Thái Ngọc Duy--- ident.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
ident.c +3 -5
@@ -72,12 +72,10 @@ static int add_mailname_host(struct strbuf *buf)FILE *mailname;struct strbuf mailnamebuf =STRBUF_INIT ; - mailname =fopen ("/etc/mailname" ,"r" ); -if (!mailname) { -if (errno !=ENOENT ) -warning_errno ("cannot open /etc/mailname" ); + mailname =fopen_or_warn ("/etc/mailname" ,"r" ); +if (!mailname)return -1 ; - } +if (strbuf_getline (&mailnamebuf, mailname) ==EOF ) {if (ferror (mailname))warning_errno ("cannot read /etc/mailname" );
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 8/21 bisect: report on fopen() error
In reply to Nguyễn Thái Ngọc DuyThe main thing to catch here is when fopen() is called on a directory. It's safe even without this change because a few lines earlier we do check if "filename" is a regular file.
Regardless, let's stay on the safe side in case somebody changes those lines. Unconditionally printing to stderr by warn_on_inaccessible() should be fine, because the caller does unconditional fprintf(stderr, too, no checking for quiet option or anything.
--- bisect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
bisect.c +1 -1
@@ -666,7 +666,7 @@ static int is_expected_rev(const struct object_id *oid)if (stat (filename, &st) || !S_ISREG (st.st_mode))return 0 ; - fp =fopen (filename,"r" ); + fp =fopen_or_warn (filename,"r" );if (!fp)return 0 ;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 08/21] bisect: report on fopen() error
Hi Duy,
Quoted 10 lines · Nguyễn Thái Ngọc Duy
The main thing to catch here is when
fopen()is called on a directory. It's safe even without this change because a few lines earlier we do check if "filename" is a regular file.Regardless, let's stay on the safe side in case somebody changes those lines. Unconditionally printing to stderr by
warn_on_inaccessible()should be fine, because the caller does unconditional fprintf(stderr, too, no checking for quiet option or anything.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
I appreciate that you mention specifically the use case that triggered my complaint that in turn prodded you into starting working on this.
However, I think this should be a mere side note in a single commit that introduces and uses fopen_or_warn().
Ciao,
Dscho
PATCH v2 9/21 blame: report error on open if graft_file is a directory
In reply to Nguyễn Thái Ngọc Duy--- builtin/blame.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
builtin/blame.c +3 -1
@@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)*/ static int read_ancestry (const char *graft_file) { -FILE *fp =fopen (graft_file,"r" ); +FILE *fp =fopen_or_warn (graft_file,"r" );struct strbuf buf =STRBUF_INIT ; +if (!fp)return -1 ; +while (!strbuf_getwholeline (&buf, fp,'\n' )) {/* The format is just "Commit Parent1 Parent2 ...\n" */ struct commit_graft *graft =read_graft_line (buf.buf, buf.len);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 09/21] blame: report error on open if graft_file is a directory
Hi Duy,
Quoted 11 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- builtin/blame.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)builtin/blame.c +0
-0@@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)*/
builtin/blame.c static int prepare_lines(struct scoreboard *sb) · line 2080
Quoted 10 lines
builtin/blame.c +3
-1static int read_ancestry (const char *graft_file) { -FILE *fp =fopen (graft_file,"r" ); +FILE *fp =fopen_or_warn (graft_file,"r" );struct strbuf buf =STRBUF_INIT ; +if (!fp)return -1 ; +while (!strbuf_getwholeline (&buf, fp,'\n' )) {
This is an excellent example to demonstrate why folding all conversions to fopen_or_warn() into the same patch as introducing that function makes
sense: I had to close this mail, find the mail with the patch introducing the function, verify that it returns NULL as before, close that mail, and continue this reply.
Oh, and I do not think it is a good idea to introduce style fixes in the middle of such a large patch series. It's a bit distracting from the real meat here.
Ciao,
Dscho
PATCH v2 10/21 log: report errno on failure to fopen() a file
In reply to Nguyễn Thái Ngọc Duy--- builtin/log.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/log.c +2 -1
@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,printf ("%s\n" , filename.buf + outdir_offset);if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) -return error (_ ("Cannot open patch file %s" ), filename.buf); +return error_errno (_ ("Cannot open patch file %s" ), + filename.buf);strbuf_release (&filename);return 0 ;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 10/21] log: report errno on failure to fopen() a file
Hi Duy,
Quoted 1 line · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
I do think that this merits a one-line explanation why you do not use fopen_or_warn() here.
builtin/log.c static int open_next_file(struct commit *commit, const char *subject, · line 864
Quoted 13 lines
builtin/log.c +2
-1@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,printf ("%s\n" , filename.buf + outdir_offset);if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) -return error (_ ("Cannot open patch file %s" ), filename.buf); +return error_errno (_ ("Cannot open patch file %s" ), + filename.buf);strbuf_release (&filename);
The patches before and after this one all convert fopen() to fopen_or_warn(). If you *must* separate those into individual patches (which just makes this patch series unnecessarily bloated, if you ask me), you should at least group them a bit better so that the patch series tells more of a consistent story rather than jumping back and forth like a hyperactive four-year-old telling you about her latest adventure.
Ciao,
Dscho
PATCH v2 11/21 log: fix memory leak in open_next_file()
In reply to Nguyễn Thái Ngọc Duy--- builtin/log.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
builtin/log.c +8 -4
@@ -842,8 +842,10 @@ static int open_next_file(struct commit *commit, const char *subject,if (output_directory) {strbuf_addstr (&filename, output_directory);if (filename.len >= -PATH_MAX -FORMAT_PATCH_NAME_MAX - suffix_len) +PATH_MAX -FORMAT_PATCH_NAME_MAX - suffix_len) { +strbuf_release (&filename);return error (_ ("name of output directory is too long" )); + }strbuf_complete (&filename,'/' ); }
@@ -857,9 +859,11 @@ static int open_next_file(struct commit *commit, const char *subject,if (!quiet)printf ("%s\n" , filename.buf + outdir_offset); -if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) -return error_errno (_ ("Cannot open patch file %s" ), - filename.buf); +if ((rev->diffopt.file =fopen (filename.buf,"w" )) ==NULL ) { +error_errno (_ ("Cannot open patch file %s" ), filename.buf); +strbuf_release (&filename); +return -1 ; + }strbuf_release (&filename);return 0 ;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 12/21 commit.c: report error on failure to fopen() the graft file
In reply to Nguyễn Thái Ngọc DuySuppressing the error (because the command requires --quiet) is not a concern because we already call error() just a couple lines down.
--- commit.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
commit.c +3 -1
@@ -167,10 +167,12 @@ struct commit_graft *read_graft_line(char *buf, int len)static int read_graft_file (const char *graft_file) { -FILE *fp =fopen (graft_file,"r" ); +FILE *fp =fopen_or_warn (graft_file,"r" );struct strbuf buf =STRBUF_INIT ; +if (!fp)return -1 ; +while (!strbuf_getwholeline (&buf, fp,'\n' )) {/* The format is just "Commit Parent1 Parent2 ...\n" */ struct commit_graft *graft =read_graft_line (buf.buf, buf.len);
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 13/21 remote.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc DuyThere's plenty of error() in this code to safely assume --quiet is not a concern.
t5512 needs update because if we check the path 'refs*master' (i.e. the asterisk is part of the path) then we'll get an EINVAL error.
--- remote.c | 4 ++-- t/t5512-ls-remote.sh | 13 ++++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-)
remote.c +2 -2
@@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s)static void read_remotes_file (struct remote *remote) {struct strbuf buf =STRBUF_INIT ; -FILE *f =fopen (git_path ("remotes/%s" , remote->name),"r" ); +FILE *f =fopen_or_warn (git_path ("remotes/%s" , remote->name),"r" );if (!f)return ;
@@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote) {char *frag;struct strbuf buf =STRBUF_INIT ; -FILE *f =fopen (git_path ("branches/%s" , remote->name),"r" ); +FILE *f =fopen_or_warn (git_path ("branches/%s" , remote->name),"r" );if (!f)return ;
t/t5512-ls-remote.sh +10 -3
@@ -85,8 +85,15 @@ test_expect_success 'use branch.<name>.remote if possible' ' ' test_expect_success 'confuses pattern as remote when no remote specified' ' - cat >exp <<-\EOF && - fatal: '\''refs*master'\'' does not appear to be a git repository + if test_have_prereq MINGW + then + # Windows does not like asterisks in pathname + does_not_exist=master + else + does_not_exist="refs*master" + fi && + cat >exp <<-EOF && + fatal: '\''$does_not_exist'\'' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights
@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' ' # fetch <branch>. # We could just as easily have used "master"; the "*" emphasizes its # role as a pattern. - test_must_fail git ls-remote refs*master >actual 2>&1 && + test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 && test_i18ncmp exp actual '
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
Hi Duy,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
There's plenty of
error()in this code to safely assume --quiet is not a concern.t5512 needs update because if we check the path 'refs*master' (i.e. the asterisk is part of the path) then we'll get an EINVAL error.
So the first change in this patch unmasks a bug that is fixed by the second patch?
Quoted 23 lines
t/t5512-ls-remote.sh +9
-2@@ -85,8 +85,15 @@ test_expect_success 'use branch.<name>.remote if possible' ' ' test_expect_success 'confuses pattern as remote when no remote specified' ' - cat >exp <<-\EOF && - fatal: '\''refs*master'\'' does not appear to be a git repository + if test_have_prereq MINGW + then + # Windows does not like asterisks in pathname + does_not_exist=master + else + does_not_exist="refs*master" + fi && + cat >exp <<-EOF && + fatal: '\''$does_not_exist'\'' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights
t/t5512-ls-remote.sh test_expect_success 'confuses pattern as remote when no remote specified' ' · line 110
Quoted 7 lines
t/t5512-ls-remote.sh +1
-1@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' ' # fetch <branch>. # We could just as easily have used "master"; the "*" emphasizes its # role as a pattern. - test_must_fail git ls-remote refs*master >actual 2>&1 && + test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 && test_i18ncmp exp actual '
Sure enough. This totally looks like it needs to be a preparatory bug fix. Please separate it out and make sure that it comes before the fopen_or_warn() change in the patch series.
Ciao,
Dscho
Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
Sorry for super late reply. I'm slowly catching up.
Quoted 5 lines · Johannes Schindelin
Hi Duy,
On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:
Quoted 8 lines
Quoted 5 lines
There's plenty of
error()in this code to safely assume --quiet is not a concern.t5512 needs update because if we check the path 'refs*master' (i.e. the asterisk is part of the path) then we'll get an EINVAL error.
So the first change in this patch unmasks a bug that is fixed by the second patch?
The change in read_branches_file() in this patch causes the failure. See the original report [1],
[1] http://public-inbox.org/git/[email protected]/
Signature · 1 line
Duy
Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()
Hi Duy,
Quoted 15 lines · Duy Nguyen
Sorry for super late reply. I'm slowly catching up.
Quoted 11 lines · Johannes Schindelin
Hi Duy,
Quoted 5 lines · Nguyễn Thái Ngọc Duy
There's plenty of
error()in this code to safely assume --quiet is not a concern.t5512 needs update because if we check the path 'refs*master' (i.e. the asterisk is part of the path) then we'll get an EINVAL error.
Quoted 7 lines
Quoted 2 lines
So the first change in this patch unmasks a bug that is fixed by the second patch?
The change in
read_branches_file()in this patch causes the failure. See the original report [1],
I disagree. I do not think that the first part of the patch causes the failure. I think the failure was always there, we just did not bother to report (and catch) it properly.
Ciao,
Dscho
PATCH v2 14/21 rerere.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- rerere.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
rerere.c +1 -1
@@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1)static void read_rr (struct string_list *rr) {struct strbuf buf =STRBUF_INIT ; -FILE *in =fopen (git_path_merge_rr (),"r" ); +FILE *in =fopen_or_warn (git_path_merge_rr (),"r" );if (!in)return ;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 15/21 rerere.c: report correct errno
In reply to Nguyễn Thái Ngọc DuyIn the first case, we should have reported the reason fopen() fails. In the second case, fclose() might change errno but we want to report fopen() failure.
--- rerere.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
rerere.c +3 -2
@@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output io.input =fopen (path,"r" ); io.io.wrerror =0 ;if (!io.input) -return error ("Could not open %s" , path); +return error_errno ("Could not open %s" , path);if (output) { io.io.output =fopen (output,"w" );if (!io.io.output) { +error_errno ("Could not write %s" , output);fclose (io.input); -return error ("Could not write %s" , output); +return -1 ; } }
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 15/21] rerere.c: report correct errno
Hi Duy,
rerere.c static int handle_file(const char *path, unsigned char *sha1, const char *output · line 487
Quoted 10 lines · Nguyễn Thái Ngọc Duy
rerere.c +1
-1@@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output io.input =fopen (path,"r" ); io.io.wrerror =0 ;if (!io.input) -return error ("Could not open %s" , path); +return error_errno ("Could not open %s" , path);
IMO the error() -> error_errno() changes should all be part of the same commit, as they probably share the explanation why fopen_or_warn() is not appropriate here.
rerere.c static int handle_file(const char *path, unsigned char *sha1, const char *output · line 495
Quoted 8 lines
rerere.c +2
-1if (output) { io.io.output =fopen (output,"w" );if (!io.io.output) { +error_errno ("Could not write %s" , output);fclose (io.input); -return error ("Could not write %s" , output); +return -1 ; }
This one is logically different from the change above, as it not only cannot be replaced by fopen_or_warn(), but also requires the reordering due to the different nature of the change.
Ciao,
Dscho
PATCH v2 16/21 sequencer.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- sequencer.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
sequencer.c +4 -4
@@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) {FILE *out;if (strbuf_read_file (&buf,rebase_path_rewritten_pending (),82 ) >0 && - !get_sha1 ("HEAD" , newsha1) && - (out =fopen (rebase_path_rewritten_list (),"a" ))) { + !get_sha1 ("HEAD" , newsha1) && + (out =fopen_or_warn (rebase_path_rewritten_list (),"a" ))) {char *bol = buf.buf, *eol;while (*bol) {
@@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) {static void record_in_rewritten (struct object_id *oid,enum todo_command next_command) { -FILE *out =fopen (rebase_path_rewritten_pending (),"a" ); +FILE *out =fopen_or_warn (rebase_path_rewritten_pending (),"a" );if (!out)return ;
@@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list,if (is_rebase_i (opts)) {struct todo_list done =TODO_LIST_INIT ; -FILE *f =fopen (rebase_path_msgtotal (),"w" ); +FILE *f =fopen_or_warn (rebase_path_msgtotal (),"w" );if (strbuf_read_file (&done.buf,rebase_path_done (),0 ) >0 && !parse_insn_buffer (done.buf.buf, &done))
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 17/21 server-info: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- server-info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
server-info.c +1 -1
@@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile)char line[1000 ];int old_cnt =0 ; - fp =fopen (infofile,"r" ); + fp =fopen_or_warn (infofile,"r" );if (!fp)return 1 ;/* nonexistent is not an error. */
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 18/21 wt-status.c: report error on failure to fopen()
In reply to Nguyễn Thái Ngọc Duy--- wt-status.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
wt-status.c +2 -1
@@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s,static char *read_line_from_git_path (const char *filename) {struct strbuf buf =STRBUF_INIT ; -FILE *fp =fopen (git_path ("%s" , filename),"r" ); +FILE *fp =fopen_or_warn (git_path ("%s" , filename),"r" ); +if (!fp) {strbuf_release (&buf);return NULL ;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 19/21 xdiff-interface.c: report errno on failure to stat() or fopen()
In reply to Nguyễn Thái Ngọc Duy--- xdiff-interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
xdiff-interface.c +2 -2
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)size_t sz;if (stat (filename, &st)) -return error ("Could not stat %s" , filename); +return error_errno ("Could not stat %s" , filename);if ((f =fopen (filename,"rb" )) ==NULL ) -return error ("Could not open %s" , filename); +return error_errno ("Could not open %s" , filename); sz =xsize_t (st.st_size); ptr->ptr =xmalloc (sz ? sz :1 );if (sz &&fread (ptr->ptr, sz,1 , f) !=1 ) {
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 20/21 config.c: handle error on failing to fopen()
In reply to Nguyễn Thái Ngọc DuyIn the first case, we already correctly return -1 if fopen() fails to open. But we should report something so people know what's wrong.
In the second case, config_file == NULL does not necessarily mean "no config file". Bail out if needed.
--- config.c | 5 ++++- t/t1308-config-set.sh | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-)
config.c +4 -1
@@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)int ret =-1 ;FILE *f; - f =fopen (filename,"r" ); + f =fopen_or_warn (filename,"r" );if (f) {flockfile (f); ret =do_config_from_file (fn,CONFIG_ORIGIN_FILE , filename, filename, f, data);
@@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char *config_filename, }if (!(config_file =fopen (config_filename,"rb" ))) { + ret =warn_on_fopen_errors (config_filename); +if (ret) + goto out;/* no config file means nothing to rename, no error */ goto commit_and_out; }
t/t1308-config-set.sh +3 -1
@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" && echo "Error (-1) reading configuration file .git/config." >expect && - test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual && + test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output && + grep "^warning:" output && + grep "^Error" output >actual && test_cmp expect actual '
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 20/21] config.c: handle error on failing to fopen()
Hi Duy,
Quoted 7 lines · Nguyễn Thái Ngọc Duy
In the first case, we already correctly return -1 if
fopen()fails to open. But we should report something so people know what's wrong.In the second case,
config_file== NULL does not necessarily mean "no config file". Bail out if needed.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
Again, it seems that your patch series tries to cut at the file boundary, not at the "logically the same change" boundary, making it a bit more cumbersome than necessary to follow this patch series.
I highly recommend squashing the first change into the big fopen() -> fopen_or_warn() patch I hinted at earlier, and let the second change (with the accompanying test case) stand on its own.
Ciao,
Dscho
PATCH v2 21/21 t1308: add a test case on open a config directory
In reply to Nguyễn Thái Ngọc DuyWe don't support config-as-a-directory (maybe someday we will?). Make sure we consistently fail in this case, which should happen on platforms where fopen(<directory>) returns non-NULL if FREAD_READS_DIRECTORIES is defined.
--- t/t1308-config-set.sh | 9 +++++++++ 1 file changed, 9 insertions(+)
t/t1308-config-set.sh +9 -0
@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' ' test_cmp expect actual ' +test_expect_success 'proper error on directory "files"' ' + echo "Error (-1) reading configuration file a-directory." >expect && + mkdir a-directory && + test_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output && + grep "^warning:" output && + grep "^Error" output >actual && + test_cmp expect actual +' + test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' ' chmod -r .git/config && test_when_finished "chmod +r .git/config" &&
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v2 21/21] t1308: add a test case on open a config directory
Hi Duy,
Quoted 6 lines · Nguyễn Thái Ngọc Duy
We don't support config-as-a-directory (maybe someday we will?). Make sure we consistently fail in this case, which should happen on platforms where fopen(<directory>) returns non-NULL if
FREAD_READS_DIRECTORIESis defined.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
How about saying that we make sure that this is the case by adding a test case? Otherwise the reader might (and this reader certainly did) expect a code change in a .c file.
I reviewed this patch series, and am happy with the overall diff.
My main problem: it should be organized in a more intuitively-graspable way, i.e. put all the fopen() -> fopen_or_warn() changes into a single patch, separate out changes (drive-by test fixing for Windows and, ahem, whitespace changes) into their own commits. Then those commits should be ordered to relate an easy-flowing story.
Other than that: well done,
Dscho
Re: [PATCH v2 00/21]
In reply to Nguyễn Thái Ngọc DuyQuoted 26 lines · Nguyễn Thái Ngọc Duy
Changes since v1:
-
fopen_or_warn()andwarn_on_fopen_errors()are introduced. The latter's name is probably not great...
- A new patch (first one) to convert a bunch to usingxfopen()
- Fix test failure on Windows, found by Johannes Sixt
- Fix the memory leak inlog.c, found by JeffThere are still a couple of
fopen()remained, but I'm getting slow again so let's get these in first and worry about the rest when somebody has time.Nguyễn Thái Ngọc Duy (21):
Usexfopen()in more places
clone: usexfopen()instead offopen()
config.mak.uname: setFREAD_READS_DIRECTORIESfor Linux and FreeBSDwrapper.c: addwarn_on_fopen_errors()
wrapper.c: addfopen_or_warn()
attr.c: usefopen_or_warn()
ident.c: usefopen_or_warn()
bisect: report onfopen()error
blame: report error on open ifgraft_fileis a directory
log: report errno on failure tofopen()a file
log: fix memory leak inopen_next_file()
commit.c: report error on failure tofopen()the graft fileremote.c: report error on failure tofopen()
Quoted 8 lines
rerere.c: report error on failure tofopen()rerere.c: report correct errno
sequencer.c: report error on failure tofopen()
server-info: report error on failure tofopen()wt-status.c: report error on failure tofopen()xdiff-interface.c: report errno on failure tostat()orfopen()config.c: handle error on failing tofopen()
t1308: add a test case on open a config directory
Thanks. If the number of parts affected by this series were smaller, it may have made the review easier to have the introduction of a helper and its use in a single larger patch, but there are spread across many, some with files that are touched by different in-flight topics, and these "collection of smaller patches" makes it easier to manage both while reviewing and also merging.
All looked good, even though I do share the doubt on the name "warn-on-fopen-errors"; when something applies equally to fopen(3) and underlying open(2), I would tend to call that with open not fopen myself. But that is a minor point.
Re: [PATCH v2 00/21]
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Nguyễn Thái Ngọc Duy
Changes since v1:
-
fopen_or_warn()andwarn_on_fopen_errors()are introduced. The latter's name is probably not great...
- A new patch (first one) to convert a bunch to usingxfopen()
- Fix test failure on Windows, found by Johannes Sixt
- Fix the memory leak inlog.c, found by Jeff
Quoted 3 lines
Quoted 3 lines
There are still a couple of
fopen()remained, but I'm getting slow again so let's get these in first and worry about the rest when somebody has time.
Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
https://travis-ci.org/git/git/jobs/229585098#L3091
seems to expect an error when test-config is fed a-directory but we are not getting the expected warning and error?
Re: [PATCH v2 00/21]
Quoted 16 lines · Junio C Hamano
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Nguyễn Thái Ngọc Duy
Changes since v1:
-
fopen_or_warn()andwarn_on_fopen_errors()are introduced. The latter's name is probably not great...
- A new patch (first one) to convert a bunch to usingxfopen()
- Fix test failure on Windows, found by Johannes Sixt
- Fix the memory leak inlog.c, found by JeffQuoted 3 lines
Quoted 3 lines
There are still a couple of
fopen()remained, but I'm getting slow again so let's get these in first and worry about the rest when somebody has time.
Quoted 6 lines
Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
https://travis-ci.org/git/git/jobs/229585098#L3091
seems to expect an error when test-config is fed a-directory but we are not getting the expected warning and error?
Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on MacOS X makes travis happy.
Signature · 1 line
Duy
Re: [PATCH v2 00/21]
Quoted 19 lines · Duy Nguyen
Quoted 18 lines · Junio C Hamano
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Nguyễn Thái Ngọc Duy
Changes since v1:
-
fopen_or_warn()andwarn_on_fopen_errors()are introduced. The latter's name is probably not great...
- A new patch (first one) to convert a bunch to usingxfopen()
- Fix test failure on Windows, found by Johannes Sixt
- Fix the memory leak inlog.c, found by JeffQuoted 3 lines
Quoted 3 lines
There are still a couple of
fopen()remained, but I'm getting slow again so let's get these in first and worry about the rest when somebody has time.Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?
Quoted 7 lines
Quoted 4 lines
https://travis-ci.org/git/git/jobs/229585098#L3091
seems to expect an error when test-config is fed a-directory but we are not getting the expected warning and error?
Sounds about right. Let me see if defining
FREAD_READS_DIRECTORIESon MacOS X makes travis happy.
Thanks. I should have suspected that myself to save a round-trip.
PATCH 1/1 config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
Let's do the same for Macs, as it is BSD variant after all.
--- config.mak.uname | 1 + 1 file changed, 1 insertion(+)
config.mak.uname +1 -0
@@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin) BASIC_CFLAGS += -DPRECOMPOSE_UNICODE BASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1 HAVE_BSD_SYSCTL = YesPlease + FREAD_READS_DIRECTORIES = UnfortunatelyYes endif ifeq ($(uname_S),SunOS) NEEDS_SOCKET = YesPlease
Signature · 1 line
2.13.0-336-gf73534b083