{"thread":{"id":"64515","subject":"`git config get --type=path` results in segmentation fault on value starting with `:(optional)`","startedAt":"2025-11-20T06:46:53Z","lastAt":"2025-11-26T15:13:51Z","messageCount":7,"participants":["Han Jiang","Jeff King","D. Ben Knoble","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"531038","messageId":"CANrWfmQUuGKWPc6JCzeCaa9t98ag_Lyk0G_Prtd8YmqP-TiRpg@mail.gmail.com","threadId":"64515","inReplyTo":null,"subject":"`git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Han Jiang","fromEmail":"jhcarl0814@gmail.com","sentAt":"2025-11-20T06:46:42Z","receivedAt":"2025-11-20T06:46:53Z","isPatch":false,"sender":{"key":"jhcarl0814@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5171262?v=4"},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\ngit -c 'section.key-path=/nonexistent' config get --show-origin\n--show-scope --all --type=path 'section.key-path'\ngit -c 'section.key-path=:(optional)/nonexistent' config get\n--show-origin --show-scope --all --type=path 'section.key-path'\n\nWhat did you expect to happen? (Expected behavior)\n\n1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n2nd command outputs nothing, $?=1;\n\nWhat happened instead? (Actual behavior)\n\n1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n2nd command outputs \"Segmentation fault\", $?=139;\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.52.0.windows.1\ncpu: x86_64\nbuilt from commit: 2912d8e9b8253723974b7baf1c890273b1a1c5bd\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: D:/git-sdk-64-build-installers/usr/bin/sh\nrust: disabled\nfeature: fsmonitor--daemon\nlibcurl: 8.17.0\nOpenSSL: OpenSSL 3.5.4 30 Sep 2025\nzlib: 1.3.1\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\ndefault-ref-format: files\ndefault-hash: sha1\nuname: Windows 10.0 26200\ncompiler info: gnuc: 15.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"531050","messageId":"20251120075019.GA1283645@coredump.intra.peff.net","threadId":"64515","inReplyTo":"CANrWfmQUuGKWPc6JCzeCaa9t98ag_Lyk0G_Prtd8YmqP-TiRpg@mail.gmail.com","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-20T07:50:19Z","receivedAt":"2025-11-20T07:50:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 20, 2025 at 07:46:42PM +1300, Han Jiang wrote:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> git -c 'section.key-path=/nonexistent' config get --show-origin\n> --show-scope --all --type=path 'section.key-path'\n> git -c 'section.key-path=:(optional)/nonexistent' config get\n> --show-origin --show-scope --all --type=path 'section.key-path'\n> \n> What did you expect to happen? (Expected behavior)\n> \n> 1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n> 2nd command outputs nothing, $?=1;\n> \n> What happened instead? (Actual behavior)\n> \n> 1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n> 2nd command outputs \"Segmentation fault\", $?=139;\n\nThe issue is that git_config_pathname(), when it sees the \":(optional)\"\nmarker, may return success (0) to the caller without actually setting\nthe \"dest\" parameter. So if we are lucky, we get a NULL and segfault,\nbut we may get any random data from the uninitialized pointer. Here's\nanother caller which exhibits similar problems:\n\n  $ git -c blame.ignorerevsfile=':(optional)foo' blame\n  double free or corruption (out)\n  Aborted                    git -c blame.ignorerevsfile=':(optional)foo' blame\n\nThis is all due to 749d6d166d (config: values of pathname type can be\nprefixed with :(optional), 2025-09-28), which changed the contract for\ngit_config_pathname(). Before that patch, if the function returned 0,\nthen \"dest\" was guaranteed to point to a string. Now the caller must:\n\n  - set the dest parameter to some known value like NULL before the call\n\n  - after seeing success, check whether dest points to a string (if they\n    want to know whether we actually got a path).\n\nThis more or less[*] does the right thing when the dest points to a\nstatic global, and we call it from a config callback. In that case the\ndestination is initialized to NULL, and anybody who looks at the\nvariables assumes that NULL means \"it was never set at all\". And that's\nthe case for commit.template, which is what the test from 749d6d166d\ncovers.\n\nBut many other callers are broken. E.g., blame.ignorerevsfile does this:\n\n          if (!strcmp(var, \"blame.ignorerevsfile\")) {\n                  char *str;\n                  int ret;\n  \n                  ret = git_config_pathname(&str, var, value);\n                  if (ret)\n                          return ret;\n                  string_list_insert(&ignore_revs_file_list, str);\n                  free(str);\n                  return 0;\n          }\n\nwhich tries to insert (and then free!) uninitialized bytes from \"str\".\nLikewise git-config does:\n\n                  } else if (opts->type == TYPE_PATH) {\n                          char *v;\n                          if (git_config_pathname(&v, key_, value_) < 0)\n                                  return -1;\n                          strbuf_addstr(buf, v);\n                          free((char *)v);\n\t\t  }[...]\n\nThose (and some others) all need to be updated to the new semantics.\nSomething like this would fix the blame one:\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 2703820258..15d719aec3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -733,13 +733,14 @@ static int git_blame_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"blame.ignorerevsfile\")) {\n-\t\tchar *str;\n+\t\tchar *str = NULL;\n \t\tint ret;\n \n \t\tret = git_config_pathname(&str, var, value);\n \t\tif (ret)\n \t\t\treturn ret;\n-\t\tstring_list_insert(&ignore_revs_file_list, str);\n+\t\tif (str)\n+\t\t\tstring_list_insert(&ignore_revs_file_list, str);\n \t\tfree(str);\n \t\treturn 0;\n \t}\n\nI am tempted to say that git_config_pathname() should set the dest to\nNULL itself in this case, but it is really only half the battle (callers\nstill need to check for NULL before looking at the value).\n\nI am not sure about the git-config one, though. What should it print for\nan optional path that is not there? The empty string? Is it an error?\n\nI put a [*] above on \"more or less does the right thing\" because there's\nanother corner case, even for callers like commit.template. What should\nthis:\n\n  [commit]\n  template = :(optional)does-exist\n  template = :(optional)does-not-exist\n\nWith the current code, we will ignore the second config entry entirely,\nand the result will point to \"does-exist\". But that feels surprising to\nme. I'd expect the \"optional\" marker to set the value unconditionally,\nbut with an annotation that the entry does not need to exist. And that's\nsomething only the caller can interpret (for commit.template, it means\nsetting it back to NULL, but for blame.ignorerevsfile, it means skipping\nthe string list insertion when it's not there).\n\nI kind of wonder if git_config_pathname() ought to be returning more\ndata to the caller, like:\n\n  struct config_pathname {\n\tchar *path; /* never NULL */\n\tunsigned missing : 1;\n  };\n\nThat would change the interface of git_config_pathname(), but that would\nalso force us to make the appropriate changes in each caller.\n\n-Peff\n"},{"id":"531055","messageId":"CALnO6CDL6iixzWD4PqGvh-K-Z12zyhL0-qwfi+iaNK-n_p19qw@mail.gmail.com","threadId":"64515","inReplyTo":"20251120075019.GA1283645@coredump.intra.peff.net","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-20T14:34:03Z","receivedAt":"2025-11-20T14:34:15Z","isPatch":false,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Thu, Nov 20, 2025 at 2:52 AM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Nov 20, 2025 at 07:46:42PM +1300, Han Jiang wrote:\n>\n> > What did you do before the bug happened? (Steps to reproduce your issue)\n> > git -c 'section.key-path=/nonexistent' config get --show-origin\n> > --show-scope --all --type=path 'section.key-path'\n> > git -c 'section.key-path=:(optional)/nonexistent' config get\n> > --show-origin --show-scope --all --type=path 'section.key-path'\n> >\n> > What did you expect to happen? (Expected behavior)\n> >\n> > 1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n> > 2nd command outputs nothing, $?=1;\n> >\n> > What happened instead? (Actual behavior)\n> >\n> > 1st command outputs \"command command line:   C:/Program Files/Git/nonexistent\";\n> > 2nd command outputs \"Segmentation fault\", $?=139;\n>\n> The issue is that git_config_pathname(), when it sees the \":(optional)\"\n> marker, may return success (0) to the caller without actually setting\n> the \"dest\" parameter. So if we are lucky, we get a NULL and segfault,\n> but we may get any random data from the uninitialized pointer. Here's\n> another caller which exhibits similar problems:\n>\n>   $ git -c blame.ignorerevsfile=':(optional)foo' blame\n>   double free or corruption (out)\n>   Aborted                    git -c blame.ignorerevsfile=':(optional)foo' blame\n>\n> This is all due to 749d6d166d (config: values of pathname type can be\n> prefixed with :(optional), 2025-09-28), which changed the contract for\n> git_config_pathname(). Before that patch, if the function returned 0,\n> then \"dest\" was guaranteed to point to a string. Now the caller must:\n>\n>   - set the dest parameter to some known value like NULL before the call\n>\n>   - after seeing success, check whether dest points to a string (if they\n>     want to know whether we actually got a path).\n>\n> This more or less[*] does the right thing when the dest points to a\n> static global, and we call it from a config callback. In that case the\n> destination is initialized to NULL, and anybody who looks at the\n> variables assumes that NULL means \"it was never set at all\". And that's\n> the case for commit.template, which is what the test from 749d6d166d\n> covers.\n>\n> But many other callers are broken. E.g., blame.ignorerevsfile does this:\n>\n>           if (!strcmp(var, \"blame.ignorerevsfile\")) {\n>                   char *str;\n>                   int ret;\n>\n>                   ret = git_config_pathname(&str, var, value);\n>                   if (ret)\n>                           return ret;\n>                   string_list_insert(&ignore_revs_file_list, str);\n>                   free(str);\n>                   return 0;\n>           }\n>\n> which tries to insert (and then free!) uninitialized bytes from \"str\".\n> Likewise git-config does:\n>\n>                   } else if (opts->type == TYPE_PATH) {\n>                           char *v;\n>                           if (git_config_pathname(&v, key_, value_) < 0)\n>                                   return -1;\n>                           strbuf_addstr(buf, v);\n>                           free((char *)v);\n>                   }[...]\n>\n\nThanks for the diagnosis; just hit this myself and tracked down the same code.\n\n> Those (and some others) all need to be updated to the new semantics.\n> Something like this would fix the blame one:\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 2703820258..15d719aec3 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -733,13 +733,14 @@ static int git_blame_config(const char *var, const char *value,\n>                 return 0;\n>         }\n>         if (!strcmp(var, \"blame.ignorerevsfile\")) {\n> -               char *str;\n> +               char *str = NULL;\n>                 int ret;\n>\n>                 ret = git_config_pathname(&str, var, value);\n>                 if (ret)\n>                         return ret;\n> -               string_list_insert(&ignore_revs_file_list, str);\n> +               if (str)\n> +                       string_list_insert(&ignore_revs_file_list, str);\n>                 free(str);\n>                 return 0;\n>         }\n>\n> I am tempted to say that git_config_pathname() should set the dest to\n> NULL itself in this case, but it is really only half the battle (callers\n> still need to check for NULL before looking at the value).\n\nYeah, unfortunately it doesn't look like string_list_insert considers\nNULL a no-op. Similarly I don't think strbuff_add can handle NULL\nbecause it calls strlen on the argument.\n\n> I am not sure about the git-config one, though. What should it print for\n> an optional path that is not there? The empty string? Is it an error?\n>\n> I put a [*] above on \"more or less does the right thing\" because there's\n> another corner case, even for callers like commit.template. What should\n> this:\n>\n>   [commit]\n>   template = :(optional)does-exist\n>   template = :(optional)does-not-exist\n>\n> With the current code, we will ignore the second config entry entirely,\n> and the result will point to \"does-exist\". But that feels surprising to\n> me. I'd expect the \"optional\" marker to set the value unconditionally,\n> but with an annotation that the entry does not need to exist. And that's\n> something only the caller can interpret (for commit.template, it means\n> setting it back to NULL, but for blame.ignorerevsfile, it means skipping\n> the string list insertion when it's not there).\n>\n> I kind of wonder if git_config_pathname() ought to be returning more\n> data to the caller, like:\n>\n>   struct config_pathname {\n>         char *path; /* never NULL */\n>         unsigned missing : 1;\n>   };\n>\n> That would change the interface of git_config_pathname(), but that would\n> also force us to make the appropriate changes in each caller.\n>\n> -Peff\n\n-- \nD. Ben Knoble\n"},{"id":"531066","messageId":"xmqq1pls8xeu.fsf@gitster.g","threadId":"64515","inReplyTo":"20251120075019.GA1283645@coredump.intra.peff.net","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-20T16:46:17Z","receivedAt":"2025-11-20T16:46:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\nThanks for analysing all of the above (omitted); I was doing the\nsame on the bus but couldn't finish it and then when I reached the\noffice, you've nicely done everything necessary ;-)\n\n> I put a [*] above on \"more or less does the right thing\" because there's\n> another corner case, even for callers like commit.template. What should\n> this:\n>\n>   [commit]\n>   template = :(optional)does-exist\n>   template = :(optional)does-not-exist\n>\n> With the current code, we will ignore the second config entry entirely,\n> and the result will point to \"does-exist\". But that feels surprising to\n> me.\n\nThe documentation says\n\n\tIf prefixed with :(optional), the configuration variable is\n\ttreated as if it does not exist, if the named path does not\n\texist.\n\nand when I wrote it, by \"the configuration variable\", I meant the\nsecond \"template = ...\" line above, not the configuration variable\ncommit.template, that the machinery pretends not to exist.  So the\nresult pointing at does-exist matches my expectation.\n\n> I kind of wonder if git_config_pathname() ought to be returning more\n> data to the caller, like:\n>\n>   struct config_pathname {\n> \tchar *path; /* never NULL */\n> \tunsigned missing : 1;\n>   };\n>\n> That would change the interface of git_config_pathname(), but that would\n> also force us to make the appropriate changes in each caller.\n\nThe problem is that there is no mechanism for the function to say\n\"success\" without setting *dest to the discovered value.  We could\nintroduce multiple kinds of \"failure\", and have callers react to the\ndifferences, but then it is like setting NULL in *dest and having\ncallers react to it, so I am not sure how much benefit we would be\ngaining by changing its interface.\n\nOn the other hand, builtin/config.c::format_config() probably needs\na richer set of return values.  When used from collect_config(), it\nneeds to be able to say \"no, pretend that the key/value pair you fed\nme did not exist\" in addition to \"that value is bogus---you have an\nerror (e.g., config_error_nonbool())\".\n"},{"id":"531246","messageId":"20251125002828.GA2353309@coredump.intra.peff.net","threadId":"64515","inReplyTo":"xmqq1pls8xeu.fsf@gitster.g","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-25T00:28:28Z","receivedAt":"2025-11-25T00:28:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 20, 2025 at 08:46:17AM -0800, Junio C Hamano wrote:\n\n> > I put a [*] above on \"more or less does the right thing\" because there's\n> > another corner case, even for callers like commit.template. What should\n> > this:\n> >\n> >   [commit]\n> >   template = :(optional)does-exist\n> >   template = :(optional)does-not-exist\n> >\n> > With the current code, we will ignore the second config entry entirely,\n> > and the result will point to \"does-exist\". But that feels surprising to\n> > me.\n> \n> The documentation says\n> \n> \tIf prefixed with :(optional), the configuration variable is\n> \ttreated as if it does not exist, if the named path does not\n> \texist.\n> \n> and when I wrote it, by \"the configuration variable\", I meant the\n> second \"template = ...\" line above, not the configuration variable\n> commit.template, that the machinery pretends not to exist.  So the\n> result pointing at does-exist matches my expectation.\n\nI confess that I did not read the documentation at all, and was only\ngoing on what I'd expect \":(optional)\" to do. So you can take what you\nwill from that. ;) It does feel to me like the user-facing behavior is\ndriven by ease of implementation, not what users would necessarily want.\nBut it probably is not worth revisiting at this point (especially\nbecause it is kind of a corner case for the distinction to matter at\nall).\n\n(I do agree that the documentation you quoted clearly covers the current\nbehavior).\n\n> > I kind of wonder if git_config_pathname() ought to be returning more\n> > data to the caller, like:\n> >\n> >   struct config_pathname {\n> > \tchar *path; /* never NULL */\n> > \tunsigned missing : 1;\n> >   };\n> >\n> > That would change the interface of git_config_pathname(), but that would\n> > also force us to make the appropriate changes in each caller.\n> \n> The problem is that there is no mechanism for the function to say\n> \"success\" without setting *dest to the discovered value.  We could\n> introduce multiple kinds of \"failure\", and have callers react to the\n> differences, but then it is like setting NULL in *dest and having\n> callers react to it, so I am not sure how much benefit we would be\n> gaining by changing its interface.\n\nIn my mind, we'd still return \"0\" as long as there was any string at all\n(i.e., the only error is the non-bool case). And then the caller would\nhave to pick the results out of the struct above. I agree that setting\n*dest to NULL is mostly equivalent to what I'm proposing. The main\nadvantages of the struct are:\n\n  1. The caller gets to actually see what the value is. This may or may\n     not be useful for stuff like format_config(). See below.\n\n  2. The interface change is a feature, since it requires examining and\n     updating each caller (enforced by the compiler).\n\n     It looks like you already produced a patch to update the existing\n     callers, and I'll assume you caught them all. It does leave any\n     topics-in-flight potentially buggy, though. As somebody who used to\n     maintain a long-running fork, and who has a years-long backlog of\n     random topics, I do not consider \"all of the branches in\n     gitster/git.git\" to necessarily be all topics in flight. ;)\n\n     (I did check all of my topics and didn't have any new callers,\n     though).\n\n> On the other hand, builtin/config.c::format_config() probably needs\n> a richer set of return values.  When used from collect_config(), it\n> needs to be able to say \"no, pretend that the key/value pair you fed\n> me did not exist\" in addition to \"that value is bogus---you have an\n> error (e.g., config_error_nonbool())\".\n\nI was thinking that we might need some way for format_config() to show\nthe original value (minus the \":(optional)\" meta-tag). The same way that\nwe may show include.path both as its own config variable, and as a\nmechanism that triggers an include. I.e., would somebody ask git-config\nabout \"commit.template\" not as a path, but as a string?\n\nBut the way to do that is to avoid saying \"--type=path\" in the first\nplace, and get the full string (including the optional tag). If we had\nsome kind of \"--type=path --show-missing-paths\" option, then we'd need\nto be able to see the missing name (like my struct proposal above). But\nwe don't, and nobody is asking for it, so I think we can punt on it for\nnow.\n\nI did wonder also if format_config() would need to roll back any output\nfor something like:\n\n  git -c foo.bar=':(optional)/no-such-file' \\\n    config --type=path --get-regexp --show-scope foo.bar\n\nwhich would show the key name and scope before even looking at the\nvalue. But because we assemble it all in a strbuf, we can just throw\naway the result.  And it looks like your patches handle that. It doesn't\nlook like the tests cover it, though.\n\nLooks your topic isn't in 'next' yet, so possibly squash this in?\n\ndiff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh\nindex 766693387f..fbbacfc67b 100755\n--- a/t/t1311-config-optional.sh\n+++ b/t/t1311-config-optional.sh\n@@ -18,7 +18,9 @@ test_expect_success 'var=:(optional)path-exists' '\n \n test_expect_success 'missing optional value is ignored' '\n \ttest_config a.path \":(optional)no-such-path\" &&\n-\ttest_must_fail git config get --path a.path >actual &&\n+\t# Using --show-scope ensures we skip writing not only the value\n+\t# but also any meta-information about the ignored key.\n+\ttest_must_fail git config get --show-scope --path a.path >actual &&\n \ttest_line_count = 0 actual\n '\n \n\n-Peff\n"},{"id":"531248","messageId":"xmqqa50budxc.fsf@gitster.g","threadId":"64515","inReplyTo":"20251125002828.GA2353309@coredump.intra.peff.net","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-25T00:57:35Z","receivedAt":"2025-11-25T00:57:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I confess that I did not read the documentation at all, and was only\n> going on what I'd expect \":(optional)\" to do. So you can take what you\n> will from that. ;) It does feel to me like the user-facing behavior is\n> driven by ease of implementation, not what users would necessarily want.\n> But it probably is not worth revisiting at this point (especially\n> because it is kind of a corner case for the distinction to matter at\n> all).\n\nHmph, I tend to disagree; this was not driven by ease of\nimplementation at all.  Rather, :(optional) cannot be an attribute\nof a variable; it is an attribute of individual setting of a variable.\n\nFor example, imagine that you want to say \"the system wide fallback\nis in this file in /etc, but you can override it with a file in your\nhome directory\", and you want to say that only once in the system\nwide configuration file so that it applies to all users, without\neach end user having to specify that they do want to override it in\ntheir Git configuration file.\n\nYou can write this in /etc/gitconfig\n\n    [default]\n\teditorConfig = /etc/editorConfig\n\teditorConfig = ':(optional)~/.editorConfig'\n\nand ask what path default.editorConfig file is.  As long as large\nenough user population agrees what the name of the file under their\n$HOME to control the behaviour, this would work better than telling\nthem \"you can override default.editorCondfig in your per-user\nconfiguration file\", as it is one fewer thing to configure.\n\nAnd this is possible only if we consider that what the system\npretends not to have seen is per :(optional) definition.\n\n> But the way to do that is to avoid saying \"--type=path\" in the first\n> place, and get the full string (including the optional tag). If we had\n> some kind of \"--type=path --show-missing-paths\" option, then we'd need\n> to be able to see the missing name (like my struct proposal above). But\n> we don't, and nobody is asking for it, so I think we can punt on it for\n> now.\n\n;-).\n\n> I did wonder also if format_config() would need to roll back any output\n> for something like:\n>\n>   git -c foo.bar=':(optional)/no-such-file' \\\n>     config --type=path --get-regexp --show-scope foo.bar\n>\n> which would show the key name and scope before even looking at the\n> value. But because we assemble it all in a strbuf, we can just throw\n> away the result.  And it looks like your patches handle that. It doesn't\n> look like the tests cover it, though.\n\nDidn't think about that case, but then we seem to be lucky ;-).\n\n> Looks your topic isn't in 'next' yet, so possibly squash this in?\n>\n> diff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh\n> index 766693387f..fbbacfc67b 100755\n> --- a/t/t1311-config-optional.sh\n> +++ b/t/t1311-config-optional.sh\n> @@ -18,7 +18,9 @@ test_expect_success 'var=:(optional)path-exists' '\n>  \n>  test_expect_success 'missing optional value is ignored' '\n>  \ttest_config a.path \":(optional)no-such-path\" &&\n> -\ttest_must_fail git config get --path a.path >actual &&\n> +\t# Using --show-scope ensures we skip writing not only the value\n> +\t# but also any meta-information about the ignored key.\n> +\ttest_must_fail git config get --show-scope --path a.path >actual &&\n>  \ttest_line_count = 0 actual\n>  '\n\nNice ;-).\n"},{"id":"531304","messageId":"20251126151349.GD4143292@coredump.intra.peff.net","threadId":"64515","inReplyTo":"xmqqa50budxc.fsf@gitster.g","subject":"Re: `git config get --type=path` results in segmentation fault on value starting with `:(optional)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-26T15:13:49Z","receivedAt":"2025-11-26T15:13:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2025 at 04:57:35PM -0800, Junio C Hamano wrote:\n\n> Hmph, I tend to disagree; this was not driven by ease of\n> implementation at all.  Rather, :(optional) cannot be an attribute\n> of a variable; it is an attribute of individual setting of a variable.\n> \n> For example, imagine that you want to say \"the system wide fallback\n> is in this file in /etc, but you can override it with a file in your\n> home directory\", and you want to say that only once in the system\n> wide configuration file so that it applies to all users, without\n> each end user having to specify that they do want to override it in\n> their Git configuration file.\n> \n> You can write this in /etc/gitconfig\n> \n>     [default]\n> \teditorConfig = /etc/editorConfig\n> \teditorConfig = ':(optional)~/.editorConfig'\n> \n> and ask what path default.editorConfig file is.  As long as large\n> enough user population agrees what the name of the file under their\n> $HOME to control the behaviour, this would work better than telling\n> them \"you can override default.editorCondfig in your per-user\n> configuration file\", as it is one fewer thing to configure.\n> \n> And this is possible only if we consider that what the system\n> pretends not to have seen is per :(optional) definition.\n\nYes, I agree that the code as-is opens up that workflow. But it forbids\nthe flipside, which is: \"the sysadmin set up a path in /etc, but I do not\never want to use that; I want to use my file if present, or nothing\".\n\nNow which is more likely, I don't know. I've never wanted to do either. ;)\n\nThe workflow I suggest would also perhaps be more elegant if there was a\nway to \"unset\" a variable. We allow that in some cases for list-like\nvariables, with an empty entry to reset the list. But usually for\nsingle-valued variables, we assume that last-one-wins is enough.\n\n-Peff\n"}]}