git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Sep 27, 2022, 16:21 UTC
Message-ID
<220927.86sfkcyebf.gmgdl@evledraar.gmail.com>
In-Reply-To
<396343ce7dd17f86bbbc66197c6f0b4012caf445.1664287711.git.gitgitgadget@gmail.com>
On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:
Show 52 quoted lines
> 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)
Previous: Derrick Stolee via GitGitGadgetNext: Derrick Stolee
Message 9 of 21 in “[RFC] config API: return empty list, not NULL”
  1. 0/5 [RFC] config API: return empty list, not NULLDerrick Stolee via GitGitGadget, Sep 27, 2022
  2. 1/5 config: relax requirements on multi-value returnDerrick Stolee via GitGitGadget, Sep 27, 2022
  3. Junio C HamanoSep 27, 2022
  4. 3/5 config: add BUG() statement instead of possible segfaultDerrick Stolee via GitGitGadget, Sep 27, 2022
  5. Ævar Arnfjörð BjarmasonSep 27, 2022
  6. Derrick StoleeSep 27, 2022
  7. Ævar Arnfjörð BjarmasonSep 27, 2022
  8. 4/5 config: return an empty list, not NULLDerrick Stolee via GitGitGadget, Sep 27, 2022
  9. Ævar Arnfjörð BjarmasonSep 27, 2022
  10. Derrick StoleeSep 27, 2022
  11. Ævar Arnfjörð BjarmasonSep 27, 2022
  12. Derrick StoleeSep 28, 2022
  13. Ævar Arnfjörð BjarmasonSep 28, 2022
  14. Derrick StoleeSep 28, 2022
  15. Ævar Arnfjörð BjarmasonSep 28, 2022
  16. 2/5 *: relax git_configset_get_value_multi resultDerrick Stolee via GitGitGadget, Sep 27, 2022
  17. Taylor BlauSep 28, 2022
  18. 5/5 *: expect a non-NULL list of config valuesDerrick Stolee via GitGitGadget, Sep 27, 2022
  19. Junio C HamanoSep 28, 2022
  20. Derrick StoleeSep 28, 2022
  21. Ævar Arnfjörð BjarmasonSep 28, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.