{"thread":{"id":"45757","subject":"[PATCH 00/15] Handle fopen() errors","startedAt":"2017-04-20T11:26:56Z","lastAt":"2017-05-10T05:12:45Z","messageCount":70,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Duy Nguyen","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":15},"messages":[{"id":"317327","messageId":"20170420112609.26089-1-pclouds@gmail.com","threadId":"45757","inReplyTo":null,"subject":"[PATCH 00/15] Handle fopen() errors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:54Z","receivedAt":"2017-04-20T11:26:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Some of you may recall a while back, nd/conditional-config-include\nfailed on Windows because I accidentally fopen()'d a directory in a\ntest, but it's not considered an serious error unless it's on Windows,\nwhere fopen() returns NULL.\n\nA couple of suggestions were thrown back and forth, but I was a bit\nbusy to follow up. Now that I have time, I have audited all fopen()\ncalls and try to fix them up for good. There 15 patches, but they only\nchange one or two lines each. I split them anyway so you can pause\nbetween patches and see if it really makes sense, as changes are all\nover the places.\n\nThere are still a few iffy fopen() calls in sequencer.c though. I only\nfixed the easy ones in there.\n\nThe last patch may fail on some platforms since I want to make sure\nthat fopen() == NULL is an expected behavior, even though I\ncould only test FreeBSD and Linux (and know Windows behaves the same).\nAt least when people shout up, we could start adding\nFREAD_READS_DIRECTORIES on those platforms. That's the goal.\n\nNguyễn Thái Ngọc Duy (15):\n config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD\n bisect: report on fopen() error\n blame: report error on open if graft_file is a directory\n clone: use xfopen() instead of fopen()\n log: report errno on failure to fopen() a file\n commit.c: report error on failure to fopen() the graft file\n remote.c: report error on failure to fopen()\n rerere.c: report error on failure to fopen()\n rerere.c: report correct errno\n sequencer.c: report error on failure to fopen()\n server-info: report error on failure to fopen()\n wt-status.c: report error on failure to fopen()\n xdiff-interface.c: report errno on failure to stat() or fopen()\n config.c: handle error on failing to fopen()\n t1308: add a test case on open a config directory\n\n bisect.c | 5 ++++-\n builtin/blame.c | 5 ++++-\n builtin/clone.c | 2 +-\n builtin/log.c | 3 ++-\n commit.c | 5 ++++-\n config.c | 8 +++++++-\n config.mak.uname | 3 +++\n remote.c | 12 ++++++++++--\n rerere.c | 10 +++++++---\n sequencer.c | 5 ++++-\n server-info.c | 5 ++++-\n t/t1308-config-set.sh | 13 ++++++++++++-\n wt-status.c | 2 ++\n xdiff-interface.c | 4 ++--\n 14 files changed, 66 insertions(+), 16 deletions(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317328","messageId":"20170420112609.26089-3-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 02/15] bisect: report on fopen() error","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:56Z","receivedAt":"2017-04-20T11:26:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The main thing to catch here is when fopen() is called on a\ndirectory. It's safe even without this change because a few lines\nearlier we do check if \"filename\" is a regular file.\n\nRegardless, let's stay on the safe side in case somebody changes those\nlines. Unconditionally printing to stderr by warn_on_inaccessible()\nshould be fine, because the caller does unconditional fprintf(stderr,\ntoo, no checking for quiet option or anything.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n bisect.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 03af06c66c..6dc4dc9397 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -669,8 +669,11 @@ static int is_expected_rev(const struct object_id *oid)\n \t\treturn 0;\n \n \tfp = fopen(filename, \"r\");\n-\tif (!fp)\n+\tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(filename);\n \t\treturn 0;\n+\t}\n \n \tif (strbuf_getline_lf(&str, fp) != EOF)\n \t\tres = !strcmp(str.buf, oid_to_hex(oid));\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317329","messageId":"20170420112609.26089-4-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 03/15] blame: report error on open if graft_file is a directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:57Z","receivedAt":"2017-04-20T11:26:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/blame.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 07506a3e45..70afa1b05c 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2073,8 +2073,11 @@ static int read_ancestry(const char *graft_file)\n {\n \tFILE *fp = fopen(graft_file, \"r\");\n \tstruct strbuf buf = STRBUF_INIT;\n-\tif (!fp)\n+\tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(graft_file);\n \t\treturn -1;\n+\t}\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317330","messageId":"20170420112609.26089-2-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 01/15] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:55Z","receivedAt":"2017-04-20T11:26:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This variable is added [1] with the assumption that on a sane system,\nfopen(, \"r\") should return NULL. Linux and FreeBSD do not meet this\nexpectation while at least Windows and AIX do. Let's make sure they\nbehave the same way.\n\nI only tested one version on Linux (4.7.0 with glibc 2.22) and\nFreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace,\nI'm pretty sure it shares the same problem.\n\n[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open\n directory - 2008-02-08)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n config.mak.uname | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 399fe19271..a25ffddb3e 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)\n \tNEEDS_LIBRT = YesPlease\n \tHAVE_GETDELIM = YesPlease\n \tSANE_TEXT_GREP=-a\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n \tLIBC_CONTAINS_LIBINTL = YesPlease\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),UnixWare)\n \tCC = cc\n@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317331","messageId":"20170420112609.26089-6-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 05/15] log: report errno on failure to fopen() a file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:59Z","receivedAt":"2017-04-20T11:27:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1ed..26d6a3cf14 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,\n \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n \n \tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL)\n-\t\treturn error(_(\"Cannot open patch file %s\"), filename.buf);\n+\t\treturn error_errno(_(\"Cannot open patch file %s\"),\n+\t\t\t\t filename.buf);\n \n \tstrbuf_release(&filename);\n \treturn 0;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317332","messageId":"20170420112609.26089-7-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 06/15] commit.c: report error on failure to fopen() the graft file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:00Z","receivedAt":"2017-04-20T11:27:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Suppressing the error (because the command requires --quiet) is not a\nconcern because we already call error() just a couple lines down.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n commit.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 73c78c2b80..8726225a7c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -169,8 +169,11 @@ static int read_graft_file(const char *graft_file)\n {\n \tFILE *fp = fopen(graft_file, \"r\");\n \tstruct strbuf buf = STRBUF_INIT;\n-\tif (!fp)\n+\tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(graft_file);\n \t\treturn -1;\n+\t}\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317333","messageId":"20170420112609.26089-10-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 09/15] rerere.c: report correct errno","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:03Z","receivedAt":"2017-04-20T11:27:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the first case, we should have reported the reason fopen() fails. In\nthe second case, fclose() might change errno but we want to report\nfopen() failure.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n rerere.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex ef9b11578f..f10c3d8ae6 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -487,13 +487,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output\n \tio.input = fopen(path, \"r\");\n \tio.io.wrerror = 0;\n \tif (!io.input)\n-\t\treturn error(\"Could not open %s\", path);\n+\t\treturn error_errno(\"Could not open %s\", path);\n \n \tif (output) {\n \t\tio.io.output = fopen(output, \"w\");\n \t\tif (!io.io.output) {\n+\t\t\terror_errno(\"Could not write %s\", output);\n \t\t\tfclose(io.input);\n-\t\t\treturn error(\"Could not write %s\", output);\n+\t\t\treturn -1;\n \t\t}\n \t}\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317334","messageId":"20170420112609.26089-5-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 04/15] clone: use xfopen() instead of fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:25:58Z","receivedAt":"2017-04-20T11:27:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This code uses the result FILE pointer right away without the NULL\ncheck. Let's use xfopen() and die if we could not open the file. That's\nstill better than crashing,\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex de85b85254..dde4fe73af 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n \t * to turn entries with paths relative to the original\n \t * absolute, so that they can be used in the new repository.\n \t */\n-\tFILE *in = fopen(src->buf, \"r\");\n+\tFILE *in = xfopen(src->buf, \"r\");\n \tstruct strbuf line = STRBUF_INIT;\n \n \twhile (strbuf_getline(&line, in) != EOF) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317335","messageId":"20170420112609.26089-11-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 10/15] sequencer.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:04Z","receivedAt":"2017-04-20T11:27:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n sequencer.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaebf..8d5ebfc14f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -919,8 +919,11 @@ static void record_in_rewritten(struct object_id *oid,\n \t\tenum todo_command next_command) {\n \tFILE *out = fopen(rebase_path_rewritten_pending(), \"a\");\n \n-\tif (!out)\n+\tif (!out) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(rebase_path_rewritten_pending());\n \t\treturn;\n+\t}\n \n \tfprintf(out, \"%s\\n\", oid_to_hex(oid));\n \tfclose(out);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317336","messageId":"20170420112609.26089-9-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 08/15] rerere.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:02Z","receivedAt":"2017-04-20T11:27:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n rerere.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 3bd55caf3b..ef9b11578f 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -202,8 +202,11 @@ static void read_rr(struct string_list *rr)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *in = fopen(git_path_merge_rr(), \"r\");\n \n-\tif (!in)\n+\tif (!in) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(git_path_merge_rr());\n \t\treturn;\n+\t}\n \twhile (!strbuf_getwholeline(&buf, in, '\\0')) {\n \t\tchar *path;\n \t\tunsigned char sha1[20];\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317337","messageId":"20170420112609.26089-12-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 11/15] server-info: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:05Z","receivedAt":"2017-04-20T11:27:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n server-info.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 7bc4e75d22..7fc2a76966 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -132,8 +132,11 @@ static int read_pack_info_file(const char *infofile)\n \tint old_cnt = 0;\n \n \tfp = fopen(infofile, \"r\");\n-\tif (!fp)\n+\tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(infofile);\n \t\treturn 1; /* nonexistent is not an error. */\n+\t}\n \n \twhile (fgets(line, sizeof(line), fp)) {\n \t\tint len = strlen(line);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317338","messageId":"20170420112609.26089-8-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:01Z","receivedAt":"2017-04-20T11:27:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There's plenty of error() in this code to safely assume --quiet is not a\nconcern.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n remote.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 801137c72e..8ccc1e7b99 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -253,8 +253,12 @@ static void read_remotes_file(struct remote *remote)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n \n-\tif (!f)\n+\tif (!f) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(git_path(\"remotes/%s\",\n+\t\t\t\t\t\t remote->name));\n \t\treturn;\n+\t}\n \tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_REMOTES;\n \twhile (strbuf_getline(&buf, f) != EOF) {\n@@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f = fopen(git_path(\"branches/%s\", remote->name), \"r\");\n \n-\tif (!f)\n+\tif (!f) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(git_path(\"branches/%s\",\n+\t\t\t\t\t\t remote->name));\n \t\treturn;\n+\t}\n \n \tstrbuf_getline_lf(&buf, f);\n \tfclose(f);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317339","messageId":"20170420112609.26089-13-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 12/15] wt-status.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:06Z","receivedAt":"2017-04-20T11:27:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n wt-status.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 0375484962..5c12bb6ae3 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1067,6 +1067,8 @@ static char *read_line_from_git_path(const char *filename)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *fp = fopen(git_path(\"%s\", filename), \"r\");\n \tif (!fp) {\n+\t\tif (errno != ENOENT)\n+\t\t\twarn_on_inaccessible(git_path(\"%s\", filename));\n \t\tstrbuf_release(&buf);\n \t\treturn NULL;\n \t}\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317340","messageId":"20170420112609.26089-16-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 15/15] t1308: add a test case on open a config directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:09Z","receivedAt":"2017-04-20T11:28:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We don't support config-as-a-directory (maybe someday we will?). Make\nsure we consistently fail in this case, which should happen on platforms\nwhere fopen() returns non-NULL if FREAD_READS_DIRECTORIES is\ndefined.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t1308-config-set.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 13e95561f4..e495a61616 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'proper error on directory \"files\"' '\n+\techo \"Error (-1) reading configuration file a-directory.\" >expect &&\n+\tmkdir a-directory &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&\n+\tgrep \"^warning:\" output &&\n+\tgrep \"^Error\" output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '\n \tchmod -r .git/config &&\n \ttest_when_finished \"chmod +r .git/config\" &&\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317341","messageId":"20170420112609.26089-15-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 14/15] config.c: handle error on failing to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:08Z","receivedAt":"2017-04-20T11:29:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the first case, we already correctly return -1 if fopen() fails to\nopen. But we should report something so people know what's wrong.\n\nIn the second case, config_file == NULL does not necessarily mean \"no\nconfig file\". Bail out if needed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n config.c | 8 +++++++-\n t/t1308-config-set.sh | 4 +++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..ac9effa7aa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1401,7 +1401,8 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);\n \t\tfunlockfile(f);\n \t\tfclose(f);\n-\t}\n+\t} else if (errno != ENOENT)\n+\t\twarn_on_inaccessible(filename);\n \treturn ret;\n }\n \n@@ -2601,6 +2602,11 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t}\n \n \tif (!(config_file = fopen(config_filename, \"rb\"))) {\n+\t\tif (errno != ENOENT) {\n+\t\t\twarn_on_inaccessible(config_filename);\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n+\t\t}\n \t\t/* no config file means nothing to rename, no error */\n \t\tgoto commit_and_out;\n \t}\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex ff50960cca..13e95561f4 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '\n \tchmod -r .git/config &&\n \ttest_when_finished \"chmod +r .git/config\" &&\n \techo \"Error (-1) reading configuration file .git/config.\" >expect &&\n-\ttest_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&\n+\tgrep \"^warning:\" output &&\n+\tgrep \"^Error\" output >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317342","messageId":"20170420112609.26089-14-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH 13/15] xdiff-interface.c: report errno on failure to stat() or fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-20T11:26:07Z","receivedAt":"2017-04-20T11:33:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n xdiff-interface.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 060038c2d6..d3f78ca2a7 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tsize_t sz;\n \n \tif (stat(filename, &st))\n-\t\treturn error(\"Could not stat %s\", filename);\n+\t\treturn error_errno(\"Could not stat %s\", filename);\n \tif ((f = fopen(filename, \"rb\")) == NULL)\n-\t\treturn error(\"Could not open %s\", filename);\n+\t\treturn error_errno(\"Could not open %s\", filename);\n \tsz = xsize_t(st.st_size);\n \tptr->ptr = xmalloc(sz ? sz : 1);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"317445","messageId":"xmqqlgqu7cq8.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T01:47:59Z","receivedAt":"2017-04-21T01:51:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Some of you may recall a while back, nd/conditional-config-include\n> failed on Windows because I accidentally fopen()'d a directory in a\n> test, but it's not considered an serious error unless it's on Windows,\n> where fopen() returns NULL.\n>\n> A couple of suggestions were thrown back and forth, but I was a bit\n> busy to follow up. Now that I have time, I have audited all fopen()\n> calls and try to fix them up for good. There 15 patches, but they only\n> change one or two lines each. I split them anyway so you can pause\n> between patches and see if it really makes sense, as changes are all\n> over the places.\n\nNicely done. \n\nI wonder if it is OK to only special case ENOENT for !fp cases,\nwhere existing code silently returns. Perhaps it is trying to read\nan optional file, and it returns silently because lack of it is\nperfectly OK for the purpose of the code. Are there cases where\nthis optional file is inside an optional directory, leading to\nENOTDIR, instead of ENOENT, observed and reported by your check?\n\n"},{"id":"317452","messageId":"xmqqk26e5swj.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"xmqqlgqu7cq8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T03:41:32Z","receivedAt":"2017-04-21T03:41:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I wonder if it is OK to only special case ENOENT for !fp cases,\n> where existing code silently returns. Perhaps it is trying to read\n> an optional file, and it returns silently because lack of it is\n> perfectly OK for the purpose of the code. Are there cases where\n> this optional file is inside an optional directory, leading to\n> ENOTDIR, instead of ENOENT, observed and reported by your check?\n\n\"git grep -B1 warn_on_inaccessible\" is enlightening. I wonder if we\nwant to wrap the two lines into a hard to misuse helper function,\nsomething along this line. Would having this patch as a preparatory\nstep shrink your series? The patch count would be the same, but you\nwouldn't be writing \"if (errno != ENOENT)\" lines yourself.\n\n attr.c | 3 +--\n git-compat-util.h | 3 +++\n wrapper.c | 6 ++++++\n 3 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 1fcf042b87..f695ded53f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -373,8 +373,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \tint lineno = 0;\n \n \tif (!fp) {\n-\t\tif (errno != ENOENT && errno != ENOTDIR)\n-\t\t\twarn_on_inaccessible(path);\n+\t\twarn_failure_to_read_open_optional_path(path);\n \t\treturn NULL;\n \t}\n \tres = xcalloc(1, sizeof(*res));\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 8a4a3f85e7..998366c628 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1094,6 +1094,9 @@ int access_or_die(const char *path, int mode, unsigned flag);\n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\n \n+/* Call the above after fopen/open fails for optional input */\n+void warn_failure_to_read_open_optional_path(const char *);\n+\n #ifdef GMTIME_UNRELIABLE_ERRORS\n struct tm *git_gmtime(const time_t *);\n struct tm *git_gmtime_r(const time_t *, struct tm *);\ndiff --git a/wrapper.c b/wrapper.c\nindex 0542fc7582..172cb9fad6 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -576,6 +576,12 @@ int remove_or_warn(unsigned int mode, const char *file)\n \treturn S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);\n }\n \n+void warn_failure_to_read_open_optional_path(const char *path)\n+{\n+\tif (errno != ENOENT && errno != ENOTDIR)\n+\t\twarn_on_inaccessible(path);\n+}\n+\n void warn_on_inaccessible(const char *path)\n {\n \twarning_errno(_(\"unable to access '%s'\"), path);\n"},{"id":"317460","messageId":"20170421062915.he5tlgjqq7kj5h32@sigill.intra.peff.net","threadId":"45757","inReplyTo":"xmqqk26e5swj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T06:29:15Z","receivedAt":"2017-04-21T06:29:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I wonder if it is OK to only special case ENOENT for !fp cases,\n> > where existing code silently returns. Perhaps it is trying to read\n> > an optional file, and it returns silently because lack of it is\n> > perfectly OK for the purpose of the code. Are there cases where\n> > this optional file is inside an optional directory, leading to\n> > ENOTDIR, instead of ENOENT, observed and reported by your check?\n> \n> \"git grep -B1 warn_on_inaccessible\" is enlightening. I wonder if we\n> want to wrap the two lines into a hard to misuse helper function,\n> something along this line. Would having this patch as a preparatory\n> step shrink your series? The patch count would be the same, but you\n> wouldn't be writing \"if (errno != ENOENT)\" lines yourself.\n\nI had a similar thought while reading through it. I think it would be\nshorter still with:\n\n FILE *fopen_or_warn(const char *path, const char *mode)\n {\n\tFILE *fh = fopen(path, mode);\n\tif (!fh)\n\t\twarn_failure_to_read_open_optional_path(path);\n\treturn fh;\n }\n\nAnd then quite a few of the patches could just be\ns/fopen/fopen_or_warn/.\n\n-Peff\n"},{"id":"317462","messageId":"20170421063324.uumqm4shxunyoii4@sigill.intra.peff.net","threadId":"45757","inReplyTo":"20170420112609.26089-6-pclouds@gmail.com","subject":"Re: [PATCH 05/15] log: report errno on failure to fopen() a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T06:33:25Z","receivedAt":"2017-04-21T06:33:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 06:25:59PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/log.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1ed..26d6a3cf14 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,\n> \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n> \n> \tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL)\n> -\t\treturn error(_(\"Cannot open patch file %s\"), filename.buf);\n> +\t\treturn error_errno(_(\"Cannot open patch file %s\"),\n> +\t\t\t\t filename.buf);\n> \n> \tstrbuf_release(&filename);\n> \treturn 0;\n\nNot a new problem with your patch, but just looking at the context it\nseems clear that \"filename\" is leaked in the error case.\n\n-Peff\n"},{"id":"317482","messageId":"CACsJy8D1LuH6qVp15MSkCM_oQphVUUK0r9SeKC5AzX+9Xi2dcw@mail.gmail.com","threadId":"45757","inReplyTo":"20170421062915.he5tlgjqq7kj5h32@sigill.intra.peff.net","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-21T11:04:49Z","receivedAt":"2017-04-21T11:05:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 21, 2017 at 1:29 PM, Jeff King wrote:\n> On Thu, Apr 20, 2017 at 08:41:32PM -0700, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>>\n>> > I wonder if it is OK to only special case ENOENT for !fp cases,\n>> > where existing code silently returns. Perhaps it is trying to read\n>> > an optional file, and it returns silently because lack of it is\n>> > perfectly OK for the purpose of the code. Are there cases where\n>> > this optional file is inside an optional directory, leading to\n>> > ENOTDIR, instead of ENOENT, observed and reported by your check?\n>>\n>> \"git grep -B1 warn_on_inaccessible\" is enlightening. I wonder if we\n>> want to wrap the two lines into a hard to misuse helper function,\n>> something along this line. Would having this patch as a preparatory\n>> step shrink your series? The patch count would be the same, but you\n>> wouldn't be writing \"if (errno != ENOENT)\" lines yourself.\n>\n> I had a similar thought while reading through it. I think it would be\n> shorter still with:\n>\n> FILE *fopen_or_warn(const char *path, const char *mode)\n> {\n> FILE *fh = fopen(path, mode);\n> if (!fh)\n> warn_failure_to_read_open_optional_path(path);\n> return fh;\n> }\n>\n> And then quite a few of the patches could just be\n> s/fopen/fopen_or_warn/.\n\nJeff.. oh Jeff.. you have made it _way_ too convenient that after a\nquick grep at fopen( again, I found a couple more places that I would\nhave just ignored last time (too much work), but now all I need to do\nis Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)\n-- \nDuy\n"},{"id":"317484","messageId":"CAPc5daXPgEFibr28-EZjk9_vYrrO2qt9VLXW6PepmFXUChpk7Q@mail.gmail.com","threadId":"45757","inReplyTo":"CACsJy8D1LuH6qVp15MSkCM_oQphVUUK0r9SeKC5AzX+9Xi2dcw@mail.gmail.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T11:52:13Z","receivedAt":"2017-04-21T11:52:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Apr 21, 2017 at 8:04 PM, Duy Nguyen wrote:\n> On Fri, Apr 21, 2017 at 1:29 PM, Jeff King wrote:\n>>\n>> I had a similar thought while reading through it. I think it would be\n>> shorter still with:\n>>\n>> FILE *fopen_or_warn(const char *path, const char *mode)\n>> {\n>> FILE *fh = fopen(path, mode);\n>> if (!fh)\n>> warn_failure_to_read_open_optional_path(path);\n>> return fh;\n>> }\n>>\n>> And then quite a few of the patches could just be\n>> s/fopen/fopen_or_warn/.\n>\n> Jeff.. oh Jeff.. you have made it _way_ too convenient that after a\n> quick grep at fopen( again, I found a couple more places that I would\n> have just ignored last time (too much work), but now all I need to do\n> is Alt-f to the end of fopen and Alt-/ a few times. Too tempting.. :)\n\nYes, but (1) we'd need to be careful about --quiet, and (2) we would also need\na wrapper for open(2).\n"},{"id":"317487","messageId":"CACsJy8CCW+gQ6n2VOC4nmRBukHSLyxYizQhowQoNOc8weZzJjA@mail.gmail.com","threadId":"45757","inReplyTo":"CAPc5daXPgEFibr28-EZjk9_vYrrO2qt9VLXW6PepmFXUChpk7Q@mail.gmail.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-21T12:27:20Z","receivedAt":"2017-04-21T12:27:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano wrote:\n> Yes, but (1) we'd need to be careful about --quiet\n\nYeah. It's a real pain point for making changes like this. At some\npoint we should just have a global (maybe multi-level) quiet flag.\n-- \nDuy\n"},{"id":"317506","messageId":"20170421170700.qyjtrvew54u2epue@sigill.intra.peff.net","threadId":"45757","inReplyTo":"CACsJy8CCW+gQ6n2VOC4nmRBukHSLyxYizQhowQoNOc8weZzJjA@mail.gmail.com","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T17:07:00Z","receivedAt":"2017-04-21T18:53:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:\n\n> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano wrote:\n> > Yes, but (1) we'd need to be careful about --quiet\n> \n> Yeah. It's a real pain point for making changes like this. At some\n> point we should just have a global (maybe multi-level) quiet flag.\n\nI don't think it's too bad here. Isn't it just:\n\n FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);\n\nIt is a little annoying to have to repeat \"x\", though (which is\nsometimes a git_path() invocation).\n\n-Peff\n"},{"id":"317623","messageId":"xmqq60hu4orz.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"20170421170700.qyjtrvew54u2epue@sigill.intra.peff.net","subject":"Re: [PATCH 00/15] Handle fopen() errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-24T00:45:04Z","receivedAt":"2017-04-24T00:45:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Apr 21, 2017 at 07:27:20PM +0700, Duy Nguyen wrote:\n>\n>> On Fri, Apr 21, 2017 at 6:52 PM, Junio C Hamano wrote:\n>> > Yes, but (1) we'd need to be careful about --quiet\n>> \n>> Yeah. It's a real pain point for making changes like this. At some\n>> point we should just have a global (maybe multi-level) quiet flag.\n>\n> I don't think it's too bad here. Isn't it just:\n>\n> FILE *fh = quiet ? fopen(x, y) : fopen_or_warn(x, y);\n>\n> It is a little annoying to have to repeat \"x\", though (which is\n> sometimes a git_path() invocation).\n\nSure, but you could do\n\n\tfopen_or_warn(quiet, x, y)\n\nif it is a problem ;-)\n"},{"id":"317936","messageId":"3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org","threadId":"45757","inReplyTo":"20170420112609.26089-8-pclouds@gmail.com","subject":"Re: [PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-26T16:59:20Z","receivedAt":"2017-04-26T16:59:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.04.2017 um 13:26 schrieb Nguyễn Thái Ngọc Duy:\n> @@ -279,8 +283,12 @@ static void read_branches_file(struct remote *remote)\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tFILE *f = fopen(git_path(\"branches/%s\", remote->name), \"r\");\n>\n> -\tif (!f)\n> +\tif (!f) {\n> +\t\tif (errno != ENOENT)\n> +\t\t\twarn_on_inaccessible(git_path(\"branches/%s\",\n> +\t\t\t\t\t\t remote->name));\n> \t\treturn;\n> +\t}\n>\n> \tstrbuf_getline_lf(&buf, f);\n> \tfclose(f);\n\nThis hunk causes a new failure in t5512.10 'confuses pattern as remote \nwhen no remote specified' on Windows:\n\n+++ diff -u exp actual\n--- exp Wed Apr 26 08:16:10 2017\n+++ actual Wed Apr 26 08:16:10 2017\n@@ -1,5 +1,19 @@\n+++ case \"$1\" in\n+++ _test_ok=\n+++ git ls-remote 'refs*master'\n+warning: unable to access '.git/branches/refs*master': Invalid argument\n fatal: 'refs*master' does not appear to be a git repository\n fatal: Could not read from remote repository.\n\n Please make sure you have the correct access rights\n and the repository exists.\n+++ exit_code=128\n\nOn Windows, it is not allowed to pass a file name with an asterisk to \nthe OS. The test case contains this comment:\n\n# We could just as easily have used \"master\"; the \"*\" emphasizes its\n# role as a pattern.\n\nSo, can we replace the name with a simple \"master\", our would this miss \nthe goal of the test case? Should we make it conditional on the MINGW \nprerequisite?\n\n-- Hannes\n\n"},{"id":"318022","messageId":"xmqqwpa6pt0b.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org","subject":"Re: [PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-27T00:57:08Z","receivedAt":"2017-04-27T00:57:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> +++ git ls-remote 'refs*master'\n> +warning: unable to access '.git/branches/refs*master': Invalid argument\n> fatal: 'refs*master' does not appear to be a git repository\n> fatal: Could not read from remote repository.\n>\n> Please make sure you have the correct access rights\n> and the repository exists.\n> +++ exit_code=128\n>\n> On Windows, it is not allowed to pass a file name with an asterisk to\n> the OS. The test case contains this comment:\n>\n> # We could just as easily have used \"master\"; the \"*\" emphasizes its\n> # role as a pattern.\n>\n> So, can we replace the name with a simple \"master\", our would this\n> miss the goal of the test case? Should we make it conditional on the\n> MINGW prerequisite?\n\nI would actually be more worried about the real-life impact of this\nchange. Those who did \"git ls-remote 'refs*master'\" merely got \"it\ndoes not appear to be a git repository\" and that was entirely sensible\nresponse from the command. Can Windows folks live with an extra\nwarning before it, or do they object to see that new warning?\n\n"},{"id":"318042","messageId":"4ef2fe61-720c-6227-11b4-4a6be24a4918@kdbg.org","threadId":"45757","inReplyTo":"xmqqwpa6pt0b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-27T05:07:29Z","receivedAt":"2017-04-27T05:07:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.04.2017 um 02:57 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n>\n>> +++ git ls-remote 'refs*master'\n>> +warning: unable to access '.git/branches/refs*master': Invalid argument\n>> fatal: 'refs*master' does not appear to be a git repository\n>> fatal: Could not read from remote repository.\n>>\n>> Please make sure you have the correct access rights\n>> and the repository exists.\n>> +++ exit_code=128\n>>\n>> On Windows, it is not allowed to pass a file name with an asterisk to\n>> the OS. The test case contains this comment:\n>>\n>> # We could just as easily have used \"master\"; the \"*\" emphasizes its\n>> # role as a pattern.\n>>\n>> So, can we replace the name with a simple \"master\", our would this\n>> miss the goal of the test case? Should we make it conditional on the\n>> MINGW prerequisite?\n>\n> I would actually be more worried about the real-life impact of this\n> change. Those who did \"git ls-remote 'refs*master'\" merely got \"it\n> does not appear to be a git repository\" and that was entirely sensible\n> response from the command. Can Windows folks live with an extra\n> warning before it, or do they object to see that new warning?\n\nI was also worried that the new warning may be irritating. However, I \nexpect that it is seen in practice only after a typo. My gut feeling is \nthat this is bearable, because the reason for the warning should be obvious.\n\nUnless a use-case turns up where the pattern occurs routinely. We'll \nhave to keep the eyes open. Until then it is better to keep the change, IMO.\n\n-- Hannes\n\n"},{"id":"318064","messageId":"CACsJy8C6_9YjgqdU7pnpUwpwreruk8bs-WeKnNtEPT7QL6RjuA@mail.gmail.com","threadId":"45757","inReplyTo":"4ef2fe61-720c-6227-11b4-4a6be24a4918@kdbg.org","subject":"Re: [PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-04-27T09:14:04Z","receivedAt":"2017-04-27T09:14:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 27, 2017 at 12:07 PM, Johannes Sixt wrote:\n> Am 27.04.2017 um 02:57 schrieb Junio C Hamano:\n>>\n>> Johannes Sixt writes:\n>>\n>>> +++ git ls-remote 'refs*master'\n>>> +warning: unable to access '.git/branches/refs*master': Invalid argument\n>>> fatal: 'refs*master' does not appear to be a git repository\n>>> fatal: Could not read from remote repository.\n>>>\n>>> Please make sure you have the correct access rights\n>>> and the repository exists.\n>>> +++ exit_code=128\n>>>\n>>> On Windows, it is not allowed to pass a file name with an asterisk to\n>>> the OS. The test case contains this comment:\n>>>\n>>> # We could just as easily have used \"master\"; the \"*\" emphasizes its\n>>> # role as a pattern.\n>>>\n>>> So, can we replace the name with a simple \"master\", our would this\n>>> miss the goal of the test case? Should we make it conditional on the\n>>> MINGW prerequisite?\n>>\n>>\n>> I would actually be more worried about the real-life impact of this\n>> change. Those who did \"git ls-remote 'refs*master'\" merely got \"it\n>> does not appear to be a git repository\" and that was entirely sensible\n>> response from the command. Can Windows folks live with an extra\n>> warning before it, or do they object to see that new warning?\n>\n>\n> I was also worried that the new warning may be irritating. However, I expect\n> that it is seen in practice only after a typo. My gut feeling is that this\n> is bearable, because the reason for the warning should be obvious.\n>\n> Unless a use-case turns up where the pattern occurs routinely. We'll have to\n> keep the eyes open. Until then it is better to keep the change, IMO.\n\nOK I'll just add MINGW to the test then. Windows folks can improve\nwarn_on_inaccessible() to suppress certain class of error messages if\nneeded.\n-- \nDuy\n"},{"id":"318079","messageId":"5dfc05b2-aff9-2f9f-2671-fa304cd1aa4f@kdbg.org","threadId":"45757","inReplyTo":"CACsJy8C6_9YjgqdU7pnpUwpwreruk8bs-WeKnNtEPT7QL6RjuA@mail.gmail.com","subject":"Re: [PATCH 07/15] remote.c: report error on failure to fopen()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-27T17:49:38Z","receivedAt":"2017-04-27T17:49:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.04.2017 um 11:14 schrieb Duy Nguyen:\n> On Thu, Apr 27, 2017 at 12:07 PM, Johannes Sixt wrote:\n>> Am 27.04.2017 um 02:57 schrieb Junio C Hamano:\n>>>\n>>> Johannes Sixt writes:\n>>>\n>>>> +++ git ls-remote 'refs*master'\n>>>> +warning: unable to access '.git/branches/refs*master': Invalid argument\n>>>> fatal: 'refs*master' does not appear to be a git repository\n>>>> fatal: Could not read from remote repository.\n>>>>\n>>>> Please make sure you have the correct access rights\n>>>> and the repository exists.\n>>>> +++ exit_code=128\n>>>>\n>>>> On Windows, it is not allowed to pass a file name with an asterisk to\n>>>> the OS. The test case contains this comment:\n>>>>\n>>>> # We could just as easily have used \"master\"; the \"*\" emphasizes its\n>>>> # role as a pattern.\n>>>>\n>>>> So, can we replace the name with a simple \"master\", our would this\n>>>> miss the goal of the test case? Should we make it conditional on the\n>>>> MINGW prerequisite?\n\n>\n> OK I'll just add MINGW to the test then. Windows folks can improve\n> warn_on_inaccessible() to suppress certain class of error messages if\n> needed.\n>\n\nI actually meant something like this:\n\n\tif test_have_prereq MINGW\n\tthen\n\t\tdoes_not_exist=master\n\telse\n\t\tdoes_not_exist=\"refs*master\"\n\tfi\n\ttest_must_fail git ls-remote \"$does_not_exist\" >actual 2>&1 &&\n\n-- Hannes\n\n"},{"id":"318617","messageId":"20170503101706.9223-1-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170420112609.26089-1-pclouds@gmail.com","subject":"[PATCH v2 00/21]","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:45Z","receivedAt":"2017-05-03T10:18:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Changes since v1:\n\n - fopen_or_warn() and warn_on_fopen_errors() are introduced. The\n latter's name is probably not great...\n - A new patch (first one) to convert a bunch to using xfopen()\n - Fix test failure on Windows, found by Johannes Sixt\n - Fix the memory leak in log.c, found by Jeff\n\nThere are still a couple of fopen() remained, but I'm getting slow\nagain so let's get these in first and worry about the rest when\nsomebody has time.\n\nNguyễn Thái Ngọc Duy (21):\n Use xfopen() in more places\n clone: use xfopen() instead of fopen()\n config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD\n wrapper.c: add warn_on_fopen_errors()\n wrapper.c: add fopen_or_warn()\n attr.c: use fopen_or_warn()\n ident.c: use fopen_or_warn()\n bisect: report on fopen() error\n blame: report error on open if graft_file is a directory\n log: report errno on failure to fopen() a file\n log: fix memory leak in open_next_file()\n commit.c: report error on failure to fopen() the graft file\n remote.c: report error on failure to fopen()\n rerere.c: report error on failure to fopen()\n rerere.c: report correct errno\n sequencer.c: report error on failure to fopen()\n server-info: report error on failure to fopen()\n wt-status.c: report error on failure to fopen()\n xdiff-interface.c: report errno on failure to stat() or fopen()\n config.c: handle error on failing to fopen()\n t1308: add a test case on open a config directory\n\n attr.c | 7 ++-----\n bisect.c | 7 ++-----\n builtin/am.c | 8 ++------\n builtin/blame.c | 4 +++-\n builtin/clone.c | 2 +-\n builtin/commit.c | 5 +----\n builtin/fast-export.c | 4 +---\n builtin/fsck.c | 3 +--\n builtin/log.c | 11 ++++++++---\n builtin/merge.c | 4 +---\n builtin/pull.c | 3 +--\n commit.c | 4 +++-\n config.c | 5 ++++-\n config.mak.uname | 3 +++\n diff.c | 8 ++------\n dir.c | 3 +--\n fast-import.c | 4 +---\n git-compat-util.h | 3 +++\n ident.c | 8 +++-----\n remote-testsvn.c | 8 ++------\n remote.c | 4 ++--\n rerere.c | 7 ++++---\n sequencer.c | 8 ++++----\n server-info.c | 2 +-\n t/t1308-config-set.sh | 13 ++++++++++++-\n t/t5512-ls-remote.sh | 13 ++++++++++---\n wrapper.c | 21 +++++++++++++++++++++\n wt-status.c | 3 ++-\n xdiff-interface.c | 4 ++--\n 29 files changed, 103 insertions(+), 76 deletions(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318618","messageId":"20170503101706.9223-2-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 01/21] Use xfopen() in more places","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:46Z","receivedAt":"2017-05-03T10:18:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"xfopen()\n\n - provides error details\n - explains error on reading, or writing, or whatever operation\n - has l10n support\n - prints file name in the error\n\nSome of these are missing in the places that are replaced with xfopen(),\nwhich is a clear win. In some other places, it's just less code (not as\nclearly a win as the previous case but still is).\n\nThe only slight regresssion is in remote-testsvn, where we don't report\nthe file class (marks files) in the error messages anymore. But since\nthis is a _test_ svn remote transport, I'm not too concerned.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n bisect.c | 5 +----\n builtin/am.c | 8 ++------\n builtin/commit.c | 5 +----\n builtin/fast-export.c | 4 +---\n builtin/fsck.c | 3 +--\n builtin/merge.c | 4 +---\n builtin/pull.c | 3 +--\n diff.c | 8 ++------\n fast-import.c | 4 +---\n remote-testsvn.c | 8 ++------\n 10 files changed, 13 insertions(+), 39 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 08c9fb7266..469a3e9061 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -438,10 +438,7 @@ static void read_bisect_paths(struct argv_array *array)\n {\n \tstruct strbuf str = STRBUF_INIT;\n \tconst char *filename = git_path_bisect_names();\n-\tFILE *fp = fopen(filename, \"r\");\n-\n-\tif (!fp)\n-\t\tdie_errno(_(\"Could not open file '%s'\"), filename);\n+\tFILE *fp = xfopen(filename, \"r\");\n \n \twhile (strbuf_getline_lf(&str, fp) != EOF) {\n \t\tstrbuf_trim(&str);\ndiff --git a/builtin/am.c b/builtin/am.c\nindex a95dd8b4e6..f5dac7783e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1275,12 +1275,8 @@ static int parse_mail(struct am_state *state, const char *mail)\n \t\tdie(\"BUG: invalid value for state->scissors\");\n \t}\n \n-\tmi.input = fopen(mail, \"r\");\n-\tif (!mi.input)\n-\t\tdie(\"could not open input\");\n-\tmi.output = fopen(am_path(state, \"info\"), \"w\");\n-\tif (!mi.output)\n-\t\tdie(\"could not open output 'info'\");\n+\tmi.input = xfopen(mail, \"r\");\n+\tmi.output = xfopen(am_path(state, \"info\"), \"w\");\n \tif (mailinfo(&mi, am_path(state, \"msg\"), am_path(state, \"patch\")))\n \t\tdie(\"could not parse patch\");\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1d805f5da8..eda0d32311 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1695,10 +1695,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n \t\tpptr = commit_list_append(current_head, pptr);\n-\t\tfp = fopen(git_path_merge_head(), \"r\");\n-\t\tif (fp == NULL)\n-\t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n-\t\t\t\t git_path_merge_head());\n+\t\tfp = xfopen(git_path_merge_head(), \"r\");\n \t\twhile (strbuf_getline_lf(&m, fp) != EOF) {\n \t\t\tstruct commit *parent;\n \ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex e0220630d0..128b99e6da 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -905,9 +905,7 @@ static void export_marks(char *file)\n static void import_marks(char *input_file)\n {\n \tchar line[512];\n-\tFILE *f = fopen(input_file, \"r\");\n-\tif (!f)\n-\t\tdie_errno(\"cannot read '%s'\", input_file);\n+\tFILE *f = xfopen(input_file, \"r\");\n \n \twhile (fgets(line, sizeof(line), f)) {\n \t\tuint32_t mark;\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex b5e13a4556..00beaaa4e6 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -280,8 +280,7 @@ static void check_unreachable_object(struct object *obj)\n \t\t\t\tfree(filename);\n \t\t\t\treturn;\n \t\t\t}\n-\t\t\tif (!(f = fopen(filename, \"w\")))\n-\t\t\t\tdie_errno(\"Could not open '%s'\", filename);\n+\t\t\tf = xfopen(filename, \"w\");\n \t\t\tif (obj->type == OBJ_BLOB) {\n \t\t\t\tif (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1))\n \t\t\t\t\tdie_errno(\"Could not write '%s'\", filename);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 703827f006..65a1501858 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -839,9 +839,7 @@ static int suggest_conflicts(void)\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \n \tfilename = git_path_merge_msg();\n-\tfp = fopen(filename, \"a\");\n-\tif (!fp)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n+\tfp = xfopen(filename, \"a\");\n \n \tappend_conflicts_hint(&msgbuf);\n \tfputs(msgbuf.buf, fp);\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex dd1a4a94e4..589c25becf 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -337,8 +337,7 @@ static void get_merge_heads(struct oid_array *merge_heads)\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n \n-\tif (!(fp = fopen(filename, \"r\")))\n-\t\tdie_errno(_(\"could not open '%s' for reading\"), filename);\n+\tfp = xfopen(filename, \"r\");\n \twhile (strbuf_getline_lf(&sb, fp) != EOF) {\n \t\tif (get_oid_hex(sb.buf, &oid))\n \t\t\tcontinue; /* invalid line: does not start with SHA1 */\ndiff --git a/diff.c b/diff.c\nindex 11eef1c85d..b6597ce568 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4071,9 +4071,7 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_OPT_CLR(options, FUNCCONTEXT);\n \telse if ((argcount = parse_long_opt(\"output\", av, &optarg))) {\n \t\tchar *path = prefix_filename(prefix, optarg);\n-\t\toptions->file = fopen(path, \"w\");\n-\t\tif (!options->file)\n-\t\t\tdie_errno(\"Could not open '%s'\", path);\n+\t\toptions->file = xfopen(path, \"w\");\n \t\toptions->close_file = 1;\n \t\tif (options->use_color != GIT_COLOR_ALWAYS)\n \t\t\toptions->use_color = GIT_COLOR_NEVER;\n@@ -4807,9 +4805,7 @@ void diff_flush(struct diff_options *options)\n \t\t */\n \t\tif (options->close_file)\n \t\t\tfclose(options->file);\n-\t\toptions->file = fopen(\"/dev/null\", \"w\");\n-\t\tif (!options->file)\n-\t\t\tdie_errno(\"Could not open /dev/null\");\n+\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n \t\toptions->close_file = 1;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\ndiff --git a/fast-import.c b/fast-import.c\nindex cf58f875b8..420b3a00d3 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3274,9 +3274,7 @@ static void option_export_pack_edges(const char *edges)\n {\n \tif (pack_edges)\n \t\tfclose(pack_edges);\n-\tpack_edges = fopen(edges, \"a\");\n-\tif (!pack_edges)\n-\t\tdie_errno(\"Cannot open '%s'\", edges);\n+\tpack_edges = xfopen(edges, \"a\");\n }\n \n static int parse_one_option(const char *option)\ndiff --git a/remote-testsvn.c b/remote-testsvn.c\nindex f87bf851ba..50404ef343 100644\n--- a/remote-testsvn.c\n+++ b/remote-testsvn.c\n@@ -124,10 +124,8 @@ static int note2mark_cb(const unsigned char *object_sha1,\n static void regenerate_marks(void)\n {\n \tint ret;\n-\tFILE *marksfile = fopen(marksfilename, \"w+\");\n+\tFILE *marksfile = xfopen(marksfilename, \"w+\");\n \n-\tif (!marksfile)\n-\t\tdie_errno(\"Couldn't create mark file %s.\", marksfilename);\n \tret = for_each_note(NULL, 0, note2mark_cb, marksfile);\n \tif (ret)\n \t\tdie(\"Regeneration of marks failed, returned %d.\", ret);\n@@ -148,9 +146,7 @@ static void check_or_regenerate_marks(int latestrev)\n \tmarksfile = fopen(marksfilename, \"r\");\n \tif (!marksfile) {\n \t\tregenerate_marks();\n-\t\tmarksfile = fopen(marksfilename, \"r\");\n-\t\tif (!marksfile)\n-\t\t\tdie_errno(\"cannot read marks file %s!\", marksfilename);\n+\t\tmarksfile = xfopen(marksfilename, \"r\");\n \t\tfclose(marksfile);\n \t} else {\n \t\tstrbuf_addf(&sb, \":%d \", latestrev);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318619","messageId":"20170503101706.9223-4-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 03/21] config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:48Z","receivedAt":"2017-05-03T10:18:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This variable is added [1] with the assumption that on a sane system,\nfopen(, \"r\") should return NULL. Linux and FreeBSD do not meet this\nexpectation while at least Windows and AIX do. Let's make sure they\nbehave the same way.\n\nI only tested one version on Linux (4.7.0 with glibc 2.22) and\nFreeBSD (11.0) but since GNU/kFreeBSD is fbsd kernel with gnu userspace,\nI'm pretty sure it shares the same problem.\n\n[1] cba22528fa (Add compat/fopen.c which returns NULL on attempt to open\n directory - 2008-02-08)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n config.mak.uname | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 399fe19271..a25ffddb3e 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -36,6 +36,7 @@ ifeq ($(uname_S),Linux)\n \tNEEDS_LIBRT = YesPlease\n \tHAVE_GETDELIM = YesPlease\n \tSANE_TEXT_GREP=-a\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n@@ -43,6 +44,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_PATHS_H = YesPlease\n \tDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n \tLIBC_CONTAINS_LIBINTL = YesPlease\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),UnixWare)\n \tCC = cc\n@@ -201,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)\n \tGMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes\n \tHAVE_BSD_SYSCTL = YesPlease\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318620","messageId":"20170503101706.9223-3-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 02/21] clone: use xfopen() instead of fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:47Z","receivedAt":"2017-05-03T10:18:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This code uses the result FILE pointer right away without the NULL\ncheck. Let's use xfopen() and die if we could not open the file. That's\nstill better than crashing,\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex de85b85254..dde4fe73af 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -357,7 +357,7 @@ static void copy_alternates(struct strbuf *src, struct strbuf *dst,\n \t * to turn entries with paths relative to the original\n \t * absolute, so that they can be used in the new repository.\n \t */\n-\tFILE *in = fopen(src->buf, \"r\");\n+\tFILE *in = xfopen(src->buf, \"r\");\n \tstruct strbuf line = STRBUF_INIT;\n \n \twhile (strbuf_getline(&line, in) != EOF) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318621","messageId":"20170503101706.9223-5-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:49Z","receivedAt":"2017-05-03T10:18:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n dir.c | 3 +--\n git-compat-util.h | 2 ++\n wrapper.c | 10 ++++++++++\n 3 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex f451bfa48c..8218a24962 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,\n \n \tfd = open(fname, O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st) < 0) {\n-\t\tif (errno != ENOENT)\n-\t\t\twarn_on_inaccessible(fname);\n+\t\twarn_on_fopen_errors(fname);\n \t\tif (0 <= fd)\n \t\t\tclose(fd);\n \t\tif (!check_index ||\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex bd04564a69..c5b59c23e8 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1099,6 +1099,8 @@ int access_or_die(const char *path, int mode, unsigned flag);\n \n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\n+/* Warn on an inaccessible file if errno indicates this is an error */\n+int warn_on_fopen_errors(const char *path);\n \n #ifdef GMTIME_UNRELIABLE_ERRORS\n struct tm *git_gmtime(const time_t *);\ndiff --git a/wrapper.c b/wrapper.c\nindex d837417709..20c25e7e65 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)\n \treturn ret;\n }\n \n+int warn_on_fopen_errors(const char *path)\n+{\n+\tif (errno != ENOENT && errno != ENOTDIR) {\n+\t\twarn_on_inaccessible(path);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n int xmkstemp(char *template)\n {\n \tint fd;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318622","messageId":"20170503101706.9223-6-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 05/21] wrapper.c: add fopen_or_warn()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:50Z","receivedAt":"2017-05-03T10:18:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When fopen() returns NULL, it could be because the given path does not\nexist, but it could also be some other errors and the caller has to\ncheck. Add a wrapper so we don't have to repeat the same error check\neverywhere.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n git-compat-util.h | 1 +\n wrapper.c | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c5b59c23e8..c1647b01e0 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -800,6 +800,7 @@ extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\n extern char *xgetcwd(void);\n extern FILE *fopen_for_writing(const char *path);\n+extern FILE *fopen_or_warn(const char *path, const char *mode);\n \n #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))\n #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))\ndiff --git a/wrapper.c b/wrapper.c\nindex 20c25e7e65..6e513c904a 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -428,6 +428,17 @@ int warn_on_fopen_errors(const char *path)\n \treturn 0;\n }\n \n+FILE *fopen_or_warn(const char *path, const char *mode)\n+{\n+\tFILE *fp = fopen(path, mode);\n+\n+\tif (fp)\n+\t\treturn fp;\n+\n+\twarn_on_fopen_errors(path);\n+\treturn NULL;\n+}\n+\n int xmkstemp(char *template)\n {\n \tint fd;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318623","messageId":"20170503101706.9223-7-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 06/21] attr.c: use fopen_or_warn()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:51Z","receivedAt":"2017-05-03T10:18:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n attr.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7e2134471c..821203e2a9 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -720,16 +720,13 @@ void git_attr_set_direction(enum git_attr_direction new_direction,\n \n static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n {\n-\tFILE *fp = fopen(path, \"r\");\n+\tFILE *fp = fopen_or_warn(path, \"r\");\n \tstruct attr_stack *res;\n \tchar buf[2048];\n \tint lineno = 0;\n \n-\tif (!fp) {\n-\t\tif (errno != ENOENT && errno != ENOTDIR)\n-\t\t\twarn_on_inaccessible(path);\n+\tif (!fp)\n \t\treturn NULL;\n-\t}\n \tres = xcalloc(1, sizeof(*res));\n \twhile (fgets(buf, sizeof(buf), fp)) {\n \t\tchar *bufp = buf;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318624","messageId":"20170503101706.9223-8-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 07/21] ident.c: use fopen_or_warn()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:52Z","receivedAt":"2017-05-03T10:18:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n ident.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex bea871c8e0..91c7609055 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -72,12 +72,10 @@ static int add_mailname_host(struct strbuf *buf)\n \tFILE *mailname;\n \tstruct strbuf mailnamebuf = STRBUF_INIT;\n \n-\tmailname = fopen(\"/etc/mailname\", \"r\");\n-\tif (!mailname) {\n-\t\tif (errno != ENOENT)\n-\t\t\twarning_errno(\"cannot open /etc/mailname\");\n+\tmailname = fopen_or_warn(\"/etc/mailname\", \"r\");\n+\tif (!mailname)\n \t\treturn -1;\n-\t}\n+\n \tif (strbuf_getline(&mailnamebuf, mailname) == EOF) {\n \t\tif (ferror(mailname))\n \t\t\twarning_errno(\"cannot read /etc/mailname\");\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318625","messageId":"20170503101706.9223-9-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 08/21] bisect: report on fopen() error","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:53Z","receivedAt":"2017-05-03T10:18:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The main thing to catch here is when fopen() is called on a\ndirectory. It's safe even without this change because a few lines\nearlier we do check if \"filename\" is a regular file.\n\nRegardless, let's stay on the safe side in case somebody changes those\nlines. Unconditionally printing to stderr by warn_on_inaccessible()\nshould be fine, because the caller does unconditional fprintf(stderr,\ntoo, no checking for quiet option or anything.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n bisect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 469a3e9061..bb28bf63b2 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -666,7 +666,7 @@ static int is_expected_rev(const struct object_id *oid)\n \tif (stat(filename, &st) || !S_ISREG(st.st_mode))\n \t\treturn 0;\n \n-\tfp = fopen(filename, \"r\");\n+\tfp = fopen_or_warn(filename, \"r\");\n \tif (!fp)\n \t\treturn 0;\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318626","messageId":"20170503101706.9223-10-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 09/21] blame: report error on open if graft_file is a directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:54Z","receivedAt":"2017-05-03T10:18:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/blame.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 07506a3e45..1648b396dc 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)\n */\n static int read_ancestry(const char *graft_file)\n {\n-\tFILE *fp = fopen(graft_file, \"r\");\n+\tFILE *fp = fopen_or_warn(graft_file, \"r\");\n \tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (!fp)\n \t\treturn -1;\n+\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318627","messageId":"20170503101706.9223-11-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 10/21] log: report errno on failure to fopen() a file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:55Z","receivedAt":"2017-05-03T10:19:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1ed..26d6a3cf14 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,\n \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n \n \tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL)\n-\t\treturn error(_(\"Cannot open patch file %s\"), filename.buf);\n+\t\treturn error_errno(_(\"Cannot open patch file %s\"),\n+\t\t\t\t filename.buf);\n \n \tstrbuf_release(&filename);\n \treturn 0;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318628","messageId":"20170503101706.9223-12-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 11/21] log: fix memory leak in open_next_file()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:56Z","receivedAt":"2017-05-03T10:19:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Noticed-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 26d6a3cf14..f075838df9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -842,8 +842,10 @@ static int open_next_file(struct commit *commit, const char *subject,\n \tif (output_directory) {\n \t\tstrbuf_addstr(&filename, output_directory);\n \t\tif (filename.len >=\n-\t\t PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len)\n+\t\t PATH_MAX - FORMAT_PATCH_NAME_MAX - suffix_len) {\n+\t\t\tstrbuf_release(&filename);\n \t\t\treturn error(_(\"name of output directory is too long\"));\n+\t\t}\n \t\tstrbuf_complete(&filename, '/');\n \t}\n \n@@ -857,9 +859,11 @@ static int open_next_file(struct commit *commit, const char *subject,\n \tif (!quiet)\n \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n \n-\tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL)\n-\t\treturn error_errno(_(\"Cannot open patch file %s\"),\n-\t\t\t\t filename.buf);\n+\tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL) {\n+\t\terror_errno(_(\"Cannot open patch file %s\"), filename.buf);\n+\t\tstrbuf_release(&filename);\n+\t\treturn -1;\n+\t}\n \n \tstrbuf_release(&filename);\n \treturn 0;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318629","messageId":"20170503101706.9223-13-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:57Z","receivedAt":"2017-05-03T10:19:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Suppressing the error (because the command requires --quiet) is not a\nconcern because we already call error() just a couple lines down.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n commit.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 73c78c2b80..075607a6da 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -167,10 +167,12 @@ struct commit_graft *read_graft_line(char *buf, int len)\n \n static int read_graft_file(const char *graft_file)\n {\n-\tFILE *fp = fopen(graft_file, \"r\");\n+\tFILE *fp = fopen_or_warn(graft_file, \"r\");\n \tstruct strbuf buf = STRBUF_INIT;\n+\n \tif (!fp)\n \t\treturn -1;\n+\n \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n \t\t/* The format is just \"Commit Parent1 Parent2 ...\\n\" */\n \t\tstruct commit_graft *graft = read_graft_line(buf.buf, buf.len);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318630","messageId":"20170503101706.9223-14-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 13/21] remote.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:58Z","receivedAt":"2017-05-03T10:19:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There's plenty of error() in this code to safely assume --quiet is not a\nconcern.\n\nt5512 needs update because if we check the path 'refs*master' (i.e. the\nasterisk is part of the path) then we'll get an EINVAL error.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n remote.c | 4 ++--\n t/t5512-ls-remote.sh | 13 ++++++++++---\n 2 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 801137c72e..1f2453d0f6 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s)\n static void read_remotes_file(struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n+\tFILE *f = fopen_or_warn(git_path(\"remotes/%s\", remote->name), \"r\");\n \n \tif (!f)\n \t\treturn;\n@@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote)\n {\n \tchar *frag;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *f = fopen(git_path(\"branches/%s\", remote->name), \"r\");\n+\tFILE *f = fopen_or_warn(git_path(\"branches/%s\", remote->name), \"r\");\n \n \tif (!f)\n \t\treturn;\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 94fc9be9ce..02106c9226 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -85,8 +85,15 @@ test_expect_success 'use branch..remote if possible' '\n '\n \n test_expect_success 'confuses pattern as remote when no remote specified' '\n-\tcat >exp <<-\\EOF &&\n-\tfatal: '\\''refs*master'\\'' does not appear to be a git repository\n+\tif test_have_prereq MINGW\n+\tthen\n+\t\t# Windows does not like asterisks in pathname\n+\t\tdoes_not_exist=master\n+\telse\n+\t\tdoes_not_exist=\"refs*master\"\n+\tfi &&\n+\tcat >exp <<-EOF &&\n+\tfatal: '\\''$does_not_exist'\\'' does not appear to be a git repository\n \tfatal: Could not read from remote repository.\n \n \tPlease make sure you have the correct access rights\n@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' '\n \t# fetch .\n \t# We could just as easily have used \"master\"; the \"*\" emphasizes its\n \t# role as a pattern.\n-\ttest_must_fail git ls-remote refs*master >actual 2>&1 &&\n+\ttest_must_fail git ls-remote \"$does_not_exist\" >actual 2>&1 &&\n \ttest_i18ncmp exp actual\n '\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318631","messageId":"20170503101706.9223-15-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 14/21] rerere.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:16:59Z","receivedAt":"2017-05-03T10:19:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n rerere.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 3bd55caf3b..971bfedfb2 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1)\n static void read_rr(struct string_list *rr)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *in = fopen(git_path_merge_rr(), \"r\");\n+\tFILE *in = fopen_or_warn(git_path_merge_rr(), \"r\");\n \n \tif (!in)\n \t\treturn;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318632","messageId":"20170503101706.9223-16-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 15/21] rerere.c: report correct errno","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:00Z","receivedAt":"2017-05-03T10:19:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the first case, we should have reported the reason fopen() fails. In\nthe second case, fclose() might change errno but we want to report\nfopen() failure.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n rerere.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 971bfedfb2..c26c29f87a 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output\n \tio.input = fopen(path, \"r\");\n \tio.io.wrerror = 0;\n \tif (!io.input)\n-\t\treturn error(\"Could not open %s\", path);\n+\t\treturn error_errno(\"Could not open %s\", path);\n \n \tif (output) {\n \t\tio.io.output = fopen(output, \"w\");\n \t\tif (!io.io.output) {\n+\t\t\terror_errno(\"Could not write %s\", output);\n \t\t\tfclose(io.input);\n-\t\t\treturn error(\"Could not write %s\", output);\n+\t\t\treturn -1;\n \t\t}\n \t}\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318633","messageId":"20170503101706.9223-18-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 17/21] server-info: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:02Z","receivedAt":"2017-05-03T10:19:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n server-info.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex f6c1a3dfb0..e01ac154a8 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile)\n \tchar line[1000];\n \tint old_cnt = 0;\n \n-\tfp = fopen(infofile, \"r\");\n+\tfp = fopen_or_warn(infofile, \"r\");\n \tif (!fp)\n \t\treturn 1; /* nonexistent is not an error. */\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318634","messageId":"20170503101706.9223-17-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 16/21] sequencer.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:01Z","receivedAt":"2017-05-03T10:19:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n sequencer.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 10c3b4ff81..11b5c7c114 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) {\n \tFILE *out;\n \n \tif (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&\n-\t\t\t!get_sha1(\"HEAD\", newsha1) &&\n-\t\t\t(out = fopen(rebase_path_rewritten_list(), \"a\"))) {\n+\t !get_sha1(\"HEAD\", newsha1) &&\n+\t (out = fopen_or_warn(rebase_path_rewritten_list(), \"a\"))) {\n \t\tchar *bol = buf.buf, *eol;\n \n \t\twhile (*bol) {\n@@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) {\n \n static void record_in_rewritten(struct object_id *oid,\n \t\tenum todo_command next_command) {\n-\tFILE *out = fopen(rebase_path_rewritten_pending(), \"a\");\n+\tFILE *out = fopen_or_warn(rebase_path_rewritten_pending(), \"a\");\n \n \tif (!out)\n \t\treturn;\n@@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list,\n \n \tif (is_rebase_i(opts)) {\n \t\tstruct todo_list done = TODO_LIST_INIT;\n-\t\tFILE *f = fopen(rebase_path_msgtotal(), \"w\");\n+\t\tFILE *f = fopen_or_warn(rebase_path_msgtotal(), \"w\");\n \n \t\tif (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&\n \t\t\t\t!parse_insn_buffer(done.buf.buf, &done))\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318635","messageId":"20170503101706.9223-19-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 18/21] wt-status.c: report error on failure to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:03Z","receivedAt":"2017-05-03T10:20:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n wt-status.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 0375484962..cdf9f5eed2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s,\n static char *read_line_from_git_path(const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tFILE *fp = fopen(git_path(\"%s\", filename), \"r\");\n+\tFILE *fp = fopen_or_warn(git_path(\"%s\", filename), \"r\");\n+\n \tif (!fp) {\n \t\tstrbuf_release(&buf);\n \t\treturn NULL;\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318636","messageId":"20170503101706.9223-21-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 20/21] config.c: handle error on failing to fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:05Z","receivedAt":"2017-05-03T10:20:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In the first case, we already correctly return -1 if fopen() fails to\nopen. But we should report something so people know what's wrong.\n\nIn the second case, config_file == NULL does not necessarily mean \"no\nconfig file\". Bail out if needed.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n config.c | 5 ++++-\n t/t1308-config-set.sh | 4 +++-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b4a3205da3..e54d99d519 100644\n--- a/config.c\n+++ b/config.c\n@@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tint ret = -1;\n \tFILE *f;\n \n-\tf = fopen(filename, \"r\");\n+\tf = fopen_or_warn(filename, \"r\");\n \tif (f) {\n \t\tflockfile(f);\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);\n@@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t}\n \n \tif (!(config_file = fopen(config_filename, \"rb\"))) {\n+\t\tret = warn_on_fopen_errors(config_filename);\n+\t\tif (ret)\n+\t\t\tgoto out;\n \t\t/* no config file means nothing to rename, no error */\n \t\tgoto commit_and_out;\n \t}\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex ff50960cca..13e95561f4 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '\n \tchmod -r .git/config &&\n \ttest_when_finished \"chmod +r .git/config\" &&\n \techo \"Error (-1) reading configuration file .git/config.\" >expect &&\n-\ttest_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar .git/config 2>output &&\n+\tgrep \"^warning:\" output &&\n+\tgrep \"^Error\" output >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318637","messageId":"20170503101706.9223-22-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 21/21] t1308: add a test case on open a config directory","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:06Z","receivedAt":"2017-05-03T10:20:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We don't support config-as-a-directory (maybe someday we will?). Make\nsure we consistently fail in this case, which should happen on platforms\nwhere fopen() returns non-NULL if FREAD_READS_DIRECTORIES is\ndefined.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n t/t1308-config-set.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 13e95561f4..e495a61616 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'proper error on directory \"files\"' '\n+\techo \"Error (-1) reading configuration file a-directory.\" >expect &&\n+\tmkdir a-directory &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar a-directory 2>output &&\n+\tgrep \"^warning:\" output &&\n+\tgrep \"^Error\" output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '\n \tchmod -r .git/config &&\n \ttest_when_finished \"chmod +r .git/config\" &&\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318638","messageId":"20170503101706.9223-20-pclouds@gmail.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"[PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-03T10:17:04Z","receivedAt":"2017-05-03T10:20:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n xdiff-interface.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 060038c2d6..d3f78ca2a7 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)\n \tsize_t sz;\n \n \tif (stat(filename, &st))\n-\t\treturn error(\"Could not stat %s\", filename);\n+\t\treturn error_errno(\"Could not stat %s\", filename);\n \tif ((f = fopen(filename, \"rb\")) == NULL)\n-\t\treturn error(\"Could not open %s\", filename);\n+\t\treturn error_errno(\"Could not open %s\", filename);\n \tsz = xsize_t(st.st_size);\n \tptr->ptr = xmalloc(sz ? sz : 1);\n \tif (sz && fread(ptr->ptr, sz, 1, f) != 1) {\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"318653","messageId":"alpine.DEB.2.20.1705031701120.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-3-pclouds@gmail.com","subject":"Re: [PATCH v2 02/21] clone: use xfopen() instead of fopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:02:36Z","receivedAt":"2017-05-03T15:02:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> This code uses the result FILE pointer right away without the NULL\n> check. Let's use xfopen() and die if we could not open the file. That's\n> still better than crashing,\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nWhile funny to read, maybe it would be better to spell it out clearly:\n\n\tcopy_alternates() called fopen() without handling errors. By\n\tswitching to xfopen(), this bug is fixed.\n\nCiao,\nDscho"},{"id":"318654","messageId":"alpine.DEB.2.20.1705031704400.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-5-pclouds@gmail.com","subject":"Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:07:46Z","receivedAt":"2017-05-03T15:08:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nThis commit message is a bit empty. How about:\n\n\tIn many places, Git warns about an inaccessible file after a\n\tfopen() failed. To discern these cases from other cases where we\n\twant to warn about inaccessible files, introduce a new helper\n\tspecifically to test whether fopen() failed because the current\n\tuser lacks the permission to open file in question.\n\n> diff --git a/dir.c b/dir.c\n> index f451bfa48c..8218a24962 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,\n> \n> \tfd = open(fname, O_RDONLY);\n> \tif (fd < 0 || fstat(fd, &st) < 0) {\n> -\t\tif (errno != ENOENT)\n> -\t\t\twarn_on_inaccessible(fname);\n> +\t\twarn_on_fopen_errors(fname);\n> \t\tif (0 <= fd)\n> \t\t\tclose(fd);\n\nI see you already converted one location. Why not simply fold all of them\ninto this patch? It's not like it would be easier to review those changes\nif you separate patches addressing this class of problems.\n\nCiao,\nDscho"},{"id":"318655","messageId":"alpine.DEB.2.20.1705031708350.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-7-pclouds@gmail.com","subject":"Re: [PATCH v2 06/21] attr.c: use fopen_or_warn()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:09:51Z","receivedAt":"2017-05-03T15:10:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nAs commented about warn_on_fopen_errors(), I do not see a benefit in\nseparating the introduction of this small function from the individual\nfiles' changes to use it. Let's bring it all together.\n\nCiao,\nDscho"},{"id":"318657","messageId":"alpine.DEB.2.20.1705031710500.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-9-pclouds@gmail.com","subject":"Re: [PATCH v2 08/21] bisect: report on fopen() error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:12:03Z","receivedAt":"2017-05-03T15:12:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> The main thing to catch here is when fopen() is called on a\n> directory. It's safe even without this change because a few lines\n> earlier we do check if \"filename\" is a regular file.\n> \n> Regardless, let's stay on the safe side in case somebody changes those\n> lines. Unconditionally printing to stderr by warn_on_inaccessible()\n> should be fine, because the caller does unconditional fprintf(stderr,\n> too, no checking for quiet option or anything.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nI appreciate that you mention specifically the use case that triggered my\ncomplaint that in turn prodded you into starting working on this.\n\nHowever, I think this should be a mere side note in a single commit that\nintroduces and uses fopen_or_warn().\n\nCiao,\nDscho"},{"id":"318658","messageId":"alpine.DEB.2.20.1705031712530.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-10-pclouds@gmail.com","subject":"Re: [PATCH v2 09/21] blame: report error on open if graft_file is a directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:15:13Z","receivedAt":"2017-05-03T15:15:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/blame.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 07506a3e45..1648b396dc 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)\n> */\n> static int read_ancestry(const char *graft_file)\n> {\n> -\tFILE *fp = fopen(graft_file, \"r\");\n> +\tFILE *fp = fopen_or_warn(graft_file, \"r\");\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\n> \tif (!fp)\n> \t\treturn -1;\n> +\n> \twhile (!strbuf_getwholeline(&buf, fp, '\\n')) {\n\nThis is an excellent example to demonstrate why folding all conversions to\nfopen_or_warn() into the same patch as introducing that function makes\nsense: I had to close this mail, find the mail with the patch introducing\nthe function, verify that it returns NULL as before, close that mail, and\ncontinue this reply.\n\nOh, and I do not think it is a good idea to introduce style fixes in the\nmiddle of such a large patch series. It's a bit distracting from the real\nmeat here.\n\nCiao,\nDscho"},{"id":"318659","messageId":"alpine.DEB.2.20.1705031717210.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-11-pclouds@gmail.com","subject":"Re: [PATCH v2 10/21] log: report errno on failure to fopen() a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:19:44Z","receivedAt":"2017-05-03T15:20:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nI do think that this merits a one-line explanation why you do not use\nfopen_or_warn() here.\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1ed..26d6a3cf14 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const char *subject,\n> \t\tprintf(\"%s\\n\", filename.buf + outdir_offset);\n> \n> \tif ((rev->diffopt.file = fopen(filename.buf, \"w\")) == NULL)\n> -\t\treturn error(_(\"Cannot open patch file %s\"), filename.buf);\n> +\t\treturn error_errno(_(\"Cannot open patch file %s\"),\n> +\t\t\t\t filename.buf);\n> \n> \tstrbuf_release(&filename);\n\nThe patches before and after this one all convert fopen() to\nfopen_or_warn(). If you *must* separate those into individual patches\n(which just makes this patch series unnecessarily bloated, if you ask me),\nyou should at least group them a bit better so that the patch series tells\nmore of a consistent story rather than jumping back and forth like a\nhyperactive four-year-old telling you about her latest adventure.\n\nCiao,\nDscho"},{"id":"318660","messageId":"alpine.DEB.2.20.1705031720000.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-14-pclouds@gmail.com","subject":"Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:22:04Z","receivedAt":"2017-05-03T15:22:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> There's plenty of error() in this code to safely assume --quiet is not a\n> concern.\n> \n> t5512 needs update because if we check the path 'refs*master' (i.e. the\n> asterisk is part of the path) then we'll get an EINVAL error.\n\nSo the first change in this patch unmasks a bug that is fixed by the\nsecond patch?\n\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 94fc9be9ce..02106c9226 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -85,8 +85,15 @@ test_expect_success 'use branch..remote if possible' '\n> '\n> \n> test_expect_success 'confuses pattern as remote when no remote specified' '\n> -\tcat >exp <<-\\EOF &&\n> -\tfatal: '\\''refs*master'\\'' does not appear to be a git repository\n> +\tif test_have_prereq MINGW\n> +\tthen\n> +\t\t# Windows does not like asterisks in pathname\n> +\t\tdoes_not_exist=master\n> +\telse\n> +\t\tdoes_not_exist=\"refs*master\"\n> +\tfi &&\n> +\tcat >exp <<-EOF &&\n> +\tfatal: '\\''$does_not_exist'\\'' does not appear to be a git repository\n> \tfatal: Could not read from remote repository.\n> \n> \tPlease make sure you have the correct access rights\n> @@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no remote specified' '\n> \t# fetch .\n> \t# We could just as easily have used \"master\"; the \"*\" emphasizes its\n> \t# role as a pattern.\n> -\ttest_must_fail git ls-remote refs*master >actual 2>&1 &&\n> +\ttest_must_fail git ls-remote \"$does_not_exist\" >actual 2>&1 &&\n> \ttest_i18ncmp exp actual\n> '\n\nSure enough. This totally looks like it needs to be a preparatory bug fix.\nPlease separate it out and make sure that it comes before the\nfopen_or_warn() change in the patch series.\n\nCiao,\nDscho"},{"id":"318661","messageId":"alpine.DEB.2.20.1705031722420.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-16-pclouds@gmail.com","subject":"Re: [PATCH v2 15/21] rerere.c: report correct errno","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:24:41Z","receivedAt":"2017-05-03T15:25:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/rerere.c b/rerere.c\n> index 971bfedfb2..c26c29f87a 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char *sha1, const char *output\n> \tio.input = fopen(path, \"r\");\n> \tio.io.wrerror = 0;\n> \tif (!io.input)\n> -\t\treturn error(\"Could not open %s\", path);\n> +\t\treturn error_errno(\"Could not open %s\", path);\n\nIMO the error() -> error_errno() changes should all be part of the same\ncommit, as they probably share the explanation why fopen_or_warn() is not\nappropriate here.\n\n> \tif (output) {\n> \t\tio.io.output = fopen(output, \"w\");\n> \t\tif (!io.io.output) {\n> +\t\t\terror_errno(\"Could not write %s\", output);\n> \t\t\tfclose(io.input);\n> -\t\t\treturn error(\"Could not write %s\", output);\n> +\t\t\treturn -1;\n> \t\t}\n\nThis one is logically different from the change above, as it not only\ncannot be replaced by fopen_or_warn(), but also requires the reordering\ndue to the different nature of the change.\n\nCiao,\nDscho"},{"id":"318662","messageId":"alpine.DEB.2.20.1705031727410.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-21-pclouds@gmail.com","subject":"Re: [PATCH v2 20/21] config.c: handle error on failing to fopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:29:10Z","receivedAt":"2017-05-03T15:29:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> In the first case, we already correctly return -1 if fopen() fails to\n> open. But we should report something so people know what's wrong.\n> \n> In the second case, config_file == NULL does not necessarily mean \"no\n> config file\". Bail out if needed.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nAgain, it seems that your patch series tries to cut at the file boundary,\nnot at the \"logically the same change\" boundary, making it a bit more\ncumbersome than necessary to follow this patch series.\n\nI highly recommend squashing the first change into the big fopen() ->\nfopen_or_warn() patch I hinted at earlier, and let the second change (with\nthe accompanying test case) stand on its own.\n\nCiao,\nDscho"},{"id":"318663","messageId":"alpine.DEB.2.20.1705031730040.3480@virtualbox","threadId":"45757","inReplyTo":"20170503101706.9223-22-pclouds@gmail.com","subject":"Re: [PATCH v2 21/21] t1308: add a test case on open a config directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-03T15:33:07Z","receivedAt":"2017-05-03T15:33:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n\n> We don't support config-as-a-directory (maybe someday we will?). Make\n> sure we consistently fail in this case, which should happen on platforms\n> where fopen() returns non-NULL if FREAD_READS_DIRECTORIES is\n> defined.\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nHow about saying that we make sure that this is the case by adding a test\ncase? Otherwise the reader might (and this reader certainly did) expect a\ncode change in a .c file.\n\nI reviewed this patch series, and am happy with the overall diff.\n\nMy main problem: it should be organized in a more intuitively-graspable\nway, i.e. put all the fopen() -> fopen_or_warn() changes into a single\npatch, separate out changes (drive-by test fixing for Windows and, ahem,\nwhitespace changes) into their own commits. Then those commits should be\nordered to relate an easy-flowing story.\n\nOther than that: well done,\nDscho"},{"id":"318725","messageId":"xmqqh9119obh.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"20170503101706.9223-5-pclouds@gmail.com","subject":"Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-04T05:35:30Z","receivedAt":"2017-05-04T05:35:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> dir.c | 3 +--\n> git-compat-util.h | 2 ++\n> wrapper.c | 10 ++++++++++\n> 3 files changed, 13 insertions(+), 2 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index f451bfa48c..8218a24962 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char *base, int baselen,\n> \n> \tfd = open(fname, O_RDONLY);\n> \tif (fd < 0 || fstat(fd, &st) < 0) {\n> -\t\tif (errno != ENOENT)\n> -\t\t\twarn_on_inaccessible(fname);\n> +\t\twarn_on_fopen_errors(fname);\n\nAt least in the original, a reader can guess that, because errno\ncannot be NOENT if open(2) succeeded and fstat(2) failed, we call\nwarn_on_inaccessible() only when we fail to open.\n\nThis change makes it harder to see why this is OK when the failure\nis not in open(2) but in fstat(2).\n\n\tfd = open(fname, O_RDONLY);\n\tif (fd < 0 || fstat(fd, &st) < 0) {\n-\t\tif (errno != ENOENT);\n-\t\t\twarn_on_inaccessible(fname);\n-\t\tif (0 <= fd)\n+\t\tif (fd < 0)\n+\t\t\twarn_on_fopen_errors(fname);\n+\t\telse\n\t\t\tclose(fd);\n\t\t... and the remainder of the original ...\n\nor something like that, perhaps?\n\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)\n> \treturn ret;\n> }\n> \n> +int warn_on_fopen_errors(const char *path)\n> +{\n\nDoes this need to be returning \"int\", or should it be \"void\",\nbecause it always warns when there is an issue, the caller does not\nget to choose to give its own warning?\n\n> +\tif (errno != ENOENT && errno != ENOTDIR) {\n> +\t\twarn_on_inaccessible(path);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> int xmkstemp(char *template)\n> {\n> \tint fd;\n"},{"id":"318726","messageId":"xmqqd1bp9nvj.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"20170503101706.9223-1-pclouds@gmail.com","subject":"Re: [PATCH v2 00/21]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-04T05:45:04Z","receivedAt":"2017-05-04T05:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Changes since v1:\n>\n> - fopen_or_warn() and warn_on_fopen_errors() are introduced. The\n> latter's name is probably not great...\n> - A new patch (first one) to convert a bunch to using xfopen()\n> - Fix test failure on Windows, found by Johannes Sixt\n> - Fix the memory leak in log.c, found by Jeff\n>\n> There are still a couple of fopen() remained, but I'm getting slow\n> again so let's get these in first and worry about the rest when\n> somebody has time.\n>\n> Nguyễn Thái Ngọc Duy (21):\n> Use xfopen() in more places\n> clone: use xfopen() instead of fopen()\n> config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD\n> wrapper.c: add warn_on_fopen_errors()\n> wrapper.c: add fopen_or_warn()\n> attr.c: use fopen_or_warn()\n> ident.c: use fopen_or_warn()\n> bisect: report on fopen() error\n> blame: report error on open if graft_file is a directory\n> log: report errno on failure to fopen() a file\n> log: fix memory leak in open_next_file()\n> commit.c: report error on failure to fopen() the graft file\n> remote.c: report error on failure to fopen()\n> rerere.c: report error on failure to fopen()\n> rerere.c: report correct errno\n> sequencer.c: report error on failure to fopen()\n> server-info: report error on failure to fopen()\n> wt-status.c: report error on failure to fopen()\n> xdiff-interface.c: report errno on failure to stat() or fopen()\n> config.c: handle error on failing to fopen()\n> t1308: add a test case on open a config directory\n\nThanks. If the number of parts affected by this series were\nsmaller, it may have made the review easier to have the introduction\nof a helper and its use in a single larger patch, but there are\nspread across many, some with files that are touched by different\nin-flight topics, and these \"collection of smaller patches\" makes it\neasier to manage both while reviewing and also merging.\n\nAll looked good, even though I do share the doubt on the name\n\"warn-on-fopen-errors\"; when something applies equally to fopen(3)\nand underlying open(2), I would tend to call that with open not\nfopen myself. But that is a minor point.\n\n"},{"id":"319028","messageId":"CAPc5daVwnsC2iqrX4PYL9LJyfD76dA1TS3xCNM7qHtmfHd0aWg@mail.gmail.com","threadId":"45757","inReplyTo":"xmqqd1bp9nvj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 00/21]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-07T04:20:39Z","receivedAt":"2017-05-08T00:37:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano wrote:\n>\n> Nguyễn Thái Ngọc Duy writes:\n>\n> > Changes since v1:\n> >\n> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The\n> > latter's name is probably not great...\n> > - A new patch (first one) to convert a bunch to using xfopen()\n> > - Fix test failure on Windows, found by Johannes Sixt\n> > - Fix the memory leak in log.c, found by Jeff\n> >\n> > There are still a couple of fopen() remained, but I'm getting slow\n> > again so let's get these in first and worry about the rest when\n> > somebody has time.\n\nHmm, is this topic responsible for recent breakage Travis claims on MacOS X?\n\nhttps://travis-ci.org/git/git/jobs/229585098#L3091\n\nseems to expect an error when test-config is fed a-directory but we are\nnot getting the expected warning and error?\n"},{"id":"319175","messageId":"CACsJy8BHMk4Xe+vjDke8tVUwMYsu5HCoLHVF9tArhE6L0Hu3TQ@mail.gmail.com","threadId":"45757","inReplyTo":"alpine.DEB.2.20.1705031720000.3480@virtualbox","subject":"Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-09T10:16:35Z","receivedAt":"2017-05-09T10:17:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Sorry for super late reply. I'm slowly catching up.\n\nOn Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin\n wrote:\n> Hi Duy,\n>\n>\n> On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n>\n>> There's plenty of error() in this code to safely assume --quiet is not a\n>> concern.\n>>\n>> t5512 needs update because if we check the path 'refs*master' (i.e. the\n>> asterisk is part of the path) then we'll get an EINVAL error.\n>\n> So the first change in this patch unmasks a bug that is fixed by the\n> second patch?\n\nThe change in read_branches_file() in this patch causes the failure.\nSee the original report [1],\n\n[1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/\n-- \nDuy\n"},{"id":"319176","messageId":"CACsJy8DyAV4y_-gsjk1QSGkcq+-jGX6y-ymuHvUEMfV3gxJa_g@mail.gmail.com","threadId":"45757","inReplyTo":"CAPc5daVwnsC2iqrX4PYL9LJyfD76dA1TS3xCNM7qHtmfHd0aWg@mail.gmail.com","subject":"Re: [PATCH v2 00/21]","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-05-09T10:22:45Z","receivedAt":"2017-05-09T10:23:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano wrote:\n> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano wrote:\n>>\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>> > Changes since v1:\n>> >\n>> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The\n>> > latter's name is probably not great...\n>> > - A new patch (first one) to convert a bunch to using xfopen()\n>> > - Fix test failure on Windows, found by Johannes Sixt\n>> > - Fix the memory leak in log.c, found by Jeff\n>> >\n>> > There are still a couple of fopen() remained, but I'm getting slow\n>> > again so let's get these in first and worry about the rest when\n>> > somebody has time.\n>\n> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?\n>\n> https://travis-ci.org/git/git/jobs/229585098#L3091\n>\n> seems to expect an error when test-config is fed a-directory but we are\n> not getting the expected warning and error?\n\nSounds about right. Let me see if defining FREAD_READS_DIRECTORIES on\nMacOS X makes travis happy.\n-- \nDuy\n"},{"id":"319194","messageId":"alpine.DEB.2.21.1.1705091455240.146734@virtualbox","threadId":"45757","inReplyTo":"CACsJy8BHMk4Xe+vjDke8tVUwMYsu5HCoLHVF9tArhE6L0Hu3TQ@mail.gmail.com","subject":"Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-09T12:56:05Z","receivedAt":"2017-05-09T12:56:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Tue, 9 May 2017, Duy Nguyen wrote:\n\n> Sorry for super late reply. I'm slowly catching up.\n> \n> On Wed, May 3, 2017 at 10:22 PM, Johannes Schindelin\n> wrote:\n> > Hi Duy,\n> >\n> >\n> > On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:\n> >\n> >> There's plenty of error() in this code to safely assume --quiet is not a\n> >> concern.\n> >>\n> >> t5512 needs update because if we check the path 'refs*master' (i.e. the\n> >> asterisk is part of the path) then we'll get an EINVAL error.\n> >\n> > So the first change in this patch unmasks a bug that is fixed by the\n> > second patch?\n> \n> The change in read_branches_file() in this patch causes the failure.\n> See the original report [1],\n> \n> [1] http://public-inbox.org/git/3a2686c2-6766-1235-001f-1b5283b5f408@kdbg.org/\n\nI disagree. I do not think that the first part of the patch causes the\nfailure. I think the failure was always there, we just did not bother to\nreport (and catch) it properly.\n\nCiao,\nDscho"},{"id":"319237","messageId":"xmqqzielzlqm.fsf@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"CACsJy8DyAV4y_-gsjk1QSGkcq+-jGX6y-ymuHvUEMfV3gxJa_g@mail.gmail.com","subject":"Re: [PATCH v2 00/21]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T22:52:33Z","receivedAt":"2017-05-09T22:52:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Sun, May 7, 2017 at 11:20 AM, Junio C Hamano wrote:\n>> On Thu, May 4, 2017 at 2:45 PM, Junio C Hamano wrote:\n>>>\n>>> Nguyễn Thái Ngọc Duy writes:\n>>>\n>>> > Changes since v1:\n>>> >\n>>> > - fopen_or_warn() and warn_on_fopen_errors() are introduced. The\n>>> > latter's name is probably not great...\n>>> > - A new patch (first one) to convert a bunch to using xfopen()\n>>> > - Fix test failure on Windows, found by Johannes Sixt\n>>> > - Fix the memory leak in log.c, found by Jeff\n>>> >\n>>> > There are still a couple of fopen() remained, but I'm getting slow\n>>> > again so let's get these in first and worry about the rest when\n>>> > somebody has time.\n>>\n>> Hmm, is this topic responsible for recent breakage Travis claims on MacOS X?\n>>\n>> https://travis-ci.org/git/git/jobs/229585098#L3091\n>>\n>> seems to expect an error when test-config is fed a-directory but we are\n>> not getting the expected warning and error?\n>\n> Sounds about right. Let me see if defining FREAD_READS_DIRECTORIES on\n> MacOS X makes travis happy.\n\nThanks. I should have suspected that myself to save a round-trip.\n"},{"id":"319252","messageId":"xmqqmvalxpkp.fsf_-_@gitster.mtv.corp.google.com","threadId":"45757","inReplyTo":"xmqqzielzlqm.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-10T05:12:38Z","receivedAt":"2017-05-10T05:12:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Let's do the same for Macs, as it is BSD variant after all.\n\nSigned-off-by: Junio C Hamano \n---\n config.mak.uname | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex a25ffddb3e..1743890164 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -110,6 +110,7 @@ ifeq ($(uname_S),Darwin)\n \tBASIC_CFLAGS += -DPRECOMPOSE_UNICODE\n \tBASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),SunOS)\n \tNEEDS_SOCKET = YesPlease\n-- \n2.13.0-336-gf73534b083\n\n"}]}