* [PATCH 0/2] Improve error messages when opening a directory as file @ 2017-03-03 9:42 Nguyễn Thái Ngọc Duy 2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy 2017-03-03 9:42 ` [PATCH 2/2] attr.c: check if .gitattributes " Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 11+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Nguyễn Thái Ngọc Duy The topic nd/conditional-config-include hit a problem on Windows [1]. The test basically does this (much simplified) echo '[include]path=foo' >~/.gitconfig cd ~ && git init foo At some point in 'git init' after 'foo' directory has been created, we request to include ~/foo as an extra config file. But it's a directory and we get some error like this fatal: bad config line 2 in file ~/.gitconfig The message gives no clue that 'foo' is a directory (and probably wasted a good chunk of time of Johannes). This series tells the user about that. The other half of the problem is, the same test runs without error on Linux because it looks like fopen(dir) returns NULL on Windows, but non-NULL on Linux and only subsequent read() returns EISDIR. Unfortunately the config parser conflates errors with eof, I think. And it simply sees <dir> as an empty config file, ie. a valid config file. So no "bad config line..." I'm making sure even Linux now reports loud and clear that config files should be _files_. The same treatment is done for .gitattributes. I'm not so sure about .gitignore because it uses open(), not fopen() and I don't know if open() behaves differently on Windows. I briefly considered fopen() and open() wrappers that always rejects directories (if you need to open a directory, do it without wrappers), but discarded it because it adds an extra stat() call everywhere. [1] https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33#commitcomment-21121210 Nguyễn Thái Ngọc Duy (2): config: check if config path is a file before parsing it attr.c: check if .gitattributes is a file before parsing it abspath.c | 7 +++++++ attr.c | 8 +++++++- cache.h | 1 + config.c | 9 +++++++++ t/t1300-repo-config.sh | 5 +++++ 5 files changed, 29 insertions(+), 1 deletion(-) -- 2.11.0.157.gd943d85 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 9:42 [PATCH 0/2] Improve error messages when opening a directory as file Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 ` Nguyễn Thái Ngọc Duy 2017-03-03 9:53 ` Jeff King 2017-03-03 20:21 ` Ramsay Jones 2017-03-03 9:42 ` [PATCH 2/2] attr.c: check if .gitattributes " Nguyễn Thái Ngọc Duy 1 sibling, 2 replies; 11+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Nguyễn Thái Ngọc Duy If a directory is given as a config file by accident, we keep open it as a file. The behavior of fopen() in this case seems to be undefined. On Linux, we open a directory as a file ok, then get error (which we consider eof) on the first read. So the config parser sees this "file" as empty (i.e. valid config). All is well and we don't complain anything (but we should). The situation is slighly different on Windows. I think fopen() returns NULL. And we get a very unhelpful message: $ cat >abc <<EOF [include] path = /tmp/foo EOF $ mkdir /tmp/foo $ git config --includes --file=abc core.bare fatal: bad config line 3 in file abc Opening a directory is wrong in the first place. Avoid it. If caught, print something better. With this patch, we have $ git config --includes --file=abc core.bare error: '/tmp/foo' is not a file fatal: bad config line 3 in file abc It's not perfect (line should be 2 instead of 3). But it's definitely improving. The new test is only relevant on linux where we blindly open the directory and consider it an empty file. On Windows, the test should pass even without this patch. --- abspath.c | 7 +++++++ cache.h | 1 + config.c | 9 +++++++++ t/t1300-repo-config.sh | 5 +++++ 4 files changed, 22 insertions(+) diff --git a/abspath.c b/abspath.c index 2f0c26e0e2..373cc3abb2 100644 --- a/abspath.c +++ b/abspath.c @@ -11,6 +11,13 @@ int is_directory(const char *path) return (!stat(path, &st) && S_ISDIR(st.st_mode)); } +int is_not_file(const char *path) +{ + struct stat st; + + return !stat(path, &st) && !S_ISREG(st.st_mode); +} + /* removes the last path component from 'path' except if 'path' is root */ static void strip_last_component(struct strbuf *path) { diff --git a/cache.h b/cache.h index 80b6372cf7..bdd1402ab9 100644 --- a/cache.h +++ b/cache.h @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path) return is_dir_sep(path[0]) || has_dos_drive_prefix(path); } int is_directory(const char *); +int is_not_file(const char *); char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error); const char *real_path(const char *path); diff --git a/config.c b/config.c index c6b874a7bf..c21c0ce433 100644 --- a/config.c +++ b/config.c @@ -13,6 +13,7 @@ #include "hashmap.h" #include "string-list.h" #include "utf8.h" +#include "dir.h" struct config_source { struct config_source *prev; @@ -1212,6 +1213,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) int ret = -1; FILE *f; + if (is_not_file(filename)) + return error(_("'%s' is not a file"), filename); + f = fopen(filename, "r"); if (f) { flockfile(f); @@ -2451,6 +2455,11 @@ int git_config_rename_section_in_file(const char *config_filename, goto out; } + if (!S_ISREG(st.st_mode)) { + ret = error(_("'%s' is not a file"), config_filename); + goto out; + } + if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) { ret = error_errno("chmod on %s failed", get_lock_file_path(lock)); diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 052f120216..3683fbb84e 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1477,4 +1477,9 @@ test_expect_success !MINGW '--show-origin blob ref' ' test_cmp expect output ' +test_expect_success 'a directory is given as a config file' ' + mkdir config-dir && + test_must_fail git config --file=config-dir --list +' + test_done -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy @ 2017-03-03 9:53 ` Jeff King 2017-03-03 10:06 ` Duy Nguyen 2017-03-03 21:05 ` Junio C Hamano 2017-03-03 20:21 ` Ramsay Jones 1 sibling, 2 replies; 11+ messages in thread From: Jeff King @ 2017-03-03 9:53 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, Junio C Hamano, Johannes Schindelin On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote: > If a directory is given as a config file by accident, we keep open it > as a file. The behavior of fopen() in this case seems to be > undefined. > > On Linux, we open a directory as a file ok, then get error (which we > consider eof) on the first read. So the config parser sees this "file" > as empty (i.e. valid config). All is well and we don't complain > anything (but we should). > > The situation is slighly different on Windows. I think fopen() returns > NULL. And we get a very unhelpful message: > > $ cat >abc <<EOF > [include] > path = /tmp/foo > EOF > $ mkdir /tmp/foo > $ git config --includes --file=abc core.bare > fatal: bad config line 3 in file abc > > Opening a directory is wrong in the first place. Avoid it. If caught, > print something better. With this patch, we have > > $ git config --includes --file=abc core.bare > error: '/tmp/foo' is not a file > fatal: bad config line 3 in file abc > > It's not perfect (line should be 2 instead of 3). But it's definitely > improving. > > The new test is only relevant on linux where we blindly open the > directory and consider it an empty file. On Windows, the test should > pass even without this patch. I'm mildly negative on this approach for two reasons: 1. It requires doing an _extra_ check anywhere we want to care about this. So if we care about file/directory confusion, we're going to sprinkle these is_not_file() checks all over the code base. I think we're much better to just do the thing we want to do (like open the file), and deal with the error results. I'm on the fence on whether we want to care about the fopen behavior on Linux here (where reading a directory essentially behaves like an empty file, because the first read() gives an error and we don't distinguish between error and EOF). But if we do, I think we'd either want to: a. actually check ferror() after getting EOF and report the read error. That catches EISDIR, along with any other unexpected errors. b. use an fopen wrapper that checks fstat(fileno(fh)) after the open, and turns fopen(some_dir) into an error. 2. It doesn't address the root problem for git_config_from_file(), which is that it is quiet when fopen fails, even if the reason is something interesting besides ENOENT. The caller can't check errno because it doesn't know if fopen() failed, or if the config callback returned an error. There's an attempt to protect the call to git_config_from_file() by checking access(), but that breaks down when access() and fopen() have two different results (which is exactly what happens on Windows in this case). -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 9:53 ` Jeff King @ 2017-03-03 10:06 ` Duy Nguyen 2017-03-03 10:15 ` Jeff King 2017-03-03 21:05 ` Junio C Hamano 1 sibling, 1 reply; 11+ messages in thread From: Duy Nguyen @ 2017-03-03 10:06 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin On Fri, Mar 3, 2017 at 4:53 PM, Jeff King <peff@peff.net> wrote: > I'm mildly negative on this approach for two reasons: > > 1. It requires doing an _extra_ check anywhere we want to care about > this. So if we care about file/directory confusion, we're going to > sprinkle these is_not_file() checks all over the code base. > > I think we're much better to just do the thing we want to do (like > open the file), and deal with the error results. I'm on the fence > on whether we want to care about the fopen behavior on Linux here > (where reading a directory essentially behaves like an empty file, > because the first read() gives an error and we don't distinguish > between error and EOF). I can't fix problems of my series on Windows because I don't use Windows (because I will not be able to verify it). So I'm definitely on the side that makes behavior consistent across platforms. Then I can at least verify some (assuming that the consistent behavior is the right one). I didn't go with yours because I would have to handle two separate code paths (fopen returns NULL and read returns EISDIR). But yeah it should be that way even if it takes more time and effort. At least we're now back on the mailing list and I didn't have to hurry to get something out, to get off github. > But if we do, I think we'd either want to: > > a. actually check ferror() after getting EOF and report the read > error. That catches EISDIR, along with any other unexpected > errors. > > b. use an fopen wrapper that checks fstat(fileno(fh)) after the > open, and turns fopen(some_dir) into an error. If you don't like extra check, I guess you're negative on b as well since it is an extra check on Windows. That leaves us with option a. > 2. It doesn't address the root problem for git_config_from_file(), > which is that it is quiet when fopen fails, even if the reason is > something interesting besides ENOENT. The caller can't check errno > because it doesn't know if fopen() failed, or if the config > callback returned an error. > > There's an attempt to protect the call to git_config_from_file() by > checking access(), but that breaks down when access() and fopen() > have two different results (which is exactly what happens on > Windows in this case). > > -Peff -- Duy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 10:06 ` Duy Nguyen @ 2017-03-03 10:15 ` Jeff King 2017-03-03 10:29 ` Duy Nguyen 2017-03-03 10:31 ` Jeff King 0 siblings, 2 replies; 11+ messages in thread From: Jeff King @ 2017-03-03 10:15 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote: > > But if we do, I think we'd either want to: > > > > a. actually check ferror() after getting EOF and report the read > > error. That catches EISDIR, along with any other unexpected > > errors. > > > > b. use an fopen wrapper that checks fstat(fileno(fh)) after the > > open, and turns fopen(some_dir) into an error. > > If you don't like extra check, I guess you're negative on b as well > since it is an extra check on Windows. That leaves us with option a. I don't mind _doing_ the extra check that much. I don't think we fopen so many files that an extra fstat on each would kill us. I mostly just don't like having to sprinkle the explicit call to it everywhere. I'd be OK with: FILE *xfopen(const char *path, const char *mode) { FILE *ret = fopen(path, mode); #ifdef FOPEN_OPENS_DIRECTORIES if (ret) { struct stat st; if (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) { fclose(ret); ret = NULL; } } #endif return ret; } But I do think option (a) is cleaner. The only trick is that for errno to be valid, we need to make sure we check ferror() soon after seeing the EOF return value. I suspect it would work OK in practice for the git_config_from_file() case. -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 10:15 ` Jeff King @ 2017-03-03 10:29 ` Duy Nguyen 2017-03-03 10:33 ` Jeff King 2017-03-03 10:31 ` Jeff King 1 sibling, 1 reply; 11+ messages in thread From: Duy Nguyen @ 2017-03-03 10:29 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote: > But I do think option (a) is cleaner. The only trick is that for errno > to be valid, we need to make sure we check ferror() soon after seeing > the EOF return value. I suspect it would work OK in practice for the > git_config_from_file() case. stdio error handling is a pain. Maybe we're better of with open() and mmap() (or even read_in_full)? I/O error handling would be at the beginning, not buried deep in the parser. Hmm.. since we already have "fgetc' version for config blobs, this could kill some code... -- Duy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 10:29 ` Duy Nguyen @ 2017-03-03 10:33 ` Jeff King 0 siblings, 0 replies; 11+ messages in thread From: Jeff King @ 2017-03-03 10:33 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote: > On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote: > > But I do think option (a) is cleaner. The only trick is that for errno > > to be valid, we need to make sure we check ferror() soon after seeing > > the EOF return value. I suspect it would work OK in practice for the > > git_config_from_file() case. > > stdio error handling is a pain. Maybe we're better of with open() and > mmap() (or even read_in_full)? I/O error handling would be at the > beginning, not buried deep in the parser. Hmm.. since we already have > "fgetc' version for config blobs, this could kill some code... Yeah, I don't mind a read_in_full() version. Config isn't _supposed_ to be big (and if it is you're in trouble anyway, because I'm pretty sure we still parse it several times per command invocation). I don't think that removes the issues I've mentioned with git_config_from_file() being too quiet. But it solves the ferror() question (though I think we pretty much return immediately from the parser on EOF, so it's _probably_ OK to use it like in the diff I just sent). -Peff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 10:15 ` Jeff King 2017-03-03 10:29 ` Duy Nguyen @ 2017-03-03 10:31 ` Jeff King 1 sibling, 0 replies; 11+ messages in thread From: Jeff King @ 2017-03-03 10:31 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin On Fri, Mar 03, 2017 at 05:15:03AM -0500, Jeff King wrote: > But I do think option (a) is cleaner. The only trick is that for errno > to be valid, we need to make sure we check ferror() soon after seeing > the EOF return value. I suspect it would work OK in practice for the > git_config_from_file() case. Something like this is a big improvement, I think: diff --git a/config.c b/config.c index c6b874a7b..27b410dfe 100644 --- a/config.c +++ b/config.c @@ -156,15 +156,14 @@ static int handle_path_include(const char *path, struct config_include_data *inc path = buf.buf; } - if (!access_or_die(path, R_OK, 0)) { - if (++inc->depth > MAX_INCLUDE_DEPTH) - die(include_depth_advice, MAX_INCLUDE_DEPTH, path, - !cf ? "<unknown>" : - cf->name ? cf->name : - "the command line"); - ret = git_config_from_file(git_config_include, path, inc); - inc->depth--; - } + if (++inc->depth > MAX_INCLUDE_DEPTH) + die(include_depth_advice, MAX_INCLUDE_DEPTH, path, + !cf ? "<unknown>" : + cf->name ? cf->name : + "the command line"); + ret = git_config_from_file(git_config_include, path, inc); + inc->depth--; + strbuf_release(&buf); free(expanded); return ret; @@ -1213,10 +1212,18 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) FILE *f; f = fopen(filename, "r"); - if (f) { + if (!f) { + /* a missing file is silently treated as an empty one */ + if (errno == ENOENT || errno == EISDIR) + ret = 0; + else + ret = error_errno("unable to open %s", filename); + } else { flockfile(f); ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data); funlockfile(f); + if (!ret && ferror(f)) + ret = error_errno("unable to read from %s", filename); fclose(f); } return ret; Then if you do: cd repo.git git config include.path this-is-broken you get useful errors for a variety of situations: $ mkdir this-is-broken $ git rev-parse error: unable to read from this-is-broken: Is a directory fatal: bad config line 7 in file config $ rmdir this-is-broken $ ln -s this-is-broken this-is-broken $ git rev-parse error: unable to open this-is-broken: Too many levels of symbolic links fatal: bad config line 7 in file config and so on. The two caveats are: 1. A few of the callers treat EACCES specially, so we'd potentially want a flag for that (or alternatively, everybody should just fopen the file themselves and pass in the handle). 2. The call in read_repository_format() does not check the return value at all. It measures errors only as "did the parser find a core.repositoryformatversion field I can look at", though arguably it should check for other errors, too (if we read "version=2", but then got a read error before we were able to read the extensions, that would be wrong and bad). But either way I suspect it probably prefers the current "quiet" behavior, since it is used to speculatively look for repositories. So probably git_config_from_file() needs a flags parameter, and both "quiet" and EACCES handling can go in there. -Peff ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 9:53 ` Jeff King 2017-03-03 10:06 ` Duy Nguyen @ 2017-03-03 21:05 ` Junio C Hamano 1 sibling, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2017-03-03 21:05 UTC (permalink / raw) To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, Johannes Schindelin Jeff King <peff@peff.net> writes: > a. actually check ferror() after getting EOF and report the read > error. That catches EISDIR, along with any other unexpected > errors. That is the most sensible, I would think (assuming that we really get EISDIR instead of silent EOF). > b. use an fopen wrapper that checks fstat(fileno(fh)) after the > open, and turns fopen(some_dir) into an error. That's already an option with FREAD_READS_DIRECTORIES, I think. > 2. It doesn't address the root problem for git_config_from_file(), > which is that it is quiet when fopen fails, even if the reason is > something interesting besides ENOENT. The caller can't check errno > because it doesn't know if fopen() failed, or if the config > callback returned an error. Perhaps like this one as a starting point, with FREAD_READS_DIRECTORIES? config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/config.c b/config.c index 0dac0f4cb2..af8c01c8a3 100644 --- a/config.c +++ b/config.c @@ -1305,6 +1305,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data) FILE *f; f = fopen(filename, "r"); + if (!f && errno != ENOENT) + die_errno("fopen('%s') failed", filename); + if (f) { flockfile(f); ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data); ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] config: check if config path is a file before parsing it 2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy 2017-03-03 9:53 ` Jeff King @ 2017-03-03 20:21 ` Ramsay Jones 1 sibling, 0 replies; 11+ messages in thread From: Ramsay Jones @ 2017-03-03 20:21 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy, git Cc: Junio C Hamano, Jeff King, Johannes Schindelin On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote: > If a directory is given as a config file by accident, we keep open it > as a file. The behavior of fopen() in this case seems to be > undefined. > > On Linux, we open a directory as a file ok, then get error (which we > consider eof) on the first read. So the config parser sees this "file" > as empty (i.e. valid config). All is well and we don't complain > anything (but we should). > > The situation is slighly different on Windows. I think fopen() returns > NULL. And we get a very unhelpful message: > > $ cat >abc <<EOF > [include] > path = /tmp/foo > EOF > $ mkdir /tmp/foo > $ git config --includes --file=abc core.bare > fatal: bad config line 3 in file abc > > Opening a directory is wrong in the first place. Avoid it. If caught, > print something better. With this patch, we have > > $ git config --includes --file=abc core.bare > error: '/tmp/foo' is not a file > fatal: bad config line 3 in file abc > > It's not perfect (line should be 2 instead of 3). But it's definitely > improving. > > The new test is only relevant on linux where we blindly open the > directory and consider it an empty file. On Windows, the test should > pass even without this patch. > --- > abspath.c | 7 +++++++ > cache.h | 1 + > config.c | 9 +++++++++ > t/t1300-repo-config.sh | 5 +++++ > 4 files changed, 22 insertions(+) > > diff --git a/abspath.c b/abspath.c > index 2f0c26e0e2..373cc3abb2 100644 > --- a/abspath.c > +++ b/abspath.c > @@ -11,6 +11,13 @@ int is_directory(const char *path) > return (!stat(path, &st) && S_ISDIR(st.st_mode)); > } > > +int is_not_file(const char *path) > +{ > + struct stat st; > + > + return !stat(path, &st) && !S_ISREG(st.st_mode); > +} > + > /* removes the last path component from 'path' except if 'path' is root */ > static void strip_last_component(struct strbuf *path) > { > diff --git a/cache.h b/cache.h > index 80b6372cf7..bdd1402ab9 100644 > --- a/cache.h > +++ b/cache.h > @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path) > return is_dir_sep(path[0]) || has_dos_drive_prefix(path); > } > int is_directory(const char *); > +int is_not_file(const char *); > char *strbuf_realpath(struct strbuf *resolved, const char *path, > int die_on_error); > const char *real_path(const char *path); > diff --git a/config.c b/config.c > index c6b874a7bf..c21c0ce433 100644 > --- a/config.c > +++ b/config.c > @@ -13,6 +13,7 @@ > #include "hashmap.h" > #include "string-list.h" > #include "utf8.h" > +#include "dir.h" Something is a bit odd here - nothing in this commit (that I can see, anyway) would require the addition of this include. Also, this include is already there in the 'pu' branch, brought in by your conditional include changes. So, ... ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] attr.c: check if .gitattributes is a file before parsing it 2017-03-03 9:42 [PATCH 0/2] Improve error messages when opening a directory as file Nguyễn Thái Ngọc Duy 2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 ` Nguyễn Thái Ngọc Duy 1 sibling, 0 replies; 11+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-03-03 9:42 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Johannes Schindelin, Nguyễn Thái Ngọc Duy Similar to the previous patch, this is about better error messages when .gitattributes happens to be a directory. FWIW .gitignore code is also checked. There open() is used instead and open("dir") does not fail on Linux. But the next read should fail with EISDIR, which is a pretty good clue already. No idea how open() on Windows behaves. --- attr.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/attr.c b/attr.c index 5493bff224..34b6a6b9a8 100644 --- a/attr.c +++ b/attr.c @@ -703,11 +703,17 @@ void git_attr_set_direction(enum git_attr_direction new_direction, static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) { - FILE *fp = fopen(path, "r"); + FILE *fp; struct attr_stack *res; char buf[2048]; int lineno = 0; + if (is_not_file(path)) { + warning(_("'%s' is not a file"), path); + return NULL; + } + + fp = fopen(path, "r"); if (!fp) { if (errno != ENOENT && errno != ENOTDIR) warn_on_inaccessible(path); -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-03-03 21:06 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-03 9:42 [PATCH 0/2] Improve error messages when opening a directory as file Nguyễn Thái Ngọc Duy 2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy 2017-03-03 9:53 ` Jeff King 2017-03-03 10:06 ` Duy Nguyen 2017-03-03 10:15 ` Jeff King 2017-03-03 10:29 ` Duy Nguyen 2017-03-03 10:33 ` Jeff King 2017-03-03 10:31 ` Jeff King 2017-03-03 21:05 ` Junio C Hamano 2017-03-03 20:21 ` Ramsay Jones 2017-03-03 9:42 ` [PATCH 2/2] attr.c: check if .gitattributes " Nguyễn Thái Ngọc Duy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).