[v3,06/12] builtin/config.c: test misuse of format_config()

Commit Message

Glen Choo June 20, 2023, 7:43 p.m. UTC

From: Glen Choo <[e-mail]>

current_config_*() functions aren't meant to be called outside of
config callbacks because they read state that is only set when iterating
through config. However, several sites in builtin/config.c are
indirectly calling these functions outside of config callbacks thanks to
the format_config() helper. Show the current, bad behavior via tests
so that the fixes in a subsequent commit will be clearer.

The misbehaving cases are:

* "git config --get-urlmatch --show-scope" results in an "unknown"
   scope, where it arguably should show the config file's scope. It's
   clear that this wasn't intended, though: we knew that
   "--get-urlmatch" couldn't show config source metadata, which is why
   "--show-origin" was marked incompatible with "--get-urlmatch" when
   it was introduced [1]. It was most likely a mistake that we allowed
   "--show-scope" to sneak through.

* Similarly, "git config --default" doesn't set config source metadata ,
  so "--show-scope" also results in "unknown", and "--show-origin"
  results in a BUG().

[1] https://lore.kernel.org/git/[e-mail]/

Signed-off-by: Glen Choo <[e-mail]>
---
 t/t1300-config.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Junio C Hamano June 20, 2023, 9:35 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

> From: Glen Choo <[e-mail]>
>
> current_config_*() functions aren't meant to be called outside of
> config callbacks because they read state that is only set when iterating
> through config.

Interesting.  And config_context is about expressing the state of
the iteration, so presumably some of these callers that do not have
config_context to pass (because they happen outside iterations) will
now (can|have to) pass NULL or something to say "I am asking format
but from outside any iteration" or something?

> +test_expect_success 'urlmatch with --show-scope' '
> +	cat >.git/config <<-\EOF &&
> +	[http "https://weak.example.com"]
> +		sslVerify = false
> +		cookieFile = /tmp/cookie.txt
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	unknown	http.cookiefile /tmp/cookie.txt
> +	unknown	http.sslverify false
> +	EOF
> +	git config --get-urlmatch --show-scope HTTP https://weak.example.com >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'urlmatch favors more specific URLs' '
>  	cat >.git/config <<-\EOF &&
>  	[http "https://example.com/"]
> @@ -2055,6 +2070,10 @@ test_expect_success '--show-origin blob ref' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success '--show-origin with --default' '
> +	test_must_fail git config --show-origin --default foo some.key
> +'
> +
>  test_expect_success '--show-scope with --list' '
>  	cat >expect <<-EOF &&
>  	global	user.global=true
> @@ -2123,6 +2142,12 @@ test_expect_success '--show-scope with --show-origin' '
>  	test_cmp expect output
>  '
>  
> +test_expect_success '--show-scope with --default' '
> +	git config --show-scope --default foo some.key >actual &&
> +	echo "unknown	foo" >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'override global and system config' '
>  	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
>  	cat >"$HOME"/.gitconfig <<-EOF &&

Glen Choo June 20, 2023, 11:06 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:

>> current_config_*() functions aren't meant to be called outside of
>> config callbacks because they read state that is only set when iterating
>> through config.
>
> Interesting.  And config_context is about expressing the state of
> the iteration, so presumably some of these callers that do not have
> config_context to pass (because they happen outside iterations) will
> now (can|have to) pass NULL or something to say "I am asking format
> but from outside any iteration" or something?

Yup, they now have to pass a default value for config_context. For .kvi,
it should be what's given by KVI_INIT (the next patch has an example).

Jonathan Tan June 23, 2023, 8:32 p.m. UTC | #3

"Glen Choo via GitGitGadget" <[e-mail]> writes:
> +test_expect_success '--show-origin with --default' '
> +	test_must_fail git config --show-origin --default foo some.key
> +'

On my machine, this fails with

  BUG: config.c:4035: current_config_origin_type called outside config callback
  /usr/local/google/home/jonathantanmy/git/t/test-lib-functions.sh: line 1067: 3255109 Aborted                 "$@" 2>&7
  test_must_fail: died by signal 6: git config --show-origin --default foo some.key

