Re: [PATCH v3 5/9] config API: have *_multi() return an "int" and take a "dest"

From: Glen Choo <chooglen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Jeff King" <peff@peff.net>, "Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v3 5/9] config API: have *_multi() return an "int" and take a "dest"
Date: Wed, 18 Jan 2023 16:50:34 -0800	[thread overview]
Message-ID: <kl6lcz7bny91.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <patch-v3-5.9-e0e6ade3f38-20221125T093159Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> writes:

> 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.

Indeed, this is the only function that's inconsistent. Great to see that
it's being fixed :)

> 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.

Sounds good.

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

Thanks for this btw, I found it quite helpful for navigating the patch.
>
> - As we've tested for in a preceding commit we can rely on getting the
>   config list in git_die_config(), and as we need to handle the new
>   return value let's BUG() out if we can't acquire it.

This wasn't immediately clear to me; I'll explain more in the code.

> - In "builtin/for-each-ref.c" we could preserve the comment added in

I think you meant for-each-repo.

> @@ -45,14 +46,10 @@ 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)
> +	err = repo_config_get_value_multi(the_repository, config_key, &values);
> +	if (err < 0)
> +		return 0;
> +	else if (err)
>  		return 0;

This conditional could be collapsed into "if (err)", but it's like this
because the next patch distinguishes between the two cases. Not really
worth the callout in commentary, but FYI for others who might be
wondering the same thing.

> diff --git a/config.c b/config.c
> index c058b2c70c3..0b07045ed8c 100644
> --- a/config.c
> +++ b/config.c
> @@ -2275,23 +2275,28 @@ void read_very_early_config(config_fn_t cb, void *data)
>  	config_with_options(cb, data, NULL, &opts);
>  }
>  
> -static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
> +static int configset_find_element(struct config_set *cs, const char *key,
> +				  struct config_set_element **dest)
>  {
>  	struct config_set_element k;
>  	struct config_set_element *found_entry;
>  	char *normalized_key;
> +	int ret;
> +
>  	/*
>  	 * `key` may come from the user, so normalize it before using it
>  	 * for querying entries from the hashmap.
>  	 */
> -	if (git_config_parse_key(key, &normalized_key, NULL))
> -		return NULL;
> +	ret = git_config_parse_key(key, &normalized_key, NULL);
> +	if (ret < 0)
> +		return ret;
>  
>  	hashmap_entry_init(&k.ent, strhash(normalized_key));
>  	k.key = normalized_key;
>  	found_entry = hashmap_get_entry(&cs->config_hash, &k, ent, NULL);
>  	free(normalized_key);
> -	return found_entry;
> +	*dest = found_entry;
> +	return 0;
>  }
>  
>  static int configset_add_value(struct config_set *cs, const char *key, const char *value)
> @@ -2300,8 +2305,11 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>  	struct string_list_item *si;
>  	struct configset_list_item *l_item;
>  	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
> +	int ret;
>  
> -	e = configset_find_element(cs, key);
> +	ret = configset_find_element(cs, key, &e);
> +	if (ret < 0)
> +		return ret;
>  	/*
>  	 * Since the keys are being fed by git_config*() callback mechanism, they
>  	 * are already normalized. So simply add them without any further munging.
> @@ -2395,24 +2403,38 @@ 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);
> +	ret = git_configset_get_value_multi(cs, key, &values);
>  
> -	if (!values)
> +	if (ret < 0)
> +		return ret;
> +	else 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 = configset_find_element(cs, key);
> -	return e ? &e->value_list : NULL;
> +	struct config_set_element *e;
> +	int ret;
> +
> +	ret = configset_find_element(cs, key, &e);
> +	if (ret < 0)
> +		return ret;
> +	else if (!e)
> +		return 1;
> +	*dest = &e->value_list;
> +
> +	return 0;
>  }

The changes here and the call sites look quite straightforward.

>  int git_config_get_string(const char *key, char **dest)
> @@ -2819,7 +2841,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);
>  }

Here is the BUG() call that wasn't immediately clear to me. What wasn't
obvious from the commentary is that this was an 'unhandled error'
before (we didn't check if the returned value was NULL). Arguably we
should have had this BUG call before, but we didn't enforce this until
we added RESULT_MUST_BE_USED.

And this should be a BUG(), and not e.g. error(), since git_die_config()
is meant to report bad config values, so git_config_get_value_multi()
should never fail if we've already managed to get a value, looks good.

> diff --git a/config.h b/config.h
> index ef9eade6414..7f6ce6f2fb5 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);

Updated comments look good too.