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

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

> @@ -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;

> 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;
>  }

>  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);
>  }

> 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);