From 770afe443784b3ec2c72d68aa509e48064942348 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 20 Nov 2025 11:32:45 -0800 Subject: [PATCH 1/3] config: mark otherwise unused function as file-scope static git_configset_get_pathname() is only used once inside config.c; we do not have to expose it as a public function. Signed-off-by: Junio C Hamano --- config.c | 2 +- config.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/config.c b/config.c index 73fc74c8fa..6552e5b0b8 100644 --- a/config.c +++ b/config.c @@ -1954,7 +1954,7 @@ int git_configset_get_maybe_bool(struct config_set *set, const char *key, int *d return 1; } -int git_configset_get_pathname(struct config_set *set, const char *key, char **dest) +static int git_configset_get_pathname(struct config_set *set, const char *key, char **dest) { const char *value; if (!git_configset_get_value(set, key, &value, NULL)) diff --git a/config.h b/config.h index 19c87fc0bc..ba426a960a 100644 --- a/config.h +++ b/config.h @@ -564,7 +564,6 @@ int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned lon int git_configset_get_bool(struct config_set *cs, const char *key, int *dest); int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest); int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest); -int git_configset_get_pathname(struct config_set *cs, const char *key, char **dest); /** * Run only the discover part of the repo_config_get_*() functions From ce1a5a22a5beefac8a52da518855b5aecc562874 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 20 Nov 2025 11:34:50 -0800 Subject: [PATCH 2/3] config: really pretend missing :(optional) value is not there Earlier we added support for a value spelled as ":(optional)path" for configuration variables whose values are of type "path", with the documented semantics "if the path is missing, behave as if such a variable definition is not even there." This has worked OK for code paths that reads configuration files and stores the configured value as a string, where NULL in such a string is treated as if the setting is not there, left as the default. However, there are other code paths that do not _ignore_ such NULL values and misbehave. "git config get --path" is one of them. When git_config_pathname() helper function finds that the value of the variable is an optional path *and* the path is missing, it leaves the destination pointer intact (which usually is left to NULL) and returns 0 to signal a success. format_config() helper however assumed that the destination pointer always gets a string, which no longer is the case, and segfaulted. Make sure that git_config_pathname() clears the destination pointer in such a case, and teach format_config() to react to the condition by returning 1 (which is different from 0 that is a normal success and negative that is an error) to its callers. Adjust the callers to react to this new return value that tells them to pretend as if they did not even see this partcular pair. Reported-by: Han Jiang Helped-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/config.c | 45 ++++++++++++++++++++++++++++++-------- config.c | 1 + t/meson.build | 1 + t/t1311-config-optional.sh | 38 ++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 9 deletions(-) create mode 100755 t/t1311-config-optional.sh diff --git a/builtin/config.c b/builtin/config.c index 59fb113b07..2b36eb7d1c 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -261,6 +261,12 @@ struct strbuf_list { int alloc; }; +/* + * Format the configuration key-value pair (`key_`, `value_`) and + * append it into strbuf `buf`. Returns a negative value on failure, + * 0 on success, 1 on a missing optional value (i.e., telling the + * caller to pretend that did not exist). + */ static int format_config(const struct config_display_options *opts, struct strbuf *buf, const char *key_, const char *value_, const struct key_value_info *kvi) @@ -299,7 +305,10 @@ static int format_config(const struct config_display_options *opts, char *v; if (git_config_pathname(&v, key_, value_) < 0) return -1; - strbuf_addstr(buf, v); + if (v) + strbuf_addstr(buf, v); + else + return 1; /* :(optional)no-such-file */ free((char *)v); } else if (opts->type == TYPE_EXPIRY_DATE) { timestamp_t t; @@ -344,6 +353,7 @@ static int collect_config(const char *key_, const char *value_, struct collect_config_data *data = cb; struct strbuf_list *values = data->values; const struct key_value_info *kvi = ctx->kvi; + int status; if (!(data->get_value_flags & GET_VALUE_KEY_REGEXP) && strcmp(key_, data->key)) @@ -361,8 +371,15 @@ static int collect_config(const char *key_, const char *value_, ALLOC_GROW(values->items, values->nr + 1, values->alloc); strbuf_init(&values->items[values->nr], 0); - return format_config(data->display_opts, &values->items[values->nr++], - key_, value_, kvi); + status = format_config(data->display_opts, &values->items[values->nr++], + key_, value_, kvi); + if (status < 0) + return status; + if (status) { + strbuf_release(&values->items[--values->nr]); + status = 0; + } + return status; } static int get_value(const struct config_location_options *opts, @@ -438,15 +455,23 @@ static int get_value(const struct config_location_options *opts, if (!values.nr && display_opts->default_value) { struct key_value_info kvi = KVI_INIT; struct strbuf *item; + int status; kvi_from_param(&kvi); ALLOC_GROW(values.items, values.nr + 1, values.alloc); item = &values.items[values.nr++]; strbuf_init(item, 0); - if (format_config(display_opts, item, key_, - display_opts->default_value, &kvi) < 0) + + status = format_config(display_opts, item, key_, + display_opts->default_value, &kvi); + if (status < 0) die(_("failed to format default config value: %s"), display_opts->default_value); + if (status) { + /* default was a missing optional value */ + values.nr--; + strbuf_release(item); + } } ret = !values.nr; @@ -706,11 +731,13 @@ static int get_urlmatch(const struct config_location_options *opts, for_each_string_list_item(item, &values) { struct urlmatch_current_candidate_value *matched = item->util; struct strbuf buf = STRBUF_INIT; + int status; - format_config(&display_opts, &buf, item->string, - matched->value_is_null ? NULL : matched->value.buf, - &matched->kvi); - fwrite(buf.buf, 1, buf.len, stdout); + status = format_config(&display_opts, &buf, item->string, + matched->value_is_null ? NULL : matched->value.buf, + &matched->kvi); + if (!status) + fwrite(buf.buf, 1, buf.len, stdout); strbuf_release(&buf); strbuf_release(&matched->value); diff --git a/config.c b/config.c index 6552e5b0b8..c6ef290011 100644 --- a/config.c +++ b/config.c @@ -1292,6 +1292,7 @@ int git_config_pathname(char **dest, const char *var, const char *value) if (is_optional && is_missing_file(path)) { free(path); + *dest = NULL; return 0; } diff --git a/t/meson.build b/t/meson.build index bbeba1a8d5..137c0caea0 100644 --- a/t/meson.build +++ b/t/meson.build @@ -182,6 +182,7 @@ integration_tests = [ 't1308-config-set.sh', 't1309-early-config.sh', 't1310-config-default.sh', + 't1311-config-optional.sh', 't1350-config-hooks-path.sh', 't1400-update-ref.sh', 't1401-symbolic-ref.sh', diff --git a/t/t1311-config-optional.sh b/t/t1311-config-optional.sh new file mode 100755 index 0000000000..fbbacfc67b --- /dev/null +++ b/t/t1311-config-optional.sh @@ -0,0 +1,38 @@ +#!/bin/sh +# +# Copyright (c) 2025 Google LLC +# + +test_description=':(optional) paths' + +. ./test-lib.sh + +test_expect_success 'var=:(optional)path-exists' ' + test_config a.path ":(optional)path-exists" && + >path-exists && + echo path-exists >expect && + + git config get --path a.path >actual && + test_cmp expect actual +' + +test_expect_success 'missing optional value is ignored' ' + test_config a.path ":(optional)no-such-path" && + # 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 +' + +test_expect_success 'missing optional value is ignored in multi-value config' ' + test_when_finished "git config unset --all a.path" && + git config set --append a.path ":(optional)path-exists" && + git config set --append a.path ":(optional)no-such-path" && + >path-exists && + echo path-exists >expect && + + git config --get --path a.path >actual && + test_cmp expect actual +' + +test_done From 0bd16856ffb3968de73699ad0555d1fae6c45406 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 20 Nov 2025 11:45:35 -0800 Subject: [PATCH 3/3] config: really treat missing optional path as not configured These callers expect that git_config_pathname() that returns 0 is a signal that the variable they passed has a string they need to act on. But with the introduction of ":(optional)path" earlier, that is no longer the case. If the path specified by the configuration variable is missing, their variable will get a NULL in it, and they need to act on it (often, just refraining from copying it elsewhere). Signed-off-by: Junio C Hamano --- builtin/blame.c | 3 ++- builtin/receive-pack.c | 5 +++-- fetch-pack.c | 5 +++-- fsck.c | 12 +++++++----- gpg-interface.c | 10 +++++++++- setup.c | 2 +- 6 files changed, 25 insertions(+), 12 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 5b10e84b66..1092834144 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -739,7 +739,8 @@ static int git_blame_config(const char *var, const char *value, ret = git_config_pathname(&str, var, value); if (ret) return ret; - string_list_insert(&ignore_revs_file_list, str); + if (str) + string_list_insert(&ignore_revs_file_list, str); free(str); return 0; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 1113137a6f..4718573354 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -177,8 +177,9 @@ static int receive_pack_config(const char *var, const char *value, if (git_config_pathname(&path, var, value)) return -1; - strbuf_addf(&fsck_msg_types, "%cskiplist=%s", - fsck_msg_types.len ? ',' : '=', path); + if (path) + strbuf_addf(&fsck_msg_types, "%cskiplist=%s", + fsck_msg_types.len ? ',' : '=', path); free(path); return 0; } diff --git a/fetch-pack.c b/fetch-pack.c index 46c39f85c4..33a3f20bfc 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1872,8 +1872,9 @@ int fetch_pack_fsck_config(const char *var, const char *value, if (git_config_pathname(&path, var, value)) return -1; - strbuf_addf(msg_types, "%cskiplist=%s", - msg_types->len ? ',' : '=', path); + if (path) + strbuf_addf(msg_types, "%cskiplist=%s", + msg_types->len ? ',' : '=', path); free(path); return 0; } diff --git a/fsck.c b/fsck.c index 171b424dd5..0c287699d2 100644 --- a/fsck.c +++ b/fsck.c @@ -1351,14 +1351,16 @@ int git_fsck_config(const char *var, const char *value, if (strcmp(var, "fsck.skiplist") == 0) { char *path; - struct strbuf sb = STRBUF_INIT; if (git_config_pathname(&path, var, value)) return -1; - strbuf_addf(&sb, "skiplist=%s", path); - free(path); - fsck_set_msg_types(options, sb.buf); - strbuf_release(&sb); + if (path) { + struct strbuf sb = STRBUF_INIT; + strbuf_addf(&sb, "skiplist=%s", path); + free(path); + fsck_set_msg_types(options, sb.buf); + strbuf_release(&sb); + } return 0; } diff --git a/gpg-interface.c b/gpg-interface.c index 06e7fb5060..8b91a11a43 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -794,8 +794,16 @@ static int git_gpg_config(const char *var, const char *value, fmtname = "ssh"; if (fmtname) { + char *program; + int status; + fmt = get_format_by_name(fmtname); - return git_config_pathname((char **) &fmt->program, var, value); + status = git_config_pathname(&program, var, value); + if (status) + return status; + if (program) + fmt->program = program; + return status; } return 0; diff --git a/setup.c b/setup.c index 98ddbf377f..18301b5907 100644 --- a/setup.c +++ b/setup.c @@ -1248,7 +1248,7 @@ static int safe_directory_cb(const char *key, const char *value, } else { char *allowed = NULL; - if (!git_config_pathname(&allowed, key, value)) { + if (!git_config_pathname(&allowed, key, value) && allowed) { char *normalized = NULL; /*