From: Lars Schneider <larsxschneider@gmail.com>
Date: Sun, 14 Feb 2016 12:26:27 GMT
Subject: Re: [PATCH v3 2/3] config: add 'type' to config_source struct that identifies config type
Message-ID: <A74A1FE2-7C03-448F-8147-BBF276322E62@gmail.com>
In-Reply-To: <xmqqio1ss4on.fsf@gitster.mtv.corp.google.com>


On 13 Feb 2016, at 22:04, Junio C Hamano <gitster@pobox.com> wrote:

> Jeff King <peff@peff.net> writes:
> 
>>> @@ -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"?
> 
> I think that is sensible.  I think s/from_buf/from_mem/ may also be
> sensible (it would match the naming used in the index_{fd,mem,...}
> functions in he hashing code).
OK, I will change that, too!


> 
>>> 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.
> 
> Makes sense.
> 
>> 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.
> 
> This too.
> 
>>> +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.
>> 
>> -Peff

Thanks,
Lars