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:

Thanks for analysing all of the above (omitted); I was doing the
same on the bus but couldn't finish it and then when I reached the
office, you've nicely done everything necessary ;-)

> I put a [*] above on "more or less does the right thing" because there's
> another corner case, even for callers like commit.template. What should
> this:
>
>   [commit]
>   template = :(optional)does-exist
>   template = :(optional)does-not-exist
>
> With the current code, we will ignore the second config entry entirely,
> and the result will point to "does-exist". But that feels surprising to
> me.

The documentation says

	If prefixed with :(optional), the configuration variable is
	treated as if it does not exist, if the named path does not
	exist.

and when I wrote it, by "the configuration variable", I meant the
second "template = ..." line above, not the configuration variable
commit.template, that the machinery pretends not to exist.  So the
result pointing at does-exist matches my expectation.

> I kind of wonder if git_config_pathname() ought to be returning more
> data to the caller, like:
>
>   struct config_pathname {
> 	char *path; /* never NULL */
> 	unsigned missing : 1;
>   };
>
> That would change the interface of git_config_pathname(), but that would
> also force us to make the appropriate changes in each caller.

The problem is that there is no mechanism for the function to say
"success" without setting *dest to the discovered value.  We could
introduce multiple kinds of "failure", and have callers react to the
differences, but then it is like setting NULL in *dest and having
callers react to it, so I am not sure how much benefit we would be
gaining by changing its interface.

On the other hand, builtin/config.c::format_config() probably needs
a richer set of return values.  When used from collect_config(), it
needs to be able to say "no, pretend that the key/value pair you fed
me did not exist" in addition to "that value is bogus---you have an
error (e.g., config_error_nonbool())".