Jeff King [off-list ref] 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())".