config API: have *_multi() return an "int" and take a "dest"

Have the "git_configset_get_value_multi()" function and its siblings
return an "int" and populate a "**dest" parameter like every other
git_configset_get_*()" in the API.

As we'll take advantage of in subsequent commits, this fixes a blind
spot in the API where it wasn't possible to tell whether a list was
empty from whether a config key existed. For now we don't make use of
those new return values, but faithfully convert existing API users.

Most of this is straightforward, commentary on cases that stand out:

- To ensure that we'll properly use the return values of this function
  in the future we're using the "RESULT_MUST_BE_USED" macro introduced
  in [1].

  As git_die_config() now has to handle this return value let's have
  it BUG() if it can't find the config entry. As tested for in a
  preceding commit we can rely on getting the config list in
  git_die_config().

- The loops after getting the "list" value in "builtin/gc.c" could
  also make use of "unsorted_string_list_has_string()" instead of using
  that loop, but let's leave that for now.

- In "versioncmp.c" we now use the return value of the functions,
  instead of checking if the lists are still non-NULL.

1. 1e8697b5c4e (submodule--helper: check repo{_submodule,}_init()
   return values, 2022-09-01),

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 6aeac37148830d316f533e5399c79ed551fda2fe..fd0e7739e6a92d8c32be478a180b688fab46e453 100644 (file)
--- a/builtin/for-each-repo.c
+++ b/builtin/for-each-repo.c
@@ -45,14 +45,11 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)
        if (!config_key)
                die(_("missing --config=<config>"));
 
-       values = repo_config_get_value_multi(the_repository,
-                                            config_key);
-
        /*
         * Do nothing on an empty list, which is equivalent to the case
         * where the config variable does not exist at all.
         */
-       if (!values)
+       if (repo_config_get_value_multi(the_repository, config_key, &values))
                return 0;
 
        for (i = 0; !result && i < values->nr; i++)

diff --git a/builtin/gc.c b/builtin/gc.c
index e38d1783f30a73ef00d1bdc3227a1c088188b607..2b3da377d52b97fd05f139c38b7d1e67654ecbac 100644 (file)
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1510,8 +1510,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
        if (git_config_get("maintenance.strategy"))
                git_config_set("maintenance.strategy", "incremental");
 
