Re: [PATCH] config: teach "git config --file -" to read from the standard input
Junio C Hamano writes:
>> I think I preferred the earlier version where you had "" 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, "", 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
Re: [PATCH] config: teach "git config --file -" to read from the standard input
Jeff King 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 "" 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
> ---
> 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 te
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 "" 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
---
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
[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
---
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_
