"Kirill A. Shutemov" <kirill@xxxxxxxxxxxxx> 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" <kirill@xxxxxxxxxxxxx>
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html