Re: ab/config-multi-and-nonbool (was Re: What's cooking in git.git (Feb 2023, #04; Wed, 22))
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 23, 2023, 22:57 UTC
- Message-ID
- <xmqqa61456ru.fsf@gitster.g>
- In-Reply-To
- <kl6lfsaw84vo.fsf@chooglen-macbookpro.roam.corp.google.com>
Glen Choo <chooglen@google.com> writes:
Show 6 quoted lines
> I would prefer to eject 06/10 [1] for now and leave it in for a future > cleanup series. I haven't confirmed whether it's safe (the practical > effect of that patch is that the *_get() functions can now return -1 > instead of 1, so the patch is safe if all callers only check if the > return value is zero, and not whether it has a particular sign), and I > don't think 06/10 is absolutely necessary to the series.
If somebody wants to help auditing potential breakage the patch in question causes, "are callers happy to declare an error upon non-zero return?" is not exactly sufficient. While all of these functions return 0 upon success, some of them return 1 on certain cases while error() on other cases, and we need to ensure that the callers are not behaving differently upon these values.
So let's see how many functions does the patch in question touch.
Show 18 quoted lines
> diff --git a/config.c b/config.c
> index 1f654daf6f..74d453f5f9 100644
> --- a/config.c
> +++ b/config.c
> @@ -2449,86 +2449,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);
> }This used to return 1 when there is no such value, and get whatever error signal git_config_string() gave if git_config_string() failed.
Luckily, git_configset_get_value() returns 1 when there is no such value, so I think this hunk is an expensive-to-audit no-op.
Show 21 quoted lines
> 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;
> }Ditto.
Show 15 quoted lines
> 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;
> }Ditto.
Show 15 quoted lines
> 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;
> }Ditto.
Show 15 quoted lines
> 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;
> }Ditto.
Show 16 quoted lines
> 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;
> }Ditto.
Show 19 quoted lines
> 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;
> }Ditto.
Show 13 quoted lines
> 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);
> }Ditto.
Show 11 quoted lines
> @@ -2775,9 +2782,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;
This was returning -1 for an error (at the end of the function), as well as 1 for "no such thing". Now, this is a breaking change if any callers wanted to tell between -1 and 1.
Luckily, I think its two callers discard the error return; they initialize expiry to some reasonable default and rely on the fact that the expiry is not updated when these configuration API calls did not find anything interesting.
> @@ -2827,15 +2837,14 @@ int git_config_get_index_threads(int *dest)
You can easily find output in "git grep git_config_get_index_threads" that all four callers only care if they get 0 or not, so this one should be safe.