Re: [PATCH] config: teach git config --file - to read from the standard input
On Sun, Feb 16, 2014 at 02:13:01PM +0200, Kirill A. Shutemov wrote:
The patch extends git config --file interface to allow read config from
stdin.
Editing stdin or setting value in stdin is an error.
Include by absolute path is allowed in stdin config, but not by relative
path.
Thanks, this looks very cleanly done.
One nit:
diff --git a/config.c b/config.c
index 6269da907964..7b42608f5c89 100644
--- a/config.c
+++ b/config.c
@@ -443,10 +443,20 @@ static int git_parse_source(config_fn_t fn, void *data)
if (get_value(fn, data, var) 0)
break;
}
- if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
- else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ if (cf-die_on_error) {
+ if (cf-name)
+ die(bad config file line %d in %s,
+ cf-linenr, cf-name);
+ else
+ die(bad config file line %d, cf-linenr);
+
+ } else {
+ if (cf-name)
+ return error(bad config file line %d in %s,
+ cf-linenr, cf-name);
+ else
+ return error(bad config file line %d, cf-linenr);
+ }
I think I preferred the earlier version where you had stdin in the
name field, and this hunk could just go away. I know you switched it to
NULL here to avoid making bogus relative filenames in includes.
But that would be pretty straightforward to fix by splitting the name
field into an additional path field. It looks like git config --blob
has the same problem (it will try relative lookups in the filesystem,
even though that is nonsensical from the blob). So we could fix the
existing bug at the same time. :)
Perhaps this could go at the start of your series?
-- 8 --
Subject: config: disallow relative include paths from blobs
When we see a relative config include like:
[include]
path = foo
we make it relative to the containing directory of the file
that contains the snippet. This makes no sense for config
read from a blob, as it is not on the filesystem. Something
like HEAD:some/path could have a relative path within the
tree, but:
1. It would not be part of include.path, which explicitly
refers to the filesystem.
2. It would need different parsing rules anyway to
determine that it is a tree path.
The current code just uses the name field, which is wrong.
Let's split that into name and path fields, use the
latter for relative includes, and fill in only the former
for blobs.
Signed-off-by: Jeff King p...@peff.net
---
I don't think we considered includes at all when adding --blob. I cannot
think of any good reason to have even an absolute filesystem include
from a blob. And I wonder if it is possible to cause security mischief
by convincing git to look at /etc/passwd or similar (it would not parse,
of course, but you might be able to glean information from the error
messages or something).
It's probably OK, though, because you would generally not read real
config from an untrusted source (there are many bad things they could
set), and other code which uses the config reader explicitly does not
turn on includes.
config.c | 10 ++
t/t1305-config-include.sh | 16
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index d969a5a..b295310 100644
--- a/config.c
+++ b/config.c
@@ -21,6 +21,7 @@ struct config_source {
} buf;
} u;
const char *name;
+ const char *path;
int die_on_error;
int linenr;
int eof;
@@ -97,12 +98,12 @@ static int handle_path_include(const char *path, struct
config_include_data *inc
if (!is_absolute_path(path)) {
char *slash;
- if (!cf || !cf-name)
+ if (!cf || !cf-path)
return error(relative config includes must come from
files);
- slash = find_last_dir_sep(cf-name);
+ slash = find_last_dir_sep(cf-path);
if (slash)
- strbuf_add(buf, cf-name, slash - cf-name + 1);
+ strbuf_add(buf, cf-path, slash - cf-path + 1);
strbuf_addstr(buf, path);
path = buf.buf;
}
@@ -1040,7 +1041,7 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
struct config_source top;
top.u.file = f;
- top.name = filename;
+ top.name = top.path = filename;
top.die_on_error = 1;
top.do_fgetc = config_file_fgetc;
top.do_ungetc = config_file_ungetc;
@@ -1062,6 +1063,7 @@ int git_config_from_buf(config_fn_t fn, const char *name,
const char *buf,
top.u.buf.len = len;
top.u.buf.pos = 0;
Re: [PATCH] config: teach git config --file - to read from the standard input
Jeff King p...@peff.net writes:
+} else {
+if (cf-name)
+return error(bad config file line %d in %s,
+cf-linenr, cf-name);
+else
+return error(bad config file line %d, cf-linenr);
+}
I think I preferred the earlier version where you had stdin in the
name field, and this hunk could just go away. I know you switched it to
NULL here to avoid making bogus relative filenames in includes.
Exactly the same comment here. I really like the way how this
series becomes more polished every time we see it.
But that would be pretty straightforward to fix by splitting the name
field into an additional path field. It looks like git config --blob
has the same problem (it will try relative lookups in the filesystem,
even though that is nonsensical from the blob). So we could fix the
existing bug at the same time. :)
Perhaps this could go at the start of your series?
Sounds like the right thing to do.
Thanks.
-- 8 --
Subject: config: disallow relative include paths from blobs
When we see a relative config include like:
[include]
path = foo
we make it relative to the containing directory of the file
that contains the snippet. This makes no sense for config
read from a blob, as it is not on the filesystem. Something
like HEAD:some/path could have a relative path within the
tree, but:
1. It would not be part of include.path, which explicitly
refers to the filesystem.
2. It would need different parsing rules anyway to
determine that it is a tree path.
The current code just uses the name field, which is wrong.
Let's split that into name and path fields, use the
latter for relative includes, and fill in only the former
for blobs.
Signed-off-by: Jeff King p...@peff.net
---
I don't think we considered includes at all when adding --blob. I cannot
think of any good reason to have even an absolute filesystem include
from a blob. And I wonder if it is possible to cause security mischief
by convincing git to look at /etc/passwd or similar (it would not parse,
of course, but you might be able to glean information from the error
messages or something).
It's probably OK, though, because you would generally not read real
config from an untrusted source (there are many bad things they could
set), and other code which uses the config reader explicitly does not
turn on includes.
config.c | 10 ++
t/t1305-config-include.sh | 16
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index d969a5a..b295310 100644
--- a/config.c
+++ b/config.c
@@ -21,6 +21,7 @@ struct config_source {
} buf;
} u;
const char *name;
+ const char *path;
int die_on_error;
int linenr;
int eof;
@@ -97,12 +98,12 @@ static int handle_path_include(const char *path, struct
config_include_data *inc
if (!is_absolute_path(path)) {
char *slash;
- if (!cf || !cf-name)
+ if (!cf || !cf-path)
return error(relative config includes must come from
files);
- slash = find_last_dir_sep(cf-name);
+ slash = find_last_dir_sep(cf-path);
if (slash)
- strbuf_add(buf, cf-name, slash - cf-name + 1);
+ strbuf_add(buf, cf-path, slash - cf-path + 1);
strbuf_addstr(buf, path);
path = buf.buf;
}
@@ -1040,7 +1041,7 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
struct config_source top;
top.u.file = f;
- top.name = filename;
+ top.name = top.path = filename;
top.die_on_error = 1;
top.do_fgetc = config_file_fgetc;
top.do_ungetc = config_file_ungetc;
@@ -1062,6 +1063,7 @@ int git_config_from_buf(config_fn_t fn, const char
*name, const char *buf,
top.u.buf.len = len;
top.u.buf.pos = 0;
top.name = name;
+ top.path = NULL;
top.die_on_error = 0;
top.do_fgetc = config_buf_fgetc;
top.do_ungetc = config_buf_ungetc;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index a707076..6edd38b 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -122,6 +122,22 @@ test_expect_success 'relative includes from command line
fail' '
test_must_fail git -c include.path=one config test.one
'
+test_expect_success 'absolute includes from blobs work' '
+ echo [test]one = 1 one
+ echo [include]path=$(pwd)/one blob
+ blob=$(git hash-object -w blob)
+ echo 1 expect
+ git config --blob=$blob test.one actual
+ test_cmp expect actual
+'
+
+test_expect_success 'relative includes from blobs fail' '
+ echo [test]one = 1 one
+
Re: [PATCH] config: teach git config --file - to read from the standard input
Junio C Hamano gits...@pobox.com writes:
I think I preferred the earlier version where you had stdin in the
name field, and this hunk could just go away. I know you switched it to
NULL here to avoid making bogus relative filenames in includes.
Exactly the same comment here. I really like the way how this
series becomes more polished every time we see it.
But that would be pretty straightforward to fix by splitting the name
field into an additional path field. It looks like git config --blob
has the same problem (it will try relative lookups in the filesystem,
even though that is nonsensical from the blob). So we could fix the
existing bug at the same time. :)
Perhaps this could go at the start of your series?
Sounds like the right thing to do.
Thanks.
And [PATCH 3/3] rebased on the others may look like this.
builtin/config.c | 11 +++
cache.h | 1 +
config.c | 42 --
t/t1300-repo-config.sh| 17 +++--
t/t1305-config-include.sh | 16 +++-
5 files changed, 70 insertions(+), 17 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index de41ba5..5677c94 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -360,6 +360,9 @@ static int get_colorbool(int print)
static void check_write(void)
{
+ if (given_config_source.use_stdin)
+ die(writing to stdin is not supported);
+
if (given_config_source.blob)
die(writing config blobs is not supported);
}
@@ -472,6 +475,12 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_config_usage,
builtin_config_options);
}
+ if (given_config_source.file
+ !strcmp(given_config_source.file, -)) {
+ given_config_source.file = NULL;
+ given_config_source.use_stdin = 1;
+ }
+
if (use_global_config) {
char *user_config = NULL;
char *xdg_config = NULL;
@@ -558,6 +567,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (!given_config_source.file nongit)
die(not in a git directory);
+ if (given_config_source.use_stdin)
+ die(editing stdin is not supported);
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
diff --git a/cache.h b/cache.h
index 9d94bd6..4db19b5 100644
--- a/cache.h
+++ b/cache.h
@@ -1147,6 +1147,7 @@ extern int update_server_info(int);
#define CONFIG_GENERIC_ERROR 7
struct git_config_source {
+ unsigned int use_stdin:1;
const char *file;
const char *blob;
};
diff --git a/config.c b/config.c
index a21b0dd..9dd0bdb 100644
--- a/config.c
+++ b/config.c
@@ -1031,24 +1031,36 @@ static int do_config_from(struct config_source *top,
config_fn_t fn, void *data)
return ret;
}
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+static int do_config_from_file(config_fn_t fn,
+ const char *filename, FILE *f,
+ const char *label, void *data)
{
- int ret;
- FILE *f = fopen(filename, r);
+ struct config_source top;
- ret = -1;
- if (f) {
- struct config_source top;
+ top.u.file = f;
+ top.name = label;
+ top.path = filename;
+ top.die_on_error = 1;
+ top.do_fgetc = config_file_fgetc;
+ top.do_ungetc = config_file_ungetc;
+ top.do_ftell = config_file_ftell;
+
+ return do_config_from(top, fn, data);
+}
- top.u.file = f;
- top.name = top.path = filename;
- top.die_on_error = 1;
- top.do_fgetc = config_file_fgetc;
- top.do_ungetc = config_file_ungetc;
- top.do_ftell = config_file_ftell;
+static int git_config_from_stdin(config_fn_t fn, void *data)
+{
+ return do_config_from_file(fn, NULL, stdin, stdin, data);
+}
- ret = do_config_from(top, fn, data);
+int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+{
+ int ret = -1;
+ FILE *f;
+ f = fopen(filename, r);
+ if (f) {
+ ret = do_config_from_file(fn, filename, f, filename, data);
fclose(f);
}
return ret;
@@ -1190,7 +1202,9 @@ int git_config_with_options(config_fn_t fn, void *data,
* If we have a specific filename, use it. Otherwise, follow the
* regular lookup sequence.
*/
- if (config_source config_source-file)
+ if (config_source config_source-use_stdin)
+ return git_config_from_stdin(fn, data);
+ else if (config_source config_source-file)
[PATCH] config: teach git config --file - to read from the standard input
The patch extends git config --file interface to allow read config from
stdin.
Editing stdin or setting value in stdin is an error.
Include by absolute path is allowed in stdin config, but not by relative
path.
Signed-off-by: Kirill A. Shutemov kir...@shutemov.name
---
builtin/config.c | 11 +
cache.h | 1 +
config.c | 58 ---
t/t1300-repo-config.sh| 17 --
t/t1305-config-include.sh | 16 -
5 files changed, 82 insertions(+), 21 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index de41ba50e9ca..5677c942b693 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -360,6 +360,9 @@ static int get_colorbool(int print)
static void check_write(void)
{
+ if (given_config_source.use_stdin)
+ die(writing to stdin is not supported);
+
if (given_config_source.blob)
die(writing config blobs is not supported);
}
@@ -472,6 +475,12 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_config_usage,
builtin_config_options);
}
+ if (given_config_source.file
+ !strcmp(given_config_source.file, -)) {
+ given_config_source.file = NULL;
+ given_config_source.use_stdin = 1;
+ }
+
if (use_global_config) {
char *user_config = NULL;
char *xdg_config = NULL;
@@ -558,6 +567,8 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 0, 0);
if (!given_config_source.file nongit)
die(not in a git directory);
+ if (given_config_source.use_stdin)
+ die(editing stdin is not supported);
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
diff --git a/cache.h b/cache.h
index 9d94bd69f7db..4db19b537059 100644
--- a/cache.h
+++ b/cache.h
@@ -1147,6 +1147,7 @@ extern int update_server_info(int);
#define CONFIG_GENERIC_ERROR 7
struct git_config_source {
+ unsigned int use_stdin:1;
const char *file;
const char *blob;
};
diff --git a/config.c b/config.c
index 6269da907964..7b42608f5c89 100644
--- a/config.c
+++ b/config.c
@@ -443,10 +443,20 @@ static int git_parse_source(config_fn_t fn, void *data)
if (get_value(fn, data, var) 0)
break;
}
- if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
- else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ if (cf-die_on_error) {
+ if (cf-name)
+ die(bad config file line %d in %s,
+ cf-linenr, cf-name);
+ else
+ die(bad config file line %d, cf-linenr);
+
+ } else {
+ if (cf-name)
+ return error(bad config file line %d in %s,
+ cf-linenr, cf-name);
+ else
+ return error(bad config file line %d, cf-linenr);
+ }
}
static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -1030,24 +1040,34 @@ static int do_config_from(struct config_source *top,
config_fn_t fn, void *data)
return ret;
}
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+static int do_config_from_file(config_fn_t fn, const char *filename, FILE *f,
+ void *data)
{
- int ret;
- FILE *f = fopen(filename, r);
+ struct config_source top;
- ret = -1;
- if (f) {
- struct config_source top;
+ top.u.file = f;
+ top.name = filename;
+ top.die_on_error = 1;
+ top.do_fgetc = config_file_fgetc;
+ top.do_ungetc = config_file_ungetc;
+ top.do_ftell = config_file_ftell;
- top.u.file = f;
- top.name = filename;
- top.die_on_error = 1;
- top.do_fgetc = config_file_fgetc;
- top.do_ungetc = config_file_ungetc;
- top.do_ftell = config_file_ftell;
+ return do_config_from(top, fn, data);
+}
+
+static int git_config_from_stdin(config_fn_t fn, void *data)
+{
+ return do_config_from_file(fn, NULL, stdin, data);
+}
- ret = do_config_from(top, fn, data);
+int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+{
+ int ret = -1;
+ FILE *f;
+ f = fopen(filename, r);
+ if (f) {
+ ret = do_config_from_file(fn, filename, f, data);
fclose(f);
}
return ret;
@@ -1188,7 +1208,9 @@ int git_config_with_options(config_fn_t fn, void *data,
*
