Glen Choo <chooglen@xxxxxxxxxx> writes:
> 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.
> 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.
> 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.
> 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.
> 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.
> 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.
> 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.
> 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.
> 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.
> @@ -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.
