From: Jeff King Date: Tue, 25 Nov 2025 00:28:28 GMT Subject: Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)` Message-ID: <20251125002828.GA2353309@coredump.intra.peff.net> In-Reply-To: On Thu, Nov 20, 2025 at 08:46:17AM -0800, Junio C Hamano wrote: > > 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 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). (I do agree that the documentation you quoted clearly covers the current behavior). > > 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. In my mind, we'd still return "0" as long as there was any string at all (i.e., the only error is the non-bool case). And then the caller would have to pick the results out of the struct above. I agree that setting *dest to NULL is mostly equivalent to what I'm proposing. The main advantages of the struct are: 1. The caller gets to actually see what the value is. This may or may not be useful for stuff like format_config(). See below. 2. The interface change is a feature, since it requires examining and updating each caller (enforced by the compiler). It looks like you already produced a patch to update the existing callers, and I'll assume you caught them all. It does leave any topics-in-flight potentially buggy, though. As somebody who used to maintain a long-running fork, and who has a years-long backlog of random topics, I do not consider "all of the branches in gitster/git.git" to necessarily be all topics in flight. ;) (I did check all of my topics and didn't have any new callers, though). > 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())". I was thinking that we might need some way for format_config() to show the original value (minus the ":(optional)" meta-tag). The same way that we may show include.path both as its own config variable, and as a mechanism that triggers an include. I.e., would somebody ask git-config about "commit.template" not as a path, but as a string? 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. 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 ' -Peff