From: Lars Schneider <larsxschneider@gmail.com>
Date: Sun, 07 Feb 2016 19:44:50 GMT
Subject: Re: [PATCH v1] config: add '--sources' option to print the source of a config value
Message-ID: <278405D6-873C-4E73-965B-543D66A893D7@gmail.com>
In-Reply-To: <20160205135855.GA19154@sigill.intra.peff.net>


On 05 Feb 2016, at 14:58, Jeff King <peff@peff.net> wrote:

> On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:
> 
>>> I'm not sure returning here is the best idea. We won't have a config
>>> filename if we are reading from "-c", but if we return early from this
>>> function, it parses differently than every other line. E.g., with your
>>> patch, if I do:
>>> 
>>>  git config -c foo.bar=true config --sources --list
>>> 
>>> I'll get:
>>> 
>>>  /home/peff/.gitconfig <tab> user.name=Jeff King
>>>  /home/peff/.gitconfig <tab> user.email=peff@peff.net
>>>  ...etc...
>>>  foo.bar=true
>>> 
>>> If somebody is parsing this as a tab-delimited list, then instead of the
>>> source field for that line being empty, it is missing (and it looks like
>>> "foo.bar=true" is the source file). I think it would be more friendly to
>>> consumers of the output to have a blank (i.e., set "fn" to the empty
>>> string and continue in the function).
>> 
>> Or to come up with a special string to denote config values specified on the
>> command line. Maybe somehting like
>> 
>>    <command line> <tab> foo.bar=true
>> 
>> I acknowledge that "<command line>" would be a valid filename on some
>> filesystems, but I think the risk is rather low that someone would actually
>> be using that name for a Git config file.
> 
> Yeah, I agree it's unlikely. And the output is already ambiguous, as the
> first field could be a blob (though I guess the caller knows if they
> passed "--blob" or not). If we really wanted an unambiguous output, we
> could have something like "file:...", "blob:...", etc. But that's a bit
> less readable for humans, and I don't think solves any real-world
> problems.
> 
> So I think it would be OK to use "<command line>" here, as long as the
> token is documented.
Sounds good to me. I'll add it!

> Are there any other reasons why current_config_filename() would return
> NULL, besides command-line config? I don't think so. It looks like we
> can read config from stdin, but we use the token "<stdin>" there for the
> name field (another ambiguity!).
During my testing I haven't found any other case either. To be honest
I didn't know the "stdin" way to set the config! I added a test case for 
that, too!

Thanks,
Lars