From: Junio C Hamano Date: Thu, 20 Nov 2025 16:46:17 GMT Subject: Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)` Message-ID: In-Reply-To: <20251120075019.GA1283645@coredump.intra.peff.net> Jeff King 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())".