Re: [PATCH] config: git_config_from_file(): handle "-" filename as stdin

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 14, 2014, 20:35 UTC
Message-ID: <xmqqzjlth1q0.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140214200435.GA13633@node.dhcp.inet.fi>

"Kirill A. Shutemov" <kirill@shutemov.name> writes:

Show 11 quoted lines

> 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.

Show 5 quoted lines

> From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill@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.

Show 11 quoted lines

> 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?

Show 39 quoted lines

> 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.