{"thread":{"id":"64518","subject":"[PATCH] config: really pretend missing :(optional) value is not there","startedAt":"2025-11-20T19:34:52Z","lastAt":"2025-11-20T22:40:56Z","messageCount":4,"participants":["Junio C Hamano","D. Ben Knoble"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"531073","messageId":"xmqqms4g7b1h.fsf@gitster.g","threadId":"64518","inReplyTo":null,"subject":"[PATCH] config: really pretend missing :(optional) value is not there","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-20T19:34:50Z","receivedAt":"2025-11-20T19:34:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier we added support for a value spelled as \":(optional)path\"\nfor configuration variables whose values are of type \"path\", with\nthe documented semantics \"if the path is missing, behave as if such\na variable definition is not even there.\"\n\nThis has worked OK for code paths that reads configuration files and\nstores the configured value as a string, where NULL in such a string\nis treated as if the setting is not there, left as the default.\n\nHowever, there are other code paths that do not _ignore_ such NULL\nvalues and misbehave.  \"git config get --path\" is one of them.\n\nWhen git_config_pathname() helper function finds that the value of\nthe variable is an optional path *and* the path is missing, it\nleaves the destination pointer intact (which usually is left to\nNULL) and returns 0 to signal a success.  format_config() helper\nhowever assumed that the destination pointer always gets a string,\nwhich no longer is the case, and segfaulted.\n\nMake sure that git_config_pathname() clears the destination pointer\nin such a case, and teach format_config() to react to the condition\nby returning 1 (which is different from 0 that is a normal success\nand negative that is an error) to its callers.  Adjust the callers\nto react to this new return value that tells them to pretend as if\nthey did not even see this partcular <key, value> pair.\n\nReported-by: Han Jiang <jhcarl0814@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is only about \"git config get --path\".  Another patch for\n   the rest of the callers of git_config_pathname() will follow in a\n   separate message.\n\n builtin/config.c           | 45 ++++++++++++++++++++++++++++++--------\n config.c                   |  1 +\n t/t1311-config-optional.sh | 36 ++++++++++++++++++++++++++++++\n 3 files changed, 73 insertions(+), 9 deletions(-)\n create mode 100755 t/t1311-config-optional.sh\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 75852bd79d..e58eb6a176 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -261,6 +261,12 @@ struct strbuf_list {\n \tint alloc;\n };\n \n+/*\n+ * Format the configuration key-value pair (`key_`, `value_`) and\n+ * append it into strbuf `buf`.  Returns a negative value on failure,\n+ * 0 on success, 1 on a missing optional value (i.e., telling the\n+ * caller to pretend that <key_,value_> did not exist).\n+ */\n static int format_config(const struct config_display_options *opts,\n \t\t\t struct strbuf *buf, const char *key_,\n \t\t\t const char *value_, const struct key_value_info *kvi)\n@@ -299,7 +305,10 @@ static int format_config(const struct config_display_options *opts,\n \t\t\tchar *v;\n \t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n \t\t\t\treturn -1;\n-\t\t\tstrbuf_addstr(buf, v);\n+\t\t\tif (v)\n+\t\t\t\tstrbuf_addstr(buf, v);\n+\t\t\telse\n+\t\t\t\treturn 1; /* :(optional)no-such-file */\n \t\t\tfree((char *)v);\n \t\t} else if (opts->type == TYPE_EXPIRY_DATE) {\n \t\t\ttimestamp_t t;\n@@ -344,6 +353,7 @@ static int collect_config(const char *key_, const char *value_,\n \tstruct collect_config_data *data = cb;\n \tstruct strbuf_list *values = data->values;\n \tconst struct key_value_info *kvi = ctx->kvi;\n+\tint status;\n \n \tif (!(data->get_value_flags & GET_VALUE_KEY_REGEXP) &&\n \t    strcmp(key_, data->key))\n@@ -361,8 +371,15 @@ static int collect_config(const char *key_, const char *value_,\n \tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n \tstrbuf_init(&values->items[values->nr], 0);\n \n-\treturn format_config(data->display_opts, &values->items[values->nr++],\n-\t\t\t     key_, value_, kvi);\n+\tstatus = format_config(data->display_opts, &values->items[values->nr++],\n+\t\t\t       key_, value_, kvi);\n+\tif (status < 0)\n+\t\treturn status;\n+\tif (status) {\n+\t\tstrbuf_release(&values->items[--values->nr]);\n+\t\tstatus = 0;\n+\t}\n+\treturn status;\n }\n \n static int get_value(const struct config_location_options *opts,\n@@ -438,15 +455,23 @@ static int get_value(const struct config_location_options *opts,\n \tif (!values.nr && display_opts->default_value) {\n \t\tstruct key_value_info kvi = KVI_INIT;\n \t\tstruct strbuf *item;\n+\t\tint status;\n \n \t\tkvi_from_param(&kvi);\n \t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n \t\titem = &values.items[values.nr++];\n \t\tstrbuf_init(item, 0);\n-\t\tif (format_config(display_opts, item, key_,\n-\t\t\t\t  display_opts->default_value, &kvi) < 0)\n+\n+\t\tstatus = format_config(display_opts, item, key_,\n+\t\t\t\t       display_opts->default_value, &kvi);\n+\t\tif (status < 0)\n \t\t\tdie(_(\"failed to format default config value: %s\"),\n \t\t\t    display_opts->default_value);\n+\t\tif (status) {\n+\t\t\t/* default was a missing optional value */\n+\t\t\tvalues.nr--;\n+\t\t\tstrbuf_release(item);\n+\t\t}\n \t}\n \n \tret = !values.nr;\n@@ -714,11 +739,13 @@ static int get_urlmatch(const struct config_location_options *opts,\n \tfor_each_string_list_item(item, &values) {\n \t\tstruct urlmatch_current_candidate_value *matched = item->util;\n \t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint status;\n \n-\t\tformat_config(&display_opts, &buf, item->string,\n-\t\t\t      matched->value_is_null ? NULL : matched->value.buf,\n-\t\t\t      &matched->kvi);\n-\t\tfwrite(buf.buf, 1, buf.len, stdout);\n+\t\tstatus = format_config(&display_opts, &buf, item->string,\n+\t\t\t\t       matched->value_is_null ? NULL : matched->value.buf,\n+\t\t\t\t       &matched->kvi);\n+\t\tif (!status)\n+\t\t\tfwrite(buf.buf, 1, buf.len, stdout);\n \t\tstrbuf_release(&buf);\n \n \t\tstrbuf_release(&matched->value);\ndiff --git a/config.c b/config.c\nindex f1def0dcfb..d55882c649 100644\n--- a/config.c\n+++ b/config.c\n@@ -1291,6 +1291,7 @@ int git_config_pathname(char **dest, const char *var, const char *value)\n \n \tif (is_optional && is_missing_file(path)) {\n \t\tfree(path);\n+\t\t*dest = NULL;\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh\nnew file mode 100755\nindex 0000000000..766693387f\n--- /dev/null\n+++ b/t/t1311-config-optional.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2025 Google LLC\n+#\n+\n+test_description=':(optional) paths'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'var=:(optional)path-exists' '\n+\ttest_config a.path \":(optional)path-exists\" &&\n+\t>path-exists &&\n+\techo path-exists >expect &&\n+\n+\tgit config get --path a.path >actual &&\n+\ttest_cmp expect actual\n+'\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+\ttest_line_count = 0 actual\n+'\n+\n+test_expect_success 'missing optional value is ignored in multi-value config' '\n+\ttest_when_finished \"git config unset --all a.path\" &&\n+\tgit config set --append a.path \":(optional)path-exists\" &&\n+\tgit config set --append a.path \":(optional)no-such-path\" &&\n+\t>path-exists &&\n+\techo path-exists >expect &&\n+\n+\tgit config --get --path a.path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.52.0-101-g4c43c53c49\n\n"},{"id":"531075","messageId":"xmqqikf47ajk.fsf@gitster.g","threadId":"64518","inReplyTo":"xmqqms4g7b1h.fsf@gitster.g","subject":"[PATCH] config: really treat missing optional path as not configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-20T19:45:35Z","receivedAt":"2025-11-20T19:45:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These callers expect that git_config_pathname() that returns 0 is a\nsignal that the variable they passed has a string they need to act\non.  But with the introduction of \":(optional)path\" earlier, that is\nno longer the case.  If the path specified by the configuration\nvariable is missing, their variable will get a NULL in it, and they\nneed to act on it (often, just refraining from copying it elsewhere).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/blame.c        |  3 ++-\n builtin/receive-pack.c |  5 +++--\n fetch-pack.c           |  5 +++--\n fsck.c                 | 12 +++++++-----\n gpg-interface.c        | 10 +++++++++-\n setup.c                |  2 +-\n 6 files changed, 25 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 2703820258..c39c1d3149 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -739,7 +739,8 @@ static int git_blame_config(const char *var, const char *value,\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}\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c9288a9c7e..c6e8e8346e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -177,8 +177,9 @@ static int receive_pack_config(const char *var, const char *value,\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn -1;\n-\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n-\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tif (path)\n+\t\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\t\t    fsck_msg_types.len ? ',' : '=', path);\n \t\tfree(path);\n \t\treturn 0;\n \t}\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex fe7a84bf2f..7162fd3ba2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1873,8 +1873,9 @@ int fetch_pack_fsck_config(const char *var, const char *value,\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn -1;\n-\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n-\t\t\tmsg_types->len ? ',' : '=', path);\n+\t\tif (path)\n+\t\t\tstrbuf_addf(msg_types, \"%cskiplist=%s\",\n+\t\t\t\t    msg_types->len ? ',' : '=', path);\n \t\tfree(path);\n \t\treturn 0;\n \t}\ndiff --git a/fsck.c b/fsck.c\nindex 341e100d24..cf6f7f3a61 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1369,14 +1369,16 @@ int git_fsck_config(const char *var, const char *value,\n \n \tif (strcmp(var, \"fsck.skiplist\") == 0) {\n \t\tchar *path;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\tif (git_config_pathname(&path, var, value))\n \t\t\treturn -1;\n-\t\tstrbuf_addf(&sb, \"skiplist=%s\", path);\n-\t\tfree(path);\n-\t\tfsck_set_msg_types(options, sb.buf);\n-\t\tstrbuf_release(&sb);\n+\t\tif (path) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"skiplist=%s\", path);\n+\t\t\tfree(path);\n+\t\t\tfsck_set_msg_types(options, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t}\n \t\treturn 0;\n \t}\n \ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex f680ed38c0..3e73513694 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -794,8 +794,16 @@ static int git_gpg_config(const char *var, const char *value,\n \t\tfmtname = \"ssh\";\n \n \tif (fmtname) {\n+\t\tchar *program;\n+\t\tint status;\n+\n \t\tfmt = get_format_by_name(fmtname);\n-\t\treturn git_config_pathname((char **) &fmt->program, var, value);\n+\t\tstatus = git_config_pathname(&program, var, value);\n+\t\tif (status)\n+\t\t\treturn status;\n+\t\tif (program)\n+\t\t\tfmt->program = program;\n+\t\treturn status;\n \t}\n \n \treturn 0;\ndiff --git a/setup.c b/setup.c\nindex 7086741e6c..cf47441b7b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1248,7 +1248,7 @@ static int safe_directory_cb(const char *key, const char *value,\n \t} else {\n \t\tchar *allowed = NULL;\n \n-\t\tif (!git_config_pathname(&allowed, key, value)) {\n+\t\tif (!git_config_pathname(&allowed, key, value) && allowed) {\n \t\t\tchar *normalized = NULL;\n \n \t\t\t/*\n-- \n2.52.0-101-g4c43c53c49\n\n"},{"id":"531087","messageId":"CALnO6CC0HU60F47yoE45ei7_K2_MeLRS7fihMPn+f8top7Jr7w@mail.gmail.com","threadId":"64518","inReplyTo":"xmqqms4g7b1h.fsf@gitster.g","subject":"Re: [PATCH] config: really pretend missing :(optional) value is not there","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-11-20T22:15:46Z","receivedAt":"2025-11-20T22:15:57Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Thu, Nov 20, 2025 at 2:35 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Earlier we added support for a value spelled as \":(optional)path\"\n> for configuration variables whose values are of type \"path\", with\n> the documented semantics \"if the path is missing, behave as if such\n> a variable definition is not even there.\"\n>\n> This has worked OK for code paths that reads configuration files and\n> stores the configured value as a string, where NULL in such a string\n> is treated as if the setting is not there, left as the default.\n>\n> However, there are other code paths that do not _ignore_ such NULL\n> values and misbehave.  \"git config get --path\" is one of them.\n>\n> When git_config_pathname() helper function finds that the value of\n> the variable is an optional path *and* the path is missing, it\n> leaves the destination pointer intact (which usually is left to\n> NULL) and returns 0 to signal a success.  format_config() helper\n> however assumed that the destination pointer always gets a string,\n> which no longer is the case, and segfaulted.\n>\n> Make sure that git_config_pathname() clears the destination pointer\n> in such a case, and teach format_config() to react to the condition\n> by returning 1 (which is different from 0 that is a normal success\n> and negative that is an error) to its callers.  Adjust the callers\n> to react to this new return value that tells them to pretend as if\n> they did not even see this partcular <key, value> pair.\n>\n> Reported-by: Han Jiang <jhcarl0814@gmail.com>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  * This is only about \"git config get --path\".  Another patch for\n>    the rest of the callers of git_config_pathname() will follow in a\n>    separate message.\n>\n>  builtin/config.c           | 45 ++++++++++++++++++++++++++++++--------\n>  config.c                   |  1 +\n>  t/t1311-config-optional.sh | 36 ++++++++++++++++++++++++++++++\n>  3 files changed, 73 insertions(+), 9 deletions(-)\n\nThis needs a tweak to Meson, probably in t/meson.build, for the new\ntest script. Otherwise Meson-based packages (like Gentoo) won't build.\n\n-- \nD. Ben Knoble\n"},{"id":"531089","messageId":"xmqqy0o05nuy.fsf@gitster.g","threadId":"64518","inReplyTo":"CALnO6CC0HU60F47yoE45ei7_K2_MeLRS7fihMPn+f8top7Jr7w@mail.gmail.com","subject":"Re: [PATCH] config: really pretend missing :(optional) value is not there","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-20T22:40:53Z","receivedAt":"2025-11-20T22:40:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" <ben.knoble@gmail.com> writes:\n\n>>  config.c                   |  1 +\n>>  t/t1311-config-optional.sh | 36 ++++++++++++++++++++++++++++++\n>>  3 files changed, 73 insertions(+), 9 deletions(-)\n>\n> This needs a tweak to Meson, probably in t/meson.build, for the new\n> test script. Otherwise Meson-based packages (like Gentoo) won't build.\n\nYup.\n\ndiff --git a/t/meson.build b/t/meson.build\nindex bbeba1a8d5..137c0caea0 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -182,6 +182,7 @@ integration_tests = [\n   't1308-config-set.sh',\n   't1309-early-config.sh',\n   't1310-config-default.sh',\n+  't1311-config-optional.sh',\n   't1350-config-hooks-path.sh',\n   't1400-update-ref.sh',\n   't1401-symbolic-ref.sh',\n"}]}