[PATCH] config: git_config_from_file(): handle - filename as stdin
The patch extends git config --file interface to allow read config from
stdin.
Signed-off-by: Kirill A. Shutemov kir...@shutemov.name
---
config.c | 10 ++
t/t1300-repo-config.sh | 4
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index d969a5aefc2b..f80cc7e657e8 100644
--- a/config.c
+++ b/config.c
@@ -1032,10 +1032,11 @@ static int do_config_from(struct config_source *top,
config_fn_t fn, void *data)
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
- int ret;
- FILE *f = fopen(filename, r);
+ int from_stdin = !strcmp(filename, -);
+ int ret = -1;
+ FILE *f;
- ret = -1;
+ f = from_stdin ? stdin : fopen(filename, r);
if (f) {
struct config_source top;
@@ -1048,7 +1049,8 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
ret = do_config_from(top, fn, data);
- fclose(f);
+ if (!from_stdin)
+ fclose(f);
}
return ret;
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 967359344dab..f1a63075e34f 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -484,6 +484,10 @@ test_expect_success 'alternative GIT_CONFIG (--file)' '
test_cmp expect output
'
+test_expect_success 'alternative GIT_CONFIG (--file=-)' '
+ git config --file - -l other-config output
+ test_cmp expect output
+'
test_expect_success 'refer config from subdirectory' '
mkdir x
(
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: git_config_from_file(): handle - filename as stdin
Kirill A. Shutemov kir...@shutemov.name writes: The patch extends git config --file interface to allow read config from stdin. Thanks. The external interface proposed by this change that behaves the way your new test expects is a good addition to the system. I would describe it as: Subject: config: teach git config --file - to read from the standard input I however think the patch implements it at the level that is too low in the callchain. It will affect a lot more than the dash given to git config --file -. Fortunately, it does not make it possible for users to make this mistake [include] path = - and scratch their heads, wondering why git config is not answering until they hit ^D. But that is _only_ because we check if a file whose name is - actually exists in the current directory before falling into this codepath (and usually no such file exists). If such a funnily-named file does exist, we read from that file, not the standard input. So that include codepath happens to be safe, but who knows what dragons lie in other codepaths that call this function. I recall that an earlier implementation of git diff --no-index that made - read one side to be compared from the standard input had exactly the same issue of comparing filename with -, which we had to fix with code reorganization recently. I'd prefer to see this update to git config --file - done the right way from the start. diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 967359344dab..f1a63075e34f 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -484,6 +484,10 @@ test_expect_success 'alternative GIT_CONFIG (--file)' ' test_cmp expect output ' +test_expect_success 'alternative GIT_CONFIG (--file=-)' ' + git config --file - -l other-config output Please leave no SP between redirection operator and its file, i.e. git config --file - --list other-config output Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: git_config_from_file(): handle - filename as stdin
On Fri, Feb 14, 2014 at 10:27:11AM -0800, Junio C Hamano wrote:
Kirill A. Shutemov kir...@shutemov.name writes:
The patch extends git config --file interface to allow read config from
stdin.
Thanks. The external interface proposed by this change that behaves
the way your new test expects is a good addition to the system. I
would describe it as:
Subject: config: teach git config --file - to read from the standard input
I however think the patch implements it at the level that is too low
in the callchain. It will affect a lot more than the dash given to
git config --file -. Fortunately, it does not make it possible
for users to make this mistake
[include]
path = -
and scratch their heads, wondering why git config is not answering
until they hit ^D. But that is _only_ because we check if a file
whose name is - actually exists in the current directory before
falling into this codepath (and usually no such file exists). If
such a funnily-named file does exist, we read from that file, not
the standard input. So that include codepath happens to be safe,
but who knows what dragons lie in other codepaths that call this
function.
I recall that an earlier implementation of git diff --no-index
that made - read one side to be compared from the standard input
had exactly the same issue of comparing filename with -, which we
had to fix with code reorganization recently. I'd prefer to see
this update to git config --file - done the right way from the
start.
Okay, reworked version is below. It's slightly more invasive then the
original.
From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
From: Kirill A. Shutemov kir...@shutemov.name
Date: Fri, 14 Feb 2014 21:59:39 +0200
Subject: [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.
Signed-off-by: Kirill A. Shutemov kir...@shutemov.name
---
builtin/config.c | 39 ++-
cache.h| 1 +
config.c | 41 +++--
t/t1300-repo-config.sh | 17 +++--
4 files changed, 69 insertions(+), 29 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 92ebf23f0a9a..625f914c44a0 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static char key_delim = ' ';
static char term = '\n';
static int use_global_config, use_system_config, use_local_config;
+static int use_stdin;
static const char *given_config_file;
static const char *given_config_blob;
static int actions, types;
@@ -224,7 +225,7 @@ static int get_value(const char *key_, const char *regex_)
}
git_config_with_options(collect_config, values,
- given_config_file, given_config_blob,
+ use_stdin, given_config_file, given_config_blob,
respect_includes);
ret = !values.nr;
@@ -309,7 +310,7 @@ static void get_color(const char *def_color)
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
- given_config_file, given_config_blob,
+ use_stdin, given_config_file, given_config_blob,
respect_includes);
if (!get_color_found def_color)
@@ -339,7 +340,7 @@ static int get_colorbool(int print)
get_diff_color_found = -1;
get_color_ui_found = -1;
git_config_with_options(git_get_colorbool_config, NULL,
- given_config_file, given_config_blob,
+ use_stdin, given_config_file, given_config_blob,
respect_includes);
if (get_colorbool_found 0) {
@@ -362,8 +363,11 @@ static int get_colorbool(int print)
return get_colorbool_found ? 0 : 1;
}
-static void check_blob_write(void)
+static void check_write(void)
{
+ if (use_stdin)
+ die(writing to stdin is not supported);
+
if (given_config_blob)
die(writing config blobs is not supported);
}
@@ -435,7 +439,8 @@ static int get_urlmatch(const char *var, const char *url)
}
git_config_with_options(urlmatch_config_entry, config,
- given_config_file, NULL, respect_includes);
+ use_stdin, given_config_file, NULL,
+ respect_includes);
for_each_string_list_item(item, values) {
struct urlmatch_current_candidate_value *matched = item-util;
@@ -476,6 +481,11 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_config_usage,
builtin_config_options);
}
+ if
Re: [PATCH] config: git_config_from_file(): handle - filename as stdin
Kirill A. Shutemov kir...@shutemov.name writes:
On Fri, Feb 14, 2014 at 10:27:11AM -0800, Junio C Hamano wrote:
...
I recall that an earlier implementation of git diff --no-index
that made - read one side to be compared from the standard input
had exactly the same issue of comparing filename with -, which we
had to fix with code reorganization recently. I'd prefer to see
this update to git config --file - done the right way from the
start.
Okay, reworked version is below. It's slightly more invasive then the
original.
Thanks.
It looks more invasive mostly because you renamed check_blob_write()
to check_write(). While I think that rename is a right thing to do
(it used to be if we are attempting to write to blob, signal an
error, but we could have called it check_write(), meaning we are
attempting to write; error out if that should not be allowed), it
would have been much easier to review and comment if this were done
as a separate clean-up preparatory patch. It wouldn't have had to
touch this many lines, I would think.
And clean-up existing code as a separate step, without changing the
behaviour, before starting to work on a new feature is actively
encouraged around here.
From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
From: Kirill A. Shutemov kir...@shutemov.name
Date: Fri, 14 Feb 2014 21:59:39 +0200
Subject: [PATCH] config: teach git config --file - to read from the standard
input
I do not see a need for these four lines in your e-mail. All the
necessary information is already in your e-mail header. Please drop
them, perhaps except for the Subject: in-body header.
If you are going to have a discussion and then your proposed patch,
please do it this way (without the 8-space indentation I added for
illustration) using the -- 8 -- scissors line:
...
Okay, reworked version is below.
-- 8 --
Subject: config: teach git config --file - to read from the standard
input
Extend git config --file interface to allow reading from
the standard input
Editing or setting value is an error.
Signed-off-by: ...
---
diffstat and patch here
The in-body header Subject: is there to let you retitle the
commit.
Editing stdin or setting value in stdin is an error.
Good thinking. Even comes with tests to cover these cases. Good.
diff --git a/builtin/config.c b/builtin/config.c
index 92ebf23f0a9a..625f914c44a0 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -21,6 +21,7 @@ static char key_delim = ' ';
static char term = '\n';
static int use_global_config, use_system_config, use_local_config;
+static int use_stdin;
static const char *given_config_file;
static const char *given_config_blob;
If we are changing the function signature of git_config_with_options()
anyway, would it be even a better idea to introduce something like:
struct config_source {
int use_stdin;
const char *config_file;
const char *config_blob;
};
static struct config_source given_config_source;
and have one _fewer_ parameter to the function, instead of adding
one extra parameter?
diff --git a/config.c b/config.c
index d969a5aefc2b..53dd39f0b9ef 100644
--- a/config.c
+++ b/config.c
@@ -1030,24 +1030,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;
+
+ return do_config_from(top, fn, data);
+}
- 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;
+static int git_config_from_stdin(config_fn_t fn, void *data)
+{
+ return do_config_from_file(fn, stdin, stdin, data);
+}
So the above callchain will set top.name to the string stdin.
How would that interact with the code in handle_path_include() that
makes sure that relative config includes are only allowed in file
with known location?
Other than that, I didn't spot any issue in this round looks. It
seems to be almost there.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
