[PATCH v5 06/10] config API: don't lose the git_*get*() return values
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 7, 2023, 16:10 UTC
- Message-ID
- <patch-v5-06.10-b515ff13f9b-20230207T154000Z-avarab@gmail.com>
- In-Reply-To
- <cover-v5-00.10-00000000000-20230207T154000Z-avarab@gmail.com>
Since a preceding commit which added the "git_config_get()" family of functions, and the preceding commit where *_multi() started returning an "int" we've finally been able to ferry up non-zero return values, rather than having negative return values normalized to a "return 1" along the way.
In practice this doesn't matter to existing callers. They're either ignoring these return values and relying on us to only populate "dest" if we'd return 0, or normalizing non-zero return values with "!".
Even if they weren't normalizing them we'll only return non-zero negative values in those cases where the config key itself is bad, which excludes the vast majority of our callers, as they hardcode a valued configuration key as a fixed string in the C sources.
So this change is expected to do nothing for now, but is really here for our own sanity. It's much harder to reason about an API that's losing return values in some cases, and coercing them in others. If there isn't a compelling reason to do otherwise we should let the caller decide if they care about the distinction between bad keys and non-existence.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- config.c | 117 ++++++++++++++++++++++++++++++------------------------- config.h | 16 ++++---- 2 files changed, 72 insertions(+), 61 deletions(-)
diff --git a/config.c b/config.c index 569819b4a1b..8d7e40ac8a4 100644 --- a/config.c +++ b/config.c @@ -2463,86 +2463,93 @@ int git_configset_get(struct config_set *cs, const char *key) int git_configset_get_string(struct config_set *cs, const char *key, char **dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) - return git_config_string((const char **)dest, key, value); - else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + return git_config_string((const char **)dest, key, value); } static int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - if (!value) - return config_error_nonbool(key); - *dest = value; - return 0; - } else { - return 1; - } + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + if (!value) + return config_error_nonbool(key); + *dest = value; + return 0; } int git_configset_get_int(struct config_set *cs, const char *key, int *dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - *dest = git_config_int(key, value); - return 0; - } else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + *dest = git_config_int(key, value); + return 0; } int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - *dest = git_config_ulong(key, value); - return 0; - } else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + *dest = git_config_ulong(key, value); + return 0; } int git_configset_get_bool(struct config_set *cs, const char *key, int *dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - *dest = git_config_bool(key, value); - return 0; - } else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + *dest = git_config_bool(key, value); + return 0; } int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - *dest = git_config_bool_or_int(key, value, is_bool); - return 0; - } else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + *dest = git_config_bool_or_int(key, value, is_bool); + return 0; } int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) { - *dest = git_parse_maybe_bool(value); - if (*dest == -1) - return -1; - return 0; - } else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + *dest = git_parse_maybe_bool(value); + if (*dest == -1) + return -1; + return 0; } int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest) { const char *value; - if (!git_configset_get_value(cs, key, &value)) - return git_config_pathname(dest, key, value); - else - return 1; + int ret; + + if ((ret = git_configset_get_value(cs, key, &value))) + return ret; + return git_config_pathname(dest, key, value); } /* Functions use to read configuration from a repository */ @@ -2789,9 +2796,11 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam const char *expiry_string; intmax_t days; timestamp_t when; + int ret; - if (git_config_get_string_tmp(key, &expiry_string)) - return 1; /* no such thing */ + if ((ret = git_config_get_string_tmp(key, &expiry_string))) + /* no such thing, or git_config_parse_key() failure etc. */ + return ret; if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) { const int scale = 86400; @@ -2834,6 +2843,7 @@ int git_config_get_max_percent_split_change(void) int git_config_get_index_threads(int *dest) { int is_bool, val; + int ret; val = git_env_ulong("GIT_TEST_INDEX_THREADS", 0); if (val) { @@ -2841,15 +2851,14 @@ int git_config_get_index_threads(int *dest) return 0; } - if (!git_config_get_bool_or_int("index.threads", &is_bool, &val)) { - if (is_bool) - *dest = val ? 0 : 1; - else - *dest = val; - return 0; - } - - return 1; + if ((ret = git_config_get_bool_or_int("index.threads", &is_bool, + &val))) + return ret; + if (is_bool) + *dest = val ? 0 : 1; + else + *dest = val; + return 0; } NORETURN diff --git a/config.h b/config.h index 115259ecb8d..da5c498d39a 100644 --- a/config.h +++ b/config.h @@ -477,20 +477,22 @@ int git_configset_get_value_multi(struct config_set *cs, const char *key, */ void git_configset_clear(struct config_set *cs); -/* +/** * These functions return 1 if not found, and 0 if found, leaving the found - * value in the 'dest' pointer. + * value in the 'dest' pointer. On error a negative value is returned. + * + * The functions that return a single value (i.e. not + * *_get_*multi*()) will return the highest-priority value for the + * configuration variable `key`, i.e. in the case where we have + * multiple values the last value found. */ RESULT_MUST_BE_USED int git_configset_get(struct config_set *cs, const char *key); /* - * Finds the highest-priority value for the configuration variable `key` - * and config set `cs`, stores the pointer to it in `value` and returns 0. - * When the configuration variable `key` is not found, returns 1 without - * touching `value`. The caller should not free or modify `value`, as it - * is owned by the cache. + * The caller should not free or modify `value`, as it is owned by the + * cache. */ int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);
-- 2.39.1.1430.gb2471c0aaf4