Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2017, 16:40 UTC
Message-ID: <xmqqefun96y6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170613114254.oo6r6oqwtchvjuim@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:
>
>> > As you probably guessed, I had tried that first and then figured that if
>> > I needed to keep the config_key_is_valid() test anyway, I could just as
>> > well keep the strbuf around for later use.
>> > 
>> > Will change the code,
>> 
>> Alas, I won't change the code after all.
>> 
>> It is really tempting to avoid the extra strbuf, but then the error
>> message would change from
>> 
>> 	error: missing value for 'alias.br'
>> 
>> to
>> 
>> 	error: missing value for 'br'
>> 
>> which is of course no good at all.
>> 
>> And since I already have to keep that strbuf, I'll simply keep the
>> config_key_is_valid() guard, too (because why not).
>
> Oof, yeah, that is definitely worse. I'm fine with keeping both parts.

When you replace Dscho's "compare 'var' with 'alias.br' that is in strbuf naively with the "skip-prefix and compare with br" without changing anything else, i.e.

    if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
	return git_config_string((const char **)&data->v, key, value);

it would cause the "br" to be fed to git_config_string() and result in problem reported for "br", not "alias.br".

But this can be trivially fixed by passing "var" instead of "key" to git_config_string(), no? Am I mistaken?