Re: [PATCH 4/5] config: return an empty list, not NULL

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
>
> For the multi-valued config API methods, Git previously returned a NULL
> list instead of an empty list. Previous changes adjusted all callers to
> instead expect an empty, non-NULL list, making this a safe change.
>
> The next change will remove the NULL checks from all callers.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  config.c | 3 ++-
>  config.h | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index 0c41606c7d4..2d4ca1ae6dc 100644
> --- a/config.c
> +++ b/config.c
> @@ -2415,8 +2415,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
>  
>  const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
>  {
> +	static struct string_list empty_list = STRING_LIST_INIT_NODUP;
>  	struct config_set_element *e = configset_find_element(cs, key);
> -	return e ? &e->value_list : NULL;
> +	return e ? &e->value_list : &empty_list;
>  }
>  
>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
> diff --git a/config.h b/config.h
> index ca994d77147..9897b97c0b9 100644
> --- a/config.h
> +++ b/config.h
> @@ -458,7 +458,7 @@ 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
> + * configuration variable `key` is not found, returns an empty list. 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);
> @@ -543,8 +543,8 @@ int git_config_get_value(const char *key, const char **value);
>  /**
>   * Finds and returns the value list, sorted in order of increasing priority
>   * for the configuration variable `key`. 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.
> + * `key` is not found, returns an empty list. The caller should not free or
> + * modify the returned pointer, as it is owned by the cache.
>   */
>  const struct string_list *git_config_get_value_multi(const char *key);

Aside from the "DWIM API" aspect of this (which I don't mind) I think
this is really taking the low-level function in the wrong direction, and
that we should just add a new simple wrapper instead.

I.e. both the pre-image API docs & this series gloss over the fact that
we'd not just return NULL here if the config wasn't there, but also if
git_config_parse_key() failed.

So it seems to me that a better direction would be starting with
something like the WIP below (which doesn't compile the whole code, I
stopped at config.[ch] and pack-bitmap.c). I.e. the same "int" return
and "dest" pattern that most other things in the config API have.

The functionality you want in this series would then just be a wrapper
for that which wouldn't care about the difference between "bad key" and
"no such config entry".

But I really think the low-level config API should care about the
difference, granted before your change it's rather crappy, and would
return NULL for both, but I think we can do better...

diff --git a/config.c b/config.c
index cbb5a3bab74..a9536d3a9e8 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.
@@ -2400,24 +2408,34 @@ int git_configset_add_parameters(struct config_set *cs)
 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 (ret < 0)
+		return ret;
 
-	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 = NULL;
+	int ret;
+
+	ret = configset_find_element(cs, key, &e);
+	if (ret < 0)
+		return ret;
+	*dest = &e->value_list;
+	return 0;
 }
 
 int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
@@ -2563,11 +2581,12 @@ int repo_config_get_value(struct repository *repo,
 	return git_configset_get_value(repo->config, key, value);
 }
 
-const struct string_list *repo_config_get_value_multi(struct repository *repo,
-						      const char *key)
+int repo_config_get_value_multi(struct repository *repo,
+				const char *key,
+				const struct string_list **dest)
 {
 	git_config_check_init(repo);
-	return git_configset_get_value_multi(repo->config, key);
+	return git_configset_get_value_multi(repo->config, key, dest);
 }
 
 int repo_config_get_string(struct repository *repo,
@@ -2684,9 +2703,9 @@ int git_config_get_value(const char *key, const char **value)
 	return repo_config_get_value(the_repository, key, value);
 }
 
-const struct string_list *git_config_get_value_multi(const char *key)
+int git_config_get_value_multi(const char *key, const struct string_list **dest)
 {
-	return repo_config_get_value_multi(the_repository, key);
+	return repo_config_get_value_multi(the_repository, key, dest);
 }
 
 int git_config_get_string(const char *key, char **dest)
@@ -2826,6 +2845,7 @@ void git_die_config(const char *key, const char *err, ...)
 	const struct string_list *values;
 	struct key_value_info *kv_info;
 	report_fn error_fn = get_error_routine();
+	int ret;
 
 	if (err) {
 		va_list params;
@@ -2833,7 +2853,9 @@ void git_die_config(const char *key, const char *err, ...)
 		error_fn(err, params);
 		va_end(params);
 	}
-	values = git_config_get_value_multi(key);
+	ret = git_config_get_value_multi(key, &values);
+	if (ret < 0)
+		BUG("...");
 	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 ca994d77147..99b8dc1944c 100644
--- a/config.h
+++ b/config.h
@@ -461,7 +461,8 @@ int git_configset_add_parameters(struct config_set *cs);
  * 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.
  */
-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);
 
 /**
  * Clears `config_set` structure, removes all saved variable-value pairs.
@@ -495,8 +496,9 @@ struct repository;
 void repo_config(struct repository *repo, config_fn_t fn, void *data);
 int repo_config_get_value(struct repository *repo,
 			  const char *key, const char **value);
-const struct string_list *repo_config_get_value_multi(struct repository *repo,
-						      const char *key);
+int repo_config_get_value_multi(struct repository *repo,
+				const char *key,
+				const struct string_list **dest);
 int repo_config_get_string(struct repository *repo,
 			   const char *key, char **dest);
 int repo_config_get_string_tmp(struct repository *repo,
@@ -546,7 +548,8 @@ int git_config_get_value(const char *key, const char **value);
  * `key` is not found, returns NULL. The caller should not free or modify
  * the returned pointer, as it is owned by the cache.
  */
-const struct string_list *git_config_get_value_multi(const char *key);
+int git_config_get_value_multi(const char *key,
+			       const struct string_list **dest);
 
 /**
  * Resets and invalidates the config cache.
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be7..0d704dc76ef 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2301,7 +2301,13 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git)
 
 const struct string_list *bitmap_preferred_tips(struct repository *r)
 {
-	return repo_config_get_value_multi(r, "pack.preferbitmaptips");
+	const struct string_list *dest;
+	int ret;
+
+	ret = repo_config_get_value_multi(r, "pack.preferbitmaptips", &dest);
+	if (ret < 0)
+		BUG("got %d from repo_config_get_value_multi()", ret);
+	return dest;
 }
 
 int bitmap_is_preferred_refname(struct repository *r, const char *refname)