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

[Junio C Hamano <gitster@pobox.com>]

"Kirill A. Shutemov" <kirill@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" <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.

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