Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type

[Lars Schneider <larsxschneider@gmail.com>]

On 13 Feb 2016, at 18:24, Jeff King <peff@peff.net> wrote:

> On Sat, Feb 13, 2016 at 03:24:15PM +0100, larsxschneider@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> Use the config type to print more detailed error messages that inform
>> the user about the origin of a config error (file, stdin, blob).
> 
> This looks OK overall. A few minor nits...
> 
>> @@ -1104,6 +1106,7 @@ int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
>> 	top.u.buf.buf = buf;
>> 	top.u.buf.len = len;
>> 	top.u.buf.pos = 0;
>> +	top.type = "blob";
>> 	top.name = name;
>> 	top.path = NULL;
>> 	top.die_on_error = 0;
> 
> This function is about reading config from a memory buffer. The only reason
> we do so _now_ is when reading from a blob, but I think it is laying a
> trap for somebody who wants to reuse the function later.
> 
> Should git_config_from_buf() take a "type" parameter, and
> git_config_from_blob_sha1() pass in "blob"?
Haha, fun fact: this was how I implemented it initially. Because of that
I noticed that "submodule-config.c" also uses "git_config_from_buf" :-)

However, then I thought the list wouldn't like it if I change the
interfaces. I will add the type parameter, again.


> 
>> @@ -1066,7 +1067,8 @@ static int do_config_from_file(config_fn_t fn,
>> 	struct config_source top;
>> 
>> 	top.u.file = f;
>> -	top.name = name;
>> +	top.type = path ? "file" : "stdin";
>> +	top.name = name ? name : "";
>> 	top.path = path;
>> 	top.die_on_error = 1;
>> 	top.do_fgetc = config_file_fgetc;
>> @@ -1078,7 +1080,7 @@ static int do_config_from_file(config_fn_t fn,
>> 
>> static int git_config_from_stdin(config_fn_t fn, void *data)
>> {
>> -	return do_config_from_file(fn, "<stdin>", NULL, stdin, data);
>> +	return do_config_from_file(fn, NULL, NULL, stdin, data);
>> }
> 
> Likewise here, we make assumptions in do_config_from_file() about what
> the NULL path means. I think this is less likely to be a problem than
> the other case, but it seems like it would be cleaner for "file" or
> "stdin" to come from the caller, which knows for sure what we are doing.
> 
> Similarly, I think git_config_from_stdin() can simply pass in an empty
> name rather than NULL to avoid do_config_from_file() having to fix it
> up.
OK


> 
>> +test_expect_success 'invalid stdin config' '
>> +	echo "fatal: bad config line 1 in stdin " >expect &&
>> +	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> +	test_cmp expect output
>> +'
> 
> The original would have been "bad config file line 1 in <stdin>"; I
> think this is an improvement to drop the "file" here.


Thanks,
Lars