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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2025, 16:46 UTC
Message-ID: <xmqq1pls8xeu.fsf@gitster.g>
In-Reply-To: <20251120075019.GA1283645@coredump.intra.peff.net>

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

Show 11 quoted lines

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

Show 10 quoted lines

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