(So it indeed fails, as expected, but test_must_fail seems to not like
the exit code.)

Not sure if we need to be more precise with the exit code we're testing
for.

Jeff King June 24, 2023, 1:31 a.m. UTC | #4

On Fri, Jun 23, 2023 at 01:32:19PM -0700, Jonathan Tan wrote:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
> > +test_expect_success '--show-origin with --default' '
> > +	test_must_fail git config --show-origin --default foo some.key
> > +'
> 
> On my machine, this fails with
> 
>   BUG: config.c:4035: current_config_origin_type called outside config callback
>   /usr/local/google/home/jonathantanmy/git/t/test-lib-functions.sh: line 1067: 3255109 Aborted                 "$@" 2>&7
>   test_must_fail: died by signal 6: git config --show-origin --default foo some.key
> 
> (So it indeed fails, as expected, but test_must_fail seems to not like
> the exit code.)
> 
> Not sure if we need to be more precise with the exit code we're testing
> for.

That is test_must_fail operating as intended. We should _never_ hit a
BUG() call, no matter what garbage we get from the user or the disk. The
resolution is generally one of:

  - there really is a bug, so we should fix it

  - the invocation is garbage and expected to fail, but we should catch
    it sooner and give a nice error message, rather than getting as far
    as a BUG() call

But I didn't look at the patches to know which case this is.

-Peff

Glen Choo June 28, 2023, 5:28 p.m. UTC | #5

Jonathan Tan <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>> +test_expect_success '--show-origin with --default' '
>> +	test_must_fail git config --show-origin --default foo some.key
>> +'
>
> On my machine, this fails with
>
>   BUG: config.c:4035: current_config_origin_type called outside config callback
>   /usr/local/google/home/jonathantanmy/git/t/test-lib-functions.sh: line 1067: 3255109 Aborted                 "$@" 2>&7
>   test_must_fail: died by signal 6: git config --show-origin --default foo some.key
>
> (So it indeed fails, as expected, but test_must_fail seems to not like
> the exit code.)

Ah you're right. I was under the impression that this was doing the
right thing on MacOS, but it doesn't work there either.

I think a good way to assert that we run into BUG() (Maybe
test_match_signal on the numeric value of SIGABRT? But that sounds error
prone), so I'll either use test_expect_failure or squash this patch.

Patch

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 86bfbc2b364..fa6a8df2521 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1668,6 +1668,21 @@  test_expect_success 'urlmatch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'urlmatch with --show-scope' '
+	cat >.git/config <<-\EOF &&
+	[http "https://weak.example.com"]
+		sslVerify = false
+		cookieFile = /tmp/cookie.txt
+	EOF
+
+	cat >expect <<-EOF &&
+	unknown	http.cookiefile /tmp/cookie.txt
+	unknown	http.sslverify false
+	EOF
+	git config --get-urlmatch --show-scope HTTP https://weak.example.com >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'urlmatch favors more specific URLs' '
 	cat >.git/config <<-\EOF &&
 	[http "https://example.com/"]
@@ -2055,6 +2070,10 @@  test_expect_success '--show-origin blob ref' '
 	test_cmp expect output
 '
 
+test_expect_success '--show-origin with --default' '
+	test_must_fail git config --show-origin --default foo some.key
+'
+
 test_expect_success '--show-scope with --list' '
 	cat >expect <<-EOF &&
 	global	user.global=true
@@ -2123,6 +2142,12 @@  test_expect_success '--show-scope with --show-origin' '
 	test_cmp expect output
 '
 
+test_expect_success '--show-scope with --default' '
+	git config --show-scope --default foo some.key >actual &&
+	echo "unknown	foo" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'override global and system config' '
 	test_when_finished rm -f \"\$HOME\"/.gitconfig &&
 	cat >"$HOME"/.gitconfig <<-EOF &&