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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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

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

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

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

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

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

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

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