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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jeff King <peff@xxxxxxxx> 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).

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]