Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin
"Kirill A. Shutemov" 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" > 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, data); > +} So the above callchain will set top.name to the string "". 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 i
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" 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" 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 --- 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); }
Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin
"Kirill A. Shutemov" 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 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