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

On Thu, Nov 20, 2025 at 08:46:17AM -0800, Junio C Hamano wrote:

quoted

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

quoted

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