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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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

;-).

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

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