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

[Jeff King <peff@peff.net>]

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

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.  Such a lookup would never succeed,
because the config file parser will also not allow it. So assuming the
syntactic rules here match what the config file parser does, they are at
worst redundant.

>  builtin/config.c |  2 +-
>  cache.h          |  2 +-
>  config.c         | 19 ++++++++++++-------
>  3 files changed, 14 insertions(+), 9 deletions(-)

Here's a test that can be squashed in:

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index da958a8..a28a2fd 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -447,4 +447,14 @@ test_expect_success TTY 'external command pagers override sub-commands' '
 	test_cmp expect actual
 '
 
+test_expect_success 'command with underscores does not complain' '
+	write_script git-under_score <<-\EOF &&
+	echo ok
+	EOF
+	git --exec-path=. under_score >actual 2>&1 &&
+	echo ok >expect &&
+	test_cmp expect actual
+'
+
+
 test_done

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.

-Peff