Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2025, 00:57 UTC
Message-ID: <xmqqa50budxc.fsf@gitster.g>
In-Reply-To: <20251125002828.GA2353309@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> I confess that I did not read the documentation at all, and was only
> going on what I'd expect ":(optional)" to do. So you can take what you
> will from that. ;) It does feel to me like the user-facing behavior is
> driven by ease of implementation, not what users would necessarily want.
> But it probably is not worth revisiting at this point (especially
> because it is kind of a corner case for the distinction to matter at
> all).

Hmph, I tend to disagree; this was not driven by ease of implementation at all. Rather, :(optional) cannot be an attribute of a variable; it is an attribute of individual setting of a variable.

For example, imagine that you want to say "the system wide fallback is in this file in /etc, but you can override it with a file in your home directory", and you want to say that only once in the system wide configuration file so that it applies to all users, without each end user having to specify that they do want to override it in their Git configuration file.

You can write this in /etc/gitconfig

    [default]
	editorConfig = /etc/editorConfig
	editorConfig = ':(optional)~/.editorConfig'

and ask what path default.editorConfig file is. As long as large enough user population agrees what the name of the file under their $HOME to control the behaviour, this would work better than telling them "you can override default.editorCondfig in your per-user configuration file", as it is one fewer thing to configure.

And this is possible only if we consider that what the system pretends not to have seen is per :(optional) definition.

Show 6 quoted lines

> But the way to do that is to avoid saying "--type=path" in the first
> place, and get the full string (including the optional tag). If we had
> some kind of "--type=path --show-missing-paths" option, then we'd need
> to be able to see the missing name (like my struct proposal above). But
> we don't, and nobody is asking for it, so I think we can punt on it for
> now.

;-).

Show 10 quoted lines

> I did wonder also if format_config() would need to roll back any output
> for something like:
>
>   git -c foo.bar=':(optional)/no-such-file' \
>     config --type=path --get-regexp --show-scope foo.bar
>
> which would show the key name and scope before even looking at the
> value. But because we assemble it all in a strbuf, we can just throw
> away the result.  And it looks like your patches handle that. It doesn't
> look like the tests cover it, though.

Didn't think about that case, but then we seem to be lucky ;-).

Show 16 quoted lines

> Looks your topic isn't in 'next' yet, so possibly squash this in?
>
> diff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh
> index 766693387f..fbbacfc67b 100755
> --- a/t/t1311-config-optional.sh
> +++ b/t/t1311-config-optional.sh
> @@ -18,7 +18,9 @@ test_expect_success 'var=:(optional)path-exists' '
>  
>  test_expect_success 'missing optional value is ignored' '
>  	test_config a.path ":(optional)no-such-path" &&
> -	test_must_fail git config get --path a.path >actual &&
> +	# Using --show-scope ensures we skip writing not only the value
> +	# but also any meta-information about the ignored key.
> +	test_must_fail git config get --show-scope --path a.path >actual &&
>  	test_line_count = 0 actual
>  '

Nice ;-).