{"thread":{"id":"45268","subject":"[PATCH 2/2] attr.c: check if .gitattributes is a file before parsing it","startedAt":"2017-03-03T09:51:24Z","lastAt":"2017-03-03T21:06:48Z","messageCount":11,"participants":["Nguyễn Thái Ngọc Duy","Duy Nguyen","Jeff King","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"313147","messageId":"20170303094252.11706-3-pclouds@gmail.com","threadId":"45268","inReplyTo":"20170303094252.11706-1-pclouds@gmail.com","subject":"[PATCH 2/2] attr.c: check if .gitattributes is a file before parsing it","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-03T09:42:52Z","receivedAt":"2017-03-03T09:51:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Similar to the previous patch, this is about better error messages\nwhen .gitattributes happens to be a directory.\n\nFWIW .gitignore code is also checked. There open() is used instead and\nopen(\"dir\") does not fail on Linux. But the next read should fail with\nEISDIR, which is a pretty good clue already. No idea how open() on\nWindows behaves.\n---\n attr.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 5493bff224..34b6a6b9a8 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -703,11 +703,17 @@ 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;\n \tstruct attr_stack *res;\n \tchar buf[2048];\n \tint lineno = 0;\n \n+\tif (is_not_file(path)) {\n+\t\twarning(_(\"'%s' is not a file\"), path);\n+\t\treturn NULL;\n+\t}\n+\n+\tfp = fopen(path, \"r\");\n \tif (!fp) {\n \t\tif (errno != ENOENT && errno != ENOTDIR)\n \t\t\twarn_on_inaccessible(path);\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"313148","messageId":"20170303094252.11706-2-pclouds@gmail.com","threadId":"45268","inReplyTo":"20170303094252.11706-1-pclouds@gmail.com","subject":"[PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-03T09:42:51Z","receivedAt":"2017-03-03T09:52:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If a directory is given as a config file by accident, we keep open it\nas a file. The behavior of fopen() in this case seems to be\nundefined.\n\nOn Linux, we open a directory as a file ok, then get error (which we\nconsider eof) on the first read. So the config parser sees this \"file\"\nas empty (i.e. valid config). All is well and we don't complain\nanything (but we should).\n\nThe situation is slighly different on Windows. I think fopen() returns\nNULL. And we get a very unhelpful message:\n\n $ cat >abc < wrote:\n> I'm mildly negative on this approach for two reasons:\n>\n> 1. It requires doing an _extra_ check anywhere we want to care about\n> this. So if we care about file/directory confusion, we're going to\n> sprinkle these is_not_file() checks all over the code base.\n>\n> I think we're much better to just do the thing we want to do (like\n> open the file), and deal with the error results. I'm on the fence\n> on whether we want to care about the fopen behavior on Linux here\n> (where reading a directory essentially behaves like an empty file,\n> because the first read() gives an error and we don't distinguish\n> between error and EOF).\n\nI can't fix problems of my series on Windows because I don't use\nWindows (because I will not be able to verify it). So I'm definitely\non the side that makes behavior consistent across platforms. Then I\ncan at least verify some (assuming that the consistent behavior is the\nright one).\n\nI didn't go with yours because I would have to handle two separate\ncode paths (fopen returns NULL and read returns EISDIR). But yeah it\nshould be that way even if it takes more time and effort. At least\nwe're now back on the mailing list and I didn't have to hurry to get\nsomething out, to get off github.\n\n> But if we do, I think we'd either want to:\n>\n> a. actually check ferror() after getting EOF and report the read\n> error. That catches EISDIR, along with any other unexpected\n> errors.\n>\n> b. use an fopen wrapper that checks fstat(fileno(fh)) after the\n> open, and turns fopen(some_dir) into an error.\n\nIf you don't like extra check, I guess you're negative on b as well\nsince it is an extra check on Windows. That leaves us with option a.\n\n> 2. It doesn't address the root problem for git_config_from_file(),\n> which is that it is quiet when fopen fails, even if the reason is\n> something interesting besides ENOENT. The caller can't check errno\n> because it doesn't know if fopen() failed, or if the config\n> callback returned an error.\n>\n> There's an attempt to protect the call to git_config_from_file() by\n> checking access(), but that breaks down when access() and fopen()\n> have two different results (which is exactly what happens on\n> Windows in this case).\n>\n> -Peff\n-- \nDuy\n"},{"id":"313151","messageId":"20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net","threadId":"45268","inReplyTo":"CACsJy8DU7-o06mfuw1L02CFFR2wmoNa0MQJ8KqsV79ULzjRaRQ@mail.gmail.com","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T10:15:03Z","receivedAt":"2017-03-03T10:15:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote:\n\n> > But if we do, I think we'd either want to:\n> >\n> > a. actually check ferror() after getting EOF and report the read\n> > error. That catches EISDIR, along with any other unexpected\n> > errors.\n> >\n> > b. use an fopen wrapper that checks fstat(fileno(fh)) after the\n> > open, and turns fopen(some_dir) into an error.\n> \n> If you don't like extra check, I guess you're negative on b as well\n> since it is an extra check on Windows. That leaves us with option a.\n\nI don't mind _doing_ the extra check that much. I don't think we fopen\nso many files that an extra fstat on each would kill us. I mostly just\ndon't like having to sprinkle the explicit call to it everywhere. I'd be\nOK with:\n\n FILE *xfopen(const char *path, const char *mode)\n {\n\tFILE *ret = fopen(path, mode);\n #ifdef FOPEN_OPENS_DIRECTORIES\n\tif (ret) {\n\t\tstruct stat st;\n\t\tif (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) {\n\t\t\tfclose(ret);\n\t\t\tret = NULL;\n\t\t}\n\t}\n #endif\n\treturn ret;\n }\n\nBut I do think option (a) is cleaner. The only trick is that for errno\nto be valid, we need to make sure we check ferror() soon after seeing\nthe EOF return value. I suspect it would work OK in practice for the\ngit_config_from_file() case.\n\n-Peff\n"},{"id":"313152","messageId":"20170303094252.11706-1-pclouds@gmail.com","threadId":"45268","inReplyTo":null,"subject":"[PATCH 0/2] Improve error messages when opening a directory as file","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-03T09:42:50Z","receivedAt":"2017-03-03T10:15:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The topic nd/conditional-config-include hit a problem on Windows [1].\nThe test basically does this (much simplified)\n\n echo '[include]path=foo' >~/.gitconfig\n cd ~ && git init foo\n\nAt some point in 'git init' after 'foo' directory has been created, we\nrequest to include ~/foo as an extra config file. But it's a directory\nand we get some error like this\n\n fatal: bad config line 2 in file ~/.gitconfig\n\nThe message gives no clue that 'foo' is a directory (and probably\nwasted a good chunk of time of Johannes). This series tells the user\nabout that.\n\nThe other half of the problem is, the same test runs without error on\nLinux because it looks like fopen(dir) returns NULL on Windows, but\nnon-NULL on Linux and only subsequent read() returns EISDIR.\nUnfortunately the config parser conflates errors with eof, I think.\nAnd it simply sees as an empty config file, ie. a valid config\nfile. So no \"bad config line...\"\n\nI'm making sure even Linux now reports loud and clear that config\nfiles should be _files_. The same treatment is done for .gitattributes.\nI'm not so sure about .gitignore because it uses open(), not fopen()\nand I don't know if open() behaves differently on Windows.\n\nI briefly considered fopen() and open() wrappers that always rejects\ndirectories (if you need to open a directory, do it without wrappers),\nbut discarded it because it adds an extra stat() call everywhere.\n\n[1] https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33#commitcomment-21121210\n\nNguyễn Thái Ngọc Duy (2):\n config: check if config path is a file before parsing it\n attr.c: check if .gitattributes is a file before parsing it\n\n abspath.c | 7 +++++++\n attr.c | 8 +++++++-\n cache.h | 1 +\n config.c | 9 +++++++++\n t/t1300-repo-config.sh | 5 +++++\n 5 files changed, 29 insertions(+), 1 deletion(-)\n\n-- \n2.11.0.157.gd943d85\n\n"},{"id":"313153","messageId":"20170303103115.an5dg2pacest4zwo@sigill.intra.peff.net","threadId":"45268","inReplyTo":"20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T10:31:16Z","receivedAt":"2017-03-03T10:32:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 05:15:03AM -0500, Jeff King wrote:\n\n> But I do think option (a) is cleaner. The only trick is that for errno\n> to be valid, we need to make sure we check ferror() soon after seeing\n> the EOF return value. I suspect it would work OK in practice for the\n> git_config_from_file() case.\n\nSomething like this is a big improvement, I think:\n\ndiff --git a/config.c b/config.c\nindex c6b874a7b..27b410dfe 100644\n--- a/config.c\n+++ b/config.c\n@@ -156,15 +156,14 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n \n-\tif (!access_or_die(path, R_OK, 0)) {\n-\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n-\t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n-\t\t\t !cf ? \"\" :\n-\t\t\t cf->name ? cf->name :\n-\t\t\t \"the command line\");\n-\t\tret = git_config_from_file(git_config_include, path, inc);\n-\t\tinc->depth--;\n-\t}\n+\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n+\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n+\t\t !cf ? \"\" :\n+\t\t cf->name ? cf->name :\n+\t\t \"the command line\");\n+\tret = git_config_from_file(git_config_include, path, inc);\n+\tinc->depth--;\n+\n \tstrbuf_release(&buf);\n \tfree(expanded);\n \treturn ret;\n@@ -1213,10 +1212,18 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tFILE *f;\n \n \tf = fopen(filename, \"r\");\n-\tif (f) {\n+\tif (!f) {\n+\t\t/* a missing file is silently treated as an empty one */\n+\t\tif (errno == ENOENT || errno == EISDIR)\n+\t\t\tret = 0;\n+\t\telse\n+\t\t\tret = error_errno(\"unable to open %s\", filename);\n+\t} else {\n \t\tflockfile(f);\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);\n \t\tfunlockfile(f);\n+\t\tif (!ret && ferror(f))\n+\t\t\tret = error_errno(\"unable to read from %s\", filename);\n \t\tfclose(f);\n \t}\n \treturn ret;\n\nThen if you do:\n\n cd repo.git\n git config include.path this-is-broken\n\nyou get useful errors for a variety of situations:\n\n $ mkdir this-is-broken\n $ git rev-parse\n error: unable to read from this-is-broken: Is a directory\n fatal: bad config line 7 in file config\n\n $ rmdir this-is-broken\n $ ln -s this-is-broken this-is-broken\n $ git rev-parse\n error: unable to open this-is-broken: Too many levels of symbolic links\n fatal: bad config line 7 in file config\n\nand so on. The two caveats are:\n\n 1. A few of the callers treat EACCES specially, so we'd potentially\n want a flag for that (or alternatively, everybody should just fopen\n the file themselves and pass in the handle).\n\n 2. The call in read_repository_format() does not check the return\n value at all. It measures errors only as \"did the parser find a\n core.repositoryformatversion field I can look at\", though arguably\n it should check for other errors, too (if we read \"version=2\", but\n then got a read error before we were able to read the extensions,\n that would be wrong and bad).\n\n But either way I suspect it probably prefers the current \"quiet\"\n behavior, since it is used to speculatively look for repositories.\n\nSo probably git_config_from_file() needs a flags parameter, and both\n\"quiet\" and EACCES handling can go in there.\n\n-Peff\n"},{"id":"313154","messageId":"20170303103319.uei446qkt6jadsn5@sigill.intra.peff.net","threadId":"45268","inReplyTo":"CACsJy8CDOBUe7S3sYz=_tAJy2ajM5h2v+ZypD6Cr1uJo7XAnGQ@mail.gmail.com","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T10:33:19Z","receivedAt":"2017-03-03T10:43:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote:\n\n> On Fri, Mar 3, 2017 at 5:15 PM, Jeff King wrote:\n> > But I do think option (a) is cleaner. The only trick is that for errno\n> > to be valid, we need to make sure we check ferror() soon after seeing\n> > the EOF return value. I suspect it would work OK in practice for the\n> > git_config_from_file() case.\n> \n> stdio error handling is a pain. Maybe we're better of with open() and\n> mmap() (or even read_in_full)? I/O error handling would be at the\n> beginning, not buried deep in the parser. Hmm.. since we already have\n> \"fgetc' version for config blobs, this could kill some code...\n\nYeah, I don't mind a read_in_full() version. Config isn't _supposed_ to\nbe big (and if it is you're in trouble anyway, because I'm pretty sure\nwe still parse it several times per command invocation).\n\nI don't think that removes the issues I've mentioned with\ngit_config_from_file() being too quiet. But it solves the ferror()\nquestion (though I think we pretty much return immediately from the\nparser on EOF, so it's _probably_ OK to use it like in the diff I just\nsent).\n\n-Peff\n"},{"id":"313155","messageId":"CACsJy8CDOBUe7S3sYz=_tAJy2ajM5h2v+ZypD6Cr1uJo7XAnGQ@mail.gmail.com","threadId":"45268","inReplyTo":"20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-03-03T10:29:47Z","receivedAt":"2017-03-03T10:58:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 3, 2017 at 5:15 PM, Jeff King wrote:\n> But I do think option (a) is cleaner. The only trick is that for errno\n> to be valid, we need to make sure we check ferror() soon after seeing\n> the EOF return value. I suspect it would work OK in practice for the\n> git_config_from_file() case.\n\nstdio error handling is a pain. Maybe we're better of with open() and\nmmap() (or even read_in_full)? I/O error handling would be at the\nbeginning, not buried deep in the parser. Hmm.. since we already have\n\"fgetc' version for config blobs, this could kill some code...\n-- \nDuy\n"},{"id":"313159","messageId":"20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net","threadId":"45268","inReplyTo":"20170303094252.11706-2-pclouds@gmail.com","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-03T09:53:51Z","receivedAt":"2017-03-03T11:41:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> If a directory is given as a config file by accident, we keep open it\n> as a file. The behavior of fopen() in this case seems to be\n> undefined.\n> \n> On Linux, we open a directory as a file ok, then get error (which we\n> consider eof) on the first read. So the config parser sees this \"file\"\n> as empty (i.e. valid config). All is well and we don't complain\n> anything (but we should).\n> \n> The situation is slighly different on Windows. I think fopen() returns\n> NULL. And we get a very unhelpful message:\n> \n> $ cat >abc < [include]\n> path = /tmp/foo\n> EOF\n> $ mkdir /tmp/foo\n> $ git config --includes --file=abc core.bare\n> fatal: bad config line 3 in file abc\n> \n> Opening a directory is wrong in the first place. Avoid it. If caught,\n> print something better. With this patch, we have\n> \n> $ git config --includes --file=abc core.bare\n> error: '/tmp/foo' is not a file\n> fatal: bad config line 3 in file abc\n> \n> It's not perfect (line should be 2 instead of 3). But it's definitely\n> improving.\n> \n> The new test is only relevant on linux where we blindly open the\n> directory and consider it an empty file. On Windows, the test should\n> pass even without this patch.\n\nI'm mildly negative on this approach for two reasons:\n\n 1. It requires doing an _extra_ check anywhere we want to care about\n this. So if we care about file/directory confusion, we're going to\n sprinkle these is_not_file() checks all over the code base.\n\n I think we're much better to just do the thing we want to do (like\n open the file), and deal with the error results. I'm on the fence\n on whether we want to care about the fopen behavior on Linux here\n (where reading a directory essentially behaves like an empty file,\n because the first read() gives an error and we don't distinguish\n between error and EOF). But if we do, I think we'd either want to:\n\n a. actually check ferror() after getting EOF and report the read\n error. That catches EISDIR, along with any other unexpected\n\t errors.\n\n b. use an fopen wrapper that checks fstat(fileno(fh)) after the\n open, and turns fopen(some_dir) into an error.\n\n 2. It doesn't address the root problem for git_config_from_file(),\n which is that it is quiet when fopen fails, even if the reason is\n something interesting besides ENOENT. The caller can't check errno\n because it doesn't know if fopen() failed, or if the config\n callback returned an error.\n\n There's an attempt to protect the call to git_config_from_file() by\n checking access(), but that breaks down when access() and fopen()\n have two different results (which is exactly what happens on\n Windows in this case).\n\n-Peff\n"},{"id":"313229","messageId":"a88c1a06-0bdb-aab5-850d-3b5627eca605@ramsayjones.plus.com","threadId":"45268","inReplyTo":"20170303094252.11706-2-pclouds@gmail.com","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-03T20:21:37Z","receivedAt":"2017-03-03T20:23:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:\n> If a directory is given as a config file by accident, we keep open it\n> as a file. The behavior of fopen() in this case seems to be\n> undefined.\n> \n> On Linux, we open a directory as a file ok, then get error (which we\n> consider eof) on the first read. So the config parser sees this \"file\"\n> as empty (i.e. valid config). All is well and we don't complain\n> anything (but we should).\n> \n> The situation is slighly different on Windows. I think fopen() returns\n> NULL. And we get a very unhelpful message:\n> \n> $ cat >abc < [include]\n> path = /tmp/foo\n> EOF\n> $ mkdir /tmp/foo\n> $ git config --includes --file=abc core.bare\n> fatal: bad config line 3 in file abc\n> \n> Opening a directory is wrong in the first place. Avoid it. If caught,\n> print something better. With this patch, we have\n> \n> $ git config --includes --file=abc core.bare\n> error: '/tmp/foo' is not a file\n> fatal: bad config line 3 in file abc\n> \n> It's not perfect (line should be 2 instead of 3). But it's definitely\n> improving.\n> \n> The new test is only relevant on linux where we blindly open the\n> directory and consider it an empty file. On Windows, the test should\n> pass even without this patch.\n> ---\n> abspath.c | 7 +++++++\n> cache.h | 1 +\n> config.c | 9 +++++++++\n> t/t1300-repo-config.sh | 5 +++++\n> 4 files changed, 22 insertions(+)\n> \n> diff --git a/abspath.c b/abspath.c\n> index 2f0c26e0e2..373cc3abb2 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -11,6 +11,13 @@ int is_directory(const char *path)\n> \treturn (!stat(path, &st) && S_ISDIR(st.st_mode));\n> }\n> \n> +int is_not_file(const char *path)\n> +{\n> +\tstruct stat st;\n> +\n> +\treturn !stat(path, &st) && !S_ISREG(st.st_mode);\n> +}\n> +\n> /* removes the last path component from 'path' except if 'path' is root */\n> static void strip_last_component(struct strbuf *path)\n> {\n> diff --git a/cache.h b/cache.h\n> index 80b6372cf7..bdd1402ab9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)\n> \treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\n> }\n> int is_directory(const char *);\n> +int is_not_file(const char *);\n> char *strbuf_realpath(struct strbuf *resolved, const char *path,\n> \t\t int die_on_error);\n> const char *real_path(const char *path);\n> diff --git a/config.c b/config.c\n> index c6b874a7bf..c21c0ce433 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -13,6 +13,7 @@\n> #include \"hashmap.h\"\n> #include \"string-list.h\"\n> #include \"utf8.h\"\n> +#include \"dir.h\"\n\nSomething is a bit odd here - nothing in this commit (that I can\nsee, anyway) would require the addition of this include. Also, this\ninclude is already there in the 'pu' branch, brought in by your\nconditional include changes. So, ...\n\nATB,\nRamsay Jones\n"},{"id":"313233","messageId":"xmqq4lza840k.fsf@gitster.mtv.corp.google.com","threadId":"45268","inReplyTo":"20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: check if config path is a file before parsing it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T21:05:47Z","receivedAt":"2017-03-03T21:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> a. actually check ferror() after getting EOF and report the read\n> error. That catches EISDIR, along with any other unexpected\n> \t errors.\n\nThat is the most sensible, I would think (assuming that we really\nget EISDIR instead of silent EOF).\n\n> b. use an fopen wrapper that checks fstat(fileno(fh)) after the\n> open, and turns fopen(some_dir) into an error.\n\nThat's already an option with FREAD_READS_DIRECTORIES, I think.\n\n> 2. It doesn't address the root problem for git_config_from_file(),\n> which is that it is quiet when fopen fails, even if the reason is\n> something interesting besides ENOENT. The caller can't check errno\n> because it doesn't know if fopen() failed, or if the config\n> callback returned an error.\n\nPerhaps like this one as a starting point, with FREAD_READS_DIRECTORIES?\n\n config.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 0dac0f4cb2..af8c01c8a3 100644\n--- a/config.c\n+++ b/config.c\n@@ -1305,6 +1305,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \tFILE *f;\n \n \tf = fopen(filename, \"r\");\n+\tif (!f && errno != ENOENT)\n+\t\tdie_errno(\"fopen('%s') failed\", filename);\n+\n \tif (f) {\n \t\tflockfile(f);\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);\n"}]}