Re: [PATCH v4 3/9] config API: add and use a "git_config_get()" family of functions

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 2, 2023, 23:56 UTC
Message-ID: <xmqqv8kjpqoe.fsf@gitster.g>
In-Reply-To: <patch-v4-3.9-998b11ae4bc-20230202T131155Z-avarab@gmail.com>

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

Show 12 quoted lines

> We already have the basic "git_config_get_value()" function and its
> "repo_*" and "configset" siblings to get a given "key" and assign the
> last key found to a provided "value".
>
> But some callers don't care about that value, but just want to use the
> return value of the "get_value()" function to check whether the key
> exist (or another non-zero return value).
>
> The immediate motivation for this is that a subsequent commit will
> need to change all callers of the "*_get_value_multi()" family of
> functions. In two cases here we (ab)used it to check whether we had
> any values for the given key, but didn't care about the return value.

So, the idea is that

	if (!git_config_get_string(key, &discard))
		free(discard);
	else
		... the key is missing ...

becomes

	if (git_config_get(key))
		... the key is missing ...

In other words, git_config_get() returns 0 only when the key is used, and non-zero return signals that the key is not used?

Similarly, get_value_multi() was an interface to get to the value_list associated to the given key, and was abused like

	if (git_config_get_value_multi(key))
		... the key exists ...

which will become

	if (!git_config_get(key))
		... the key exists ...

right?

Show 6 quoted lines

>  	/* Set maintenance strategy, if unset */
> -	if (!git_config_get_string("maintenance.strategy", &config_value))
> -		free(config_value);
> -	else
> +	if (git_config_get("maintenance.strategy"))
>  		git_config_set("maintenance.strategy", "incremental");

OK.  config_get() says "true" meaning the key is missing.

> -	if (!argc && git_config_get_value_multi("submodule.active"))
> +	if (!argc && !git_config_get("submodule.active"))
>  		module_list_active(&list);

OK.

Show 7 quoted lines

> @@ -2743,7 +2743,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
>  		 * If there are no path args and submodule.active is set then,
>  		 * by default, only initialize 'active' modules.
>  		 */
> -		if (!argc && git_config_get_value_multi("submodule.active"))
> +		if (!argc && !git_config_get("submodule.active"))
>  			module_list_active(&list);

OK.

Show 6 quoted lines

> @@ -3185,7 +3184,7 @@ static void configure_added_submodule(struct add_data *add_data)
>  	 * is_submodule_active(), since that function needs to find
>  	 * out the value of "submodule.active" again anyway.
>  	 */
> -	if (!git_config_get_string_tmp("submodule.active", &val)) {
> +	if (!git_config_get("submodule.active")) {

string_tmp() variant is to retrieve borrowed value, and it returns 0 when there is a value. If it is a valueless true, we get -1 back with an error message. What does the updated version do in the valueless true case?

Show 10 quoted lines

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index f51c40f1e1e..6ba42d4ad20 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -338,7 +337,7 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
>  				to_file, "core.bare", NULL, "true", 0))
>  			error(_("failed to unset '%s' in '%s'"),
>  				"core.bare", to_file);
> -		if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
> +		if (!git_configset_get(&cs, "core.worktree") &&

OK.

Show 34 quoted lines

> diff --git a/config.c b/config.c
> index 00090a32fc3..b88da70c664 100644
> --- a/config.c
> +++ b/config.c
> @@ -2289,23 +2289,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)
> +		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;

OK, so we used to return NULL when the key is not parseable, and otherwise we returned the config_set_element we found for the key, or NULL if there is no such element. Now we return error code as the return value and allow the caller to peek the element via *dest parameter.

So, from the caller's point of view (dest != NULL) is how it checks if the key is used. The function returning 0 is a sign that the key passed to it is healthy. OK.

Show 10 quoted lines

> @@ -2314,8 +2319,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)
> +		return ret;

The function never returned any meaningful error, so the callers may not be prepared to see such an error return. But now we at least notice an error at this level.

Show 14 quoted lines

> @@ -2425,8 +2433,25 @@ 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)
>  {
> -	struct config_set_element *e = configset_find_element(cs, key);
> -	return e ? &e->value_list : NULL;
> +	struct config_set_element *e;
> +
> +	if (configset_find_element(cs, key, &e))
> +		return NULL;
> +	else if (!e)
> +		return NULL;
> +	return &e->value_list;
> +}

OK. !e means "we got a healthy key and peeking into the hash table, there wasn't any entry for the key", and that is reported with NULL. Do we evern return a string list with .nr == 0, I wonder. Having to deal with such a list would make the caller's job more complex, but perhaps we are not allowing the code to shrink value_list.nr to avoid such a situation?

Show 11 quoted lines

> +int git_configset_get(struct config_set *cs, const char *key)
> +{
> +	struct config_set_element *e;
> +	int ret;
> +
> +	if ((ret = configset_find_element(cs, key, &e)))
> +		return ret;
> +	else if (!e)
> +		return 1;
> +	return 0;
>  }

OK. So 0 return from the function means there is a value (or more) for a given key. Good.

Show 11 quoted lines

> diff --git a/config.h b/config.h
> index ef9eade6414..04c5e594015 100644
> --- a/config.h
> +++ b/config.h
> @@ -471,9 +471,12 @@ void git_configset_clear(struct config_set *cs);
>  
>  /*
>   * These functions return 1 if not found, and 0 if found, leaving the found
> - * value in the 'dest' pointer.
> + * value in the 'dest' pointer (if any).
>   */

Now the returned non-zero values are no longer 1 alone, no? Whatever lower-level functions use to signal an error is propagated up with the

	if ((ret = func())
		return ret;

pattern.