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 <key, value> pair.

Reported-by: Han Jiang <jhcarl0814@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/config.c b/builtin/config.c
index 59fb113b0739268e0602a76db66c63049ab88960..2b36eb7d1cd2045cbe8a65374e74861656f2bbd7 100644 (file)
--- 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 <key_,value_> 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 6552e5b0b8020232fbc5dcd128794e9cfe2ab698..c6ef290011b752bb5b6c1ed3aea79e4360fdf646 100644 (file)
--- 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 bbeba1a8d50e1bb2174e9548656a8f10f5c1fd9d..137c0caea0e1b508445d45698b5a2752c5f2b18d 100644 (file)
--- 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 (executable)
index 0000000..fbbacfc
--- /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