Re: [PATCH] config: add show_err flag to git_config_parse_key()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 11, 2015, 18:47 UTC
Message-ID: <xmqq386cuvxl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150211002754.GC30561@peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> On Wed, Feb 11, 2015 at 01:15:05AM +0530, Tanay Abhra wrote:
>
>> I just saw your mail late in the night (I didn't had net for a week).
>> This patch just squelches the error message, I will take a better
>> look tomorrow morning.
>
> Thanks, this is probably a good first step. We can worry about making
> the config look actually _work_ as the next step (which does not even
> have to happen right now; it is not like it hasn't been this way since
> the very beginning of git).

I agree this is probably a good first step in the right direction. As to the implementation, there are a few minor things I would change, but they are both minor:

 - "defective" may want to be a bit more descriptive to clarify what
   kind fo defect is undesired. In the context of this patch, I
   think Tanay meant (syntactically) "malformed", perhaps?

 - "int show_err" should be "unsigned flags" with its bit 01 defined
   to be used as QUIET bit.

Show 6 quoted lines

> Another option for this first step would be to actually make
> git_config_parse_key permissive, rather than just squelching the
> error.  That is, to actually look up pager.under_score rather than
> silently erroring out with an invalid key whenever we are reading
> (whereas on the writing side, we _do_ want to make sure we enforce
> syntactic validity).  I doubt it matters, much, though.

Sensible.

Show 14 quoted lines

> I was tempted to also add something like:
>
>   test_expect_failure TTY 'command with underscores can override pager' '
> 	test_config pager.under_score "sed s/^/paged://" &&
> 	git --exec-path=. under_score >actual &&
> 	echo paged:ok >expect &&
> 	test_cmp expect actual
>   '
>
> but I am not sure it is worth adding the test, even as a placeholder.
> Unless we are planning to relax the config syntax, the correct spelling
> is more like "pager.under_score.command". It's probably better to just
> add the test along with the code when we know what the final form will
> look like.

Concurred.