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 see in 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. We'll take advantage of that in
subsequent commits, but for now we're faithfully converting existing
API callers.
A logical follow-up to this would be to change the various "*_get_*()"
functions to ferry the git_configset_get_value() return value to their
own callers, e.g. git_configset_get_int() returns "1" rather than
ferrying up the "-1" that "git_configset_get_value()" might return,
but that's not being done in this series
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@xxxxxxxxx>
---
builtin/for-each-repo.c | 5 +----
builtin/gc.c | 10 ++++------
builtin/log.c | 6 +++---
config.c | 34 ++++++++++++++++++++--------------
config.h | 29 +++++++++++++++++++++--------
pack-bitmap.c | 6 +++++-
submodule.c | 3 +--
t/helper/test-config.c | 6 ++----
versioncmp.c | 11 +++++++----
9 files changed, 64 insertions(+), 46 deletions(-)
diff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c
index 6aeac371488..fd0e7739e6a 100644
--- 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 e38d1783f30..2b3da377d52 100644
--- 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 04412dd9c93..cec8cabd21e 100644
--- 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 b88da70c664..ce5d50a490c 100644
--- a/config.c
+++ b/config.c
@@ -2417,29 +2417,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)
@@ -2603,11 +2608,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,
@@ -2720,9 +2725,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)
@@ -2869,7 +2874,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 04c5e594015..fbc153cdc96 100644
--- 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.
@@ -507,8 +515,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,
@@ -561,10 +570,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 d2a42abf28c..15c5eb507c0 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2314,7 +2314,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 3a0dfc417c0..4b6f5223b0c 100644
--- 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 4ba9eb65606..8f70beb6c9d 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -95,8 +95,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)
@@ -159,8 +158,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 323f5d35ea8..60c3a517122 100644
--- 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;
--
2.39.1.1397.g8c8c074958d