-       list = git_config_get_value_multi(key);
-       if (list) {
+       if (!git_config_get_value_multi(key, &list)) {
                for_each_string_list_item(item, list) {
                        if (!strcmp(maintpath, item->string)) {
                                found = 1;
@@ -1577,11 +1576,10 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
        if (config_file) {
                git_configset_init(&cs);
                git_configset_add_file(&cs, config_file);
-               list = git_configset_get_value_multi(&cs, key);
-       } else {
-               list = git_config_get_value_multi(key);
        }
-       if (list) {
+       if (!(config_file
+             ? git_configset_get_value_multi(&cs, key, &list)
+             : git_config_get_value_multi(key, &list))) {
                for_each_string_list_item(item, list) {
                        if (!strcmp(maintpath, item->string)) {
                                found = 1;

diff --git a/builtin/log.c b/builtin/log.c
index 5eafcf26b49bd92d1d24780107ca87d00a054e3a..cc9d92f95da895d987c5f4549227babe915d2aca 100644 (file)
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -182,10 +182,10 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f
        int i;
        char *value = NULL;
        struct string_list *include = decoration_filter->include_ref_pattern;
-       const struct string_list *config_exclude =
-                       git_config_get_value_multi("log.excludeDecoration");
+       const struct string_list *config_exclude;
 
-       if (config_exclude) {
+       if (!git_config_get_value_multi("log.excludeDecoration",
+                                       &config_exclude)) {
                struct string_list_item *item;
                for_each_string_list_item(item, config_exclude)
                        string_list_append(decoration_filter->exclude_ref_config_pattern,

diff --git a/config.c b/config.c
index 5d3e84f85ad2eb6e04733158382f5526a1e60ffc..1f654daf6fcc936e9bc28314eb173d1d598c6b68 100644 (file)
--- a/config.c
+++ b/config.c
@@ -2404,29 +2404,34 @@ int git_configset_add_file(struct config_set *cs, const char *filename)
 int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
 {
        const struct string_list *values = NULL;
+       int ret;
+
        /*
         * Follows "last one wins" semantic, i.e., if there are multiple matches for the
         * queried key in the files of the configset, the value returned will be the last
         * value in the value list for that key.
         */
-       values = git_configset_get_value_multi(cs, key);
+       if ((ret = git_configset_get_value_multi(cs, key, &values)))
+               return ret;
 
-       if (!values)
-               return 1;
        assert(values->nr > 0);
        *value = values->items[values->nr - 1].string;
        return 0;
 }
 
-const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
+int git_configset_get_value_multi(struct config_set *cs, const char *key,
+                                 const struct string_list **dest)
 {
        struct config_set_element *e;
+       int ret;
 
-       if (configset_find_element(cs, key, &e))
-               return NULL;
+       if ((ret = configset_find_element(cs, key, &e)))
+               return ret;
        else if (!e)
-               return NULL;
-       return &e->value_list;
+               return 1;
+       *dest = &e->value_list;
+
+       return 0;
 }
 
 int git_configset_get(struct config_set *cs, const char *key)
@@ -2590,11 +2595,11 @@ int repo_config_get_value(struct repository *repo,
        return git_configset_get_value(repo->config, key, value);
 }
 
-const struct string_list *repo_config_get_value_multi(struct repository *repo,
-                                                     const char *key)
+int repo_config_get_value_multi(struct repository *repo, const char *key,
+                               const struct string_list **dest)
 {
        git_config_check_init(repo);
-       return git_configset_get_value_multi(repo->config, key);
+       return git_configset_get_value_multi(repo->config, key, dest);
 }
 
 int repo_config_get_string(struct repository *repo,
@@ -2707,9 +2712,9 @@ int git_config_get_value(const char *key, const char **value)
        return repo_config_get_value(the_repository, key, value);
 }
 
-const struct string_list *git_config_get_value_multi(const char *key)
+int git_config_get_value_multi(const char *key, const struct string_list **dest)
 {
-       return repo_config_get_value_multi(the_repository, key);
+       return repo_config_get_value_multi(the_repository, key, dest);
 }
 
 int git_config_get_string(const char *key, char **dest)
@@ -2856,7 +2861,8 @@ void git_die_config(const char *key, const char *err, ...)
                error_fn(err, params);
                va_end(params);
        }
-       values = git_config_get_value_multi(key);
+       if (git_config_get_value_multi(key, &values))
+               BUG("for key '%s' we must have a value to report on", key);
        kv_info = values->items[values->nr - 1].util;
        git_die_config_linenr(key, kv_info->filename, kv_info->linenr);
 }

diff --git a/config.h b/config.h
index cbd82e5c43b90bdda50bc3001cf07a74195f8039..f228fa9d5d12345265c5bf0ac20eecbb4b3ed65a 100644 (file)
--- a/config.h
+++ b/config.h
@@ -459,10 +459,18 @@ int git_configset_add_parameters(struct config_set *cs);
 /**
  * Finds and returns the value list, sorted in order of increasing priority
  * for the configuration variable `key` and config set `cs`. When the
- * configuration variable `key` is not found, returns NULL. The caller
- * should not free or modify the returned pointer, as it is owned by the cache.
+ * configuration variable `key` is not found, returns 1 without touching
+ * `value`.
+ *
+ * The key will be parsed for validity with git_config_parse_key(), on
+ * error a negative value will be returned.
+ *
+ * The caller should not free or modify the returned pointer, as it is
+ * owned by the cache.
  */
-const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
+RESULT_MUST_BE_USED
+int git_configset_get_value_multi(struct config_set *cs, const char *key,
+                                 const struct string_list **dest);
 
 /**
  * Clears `config_set` structure, removes all saved variable-value pairs.
@@ -511,8 +519,9 @@ RESULT_MUST_BE_USED
 int repo_config_get(struct repository *repo, const char *key);
 int repo_config_get_value(struct repository *repo,
                          const char *key, const char **value);
-const struct string_list *repo_config_get_value_multi(struct repository *repo,
-                                                     const char *key);
+RESULT_MUST_BE_USED
+int repo_config_get_value_multi(struct repository *repo, const char *key,
+                               const struct string_list **dest);
 int repo_config_get_string(struct repository *repo,
                           const char *key, char **dest);
 int repo_config_get_string_tmp(struct repository *repo,
@@ -566,10 +575,14 @@ int git_config_get_value(const char *key, const char **value);
 /**
  * Finds and returns the value list, sorted in order of increasing priority
  * for the configuration variable `key`. When the configuration variable
- * `key` is not found, returns NULL. The caller should not free or modify
- * the returned pointer, as it is owned by the cache.
+ * `key` is not found, returns 1 without touching `value`.
+ *
+ * The caller should not free or modify the returned pointer, as it is
+ * owned by the cache.
  */
-const struct string_list *git_config_get_value_multi(const char *key);
+RESULT_MUST_BE_USED
+int git_config_get_value_multi(const char *key,
+                              const struct string_list **dest);
 
 /**
  * Resets and invalidates the config cache.

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be742c7e2a19a62f5adb968d409bcc29..81f0c0e016bd5a59a23fed1b0708dfeddb8b63db 100644 (file)
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2301,7 +2301,11 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git)
 
 const struct string_list *bitmap_preferred_tips(struct repository *r)
 {
-       return repo_config_get_value_multi(r, "pack.preferbitmaptips");
+       const struct string_list *dest;
+
+       if (!repo_config_get_value_multi(r, "pack.preferbitmaptips", &dest))
+               return dest;
+       return NULL;
 }
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname)

diff --git a/submodule.c b/submodule.c
index 8ac2fca855d6c3615be2e5b8a7f16a4746441e1a..e43c4230ba39524503f221577aa9dfb06a235003 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -274,8 +274,7 @@ int is_tree_submodule_active(struct repository *repo,
        free(key);
 
        /* submodule.active is set */
-       sl = repo_config_get_value_multi(repo, "submodule.active");
-       if (sl) {
+       if (!repo_config_get_value_multi(repo, "submodule.active", &sl)) {
                struct pathspec ps;
                struct strvec args = STRVEC_INIT;
                const struct string_list_item *item;

diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index cbb33ae1fffef9d4842bb31bd3120a5dbe955130..6dc4c37444fcbbc97db54bfd402a396b27e730bd 100644 (file)
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -97,8 +97,7 @@ int cmd__config(int argc, const char **argv)
                        goto exit1;
                }
        } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
-               strptr = git_config_get_value_multi(argv[2]);
-               if (strptr) {
+               if (!git_config_get_value_multi(argv[2], &strptr)) {
                        for (i = 0; i < strptr->nr; i++) {
                                v = strptr->items[i].string;
                                if (!v)
@@ -181,8 +180,7 @@ int cmd__config(int argc, const char **argv)
                                goto exit2;
                        }
                }
-               strptr = git_configset_get_value_multi(&cs, argv[2]);
-               if (strptr) {
+               if (!git_configset_get_value_multi(&cs, argv[2], &strptr)) {
                        for (i = 0; i < strptr->nr; i++) {
                                v = strptr->items[i].string;
                                if (!v)

diff --git a/versioncmp.c b/versioncmp.c
index 323f5d35ea83c4297f77a5157bd368f61a741884..60c3a51712291a3225b95ccd2fef593a7cb85c3c 100644 (file)
--- a/versioncmp.c
+++ b/versioncmp.c
@@ -162,13 +162,16 @@ int versioncmp(const char *s1, const char *s2)
        if (!initialized) {
                const char *const newk = "versionsort.suffix";
                const char *const oldk = "versionsort.prereleasesuffix";
+               const struct string_list *newl;
                const struct string_list *oldl;
+               int new = git_config_get_value_multi(newk, &newl);
+               int old = git_config_get_value_multi(oldk, &oldl);
 
-               prereleases = git_config_get_value_multi(newk);
-               oldl = git_config_get_value_multi(oldk);
-               if (prereleases && oldl)
+               if (!new && !old)
                        warning("ignoring %s because %s is set", oldk, newk);
-               else if (!prereleases)
+               if (!new)
+                       prereleases = newl;
+               else if (!old)
                        prereleases = oldl;
 
                initialized = 1;