Thread (10 messages) flat view 10 messages, 3 authors, 2017-03-03

Re: [PATCH 1/2] config: check if config path is a file before parsing it

From: Ramsay Jones <hidden>
Date: 2017-03-03 20:23:14


On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:
quoted hunk ↗ jump to hunk
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help