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

From: Glen Choo <chooglen@google.com>
Date: Feb 6, 2023, 12:37 UTC
Message-ID: <kl6lttzzgebw.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v4-3.9-998b11ae4bc-20230202T131155Z-avarab@gmail.com>

I think introducing a new function to replace the *_get_value_multi() abuses is a good way forward. Junio has already adequately commented on your implementation, so I'll focus this review on a different approach that this patch could have taken.

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

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

[...]

> We could have changed git_configset_get_value_multi() to accept a
> "NULL" as a "dest" for all callers, but let's avoid changing the
> behavior of existing API users. Having an "unused" value that we throw
> away internal to config.c is cheap.

There is yet another option, which is to teach "git_config_get_value()" (mentioned earlier) to accept NULL to mean "I just want to know if there is a value, I don't care what it is". That's what the *_get_<type>() functions use under the hood (i.e. the ones that return either 0 or 1 or exit).

This amounts to implementing the "*_config_key_exists()" API you mentioned, but I think this is better fit for the current set of semantics. At the very least, that would be an easy 1-1 replacement for the *_get_string[_tmp]() replacements we make here. There's also the small benefit of saving one function implementation.

Show 9 quoted lines

> Another name for this function could have been
> "*_config_key_exists()", as suggested in [1]. That would work for all
> of these callers, and would currently be equivalent to this function,
> as the git_configset_get_value() API normalizes all non-zero return
> values to a "1".
>
> But adding that API would set us up to lose information, as e.g. if
> git_config_parse_key() in the underlying configset_find_element()
> fails we'd like to return -1, not 1.

We were already 'losing' (or rather, not caring about) this information with the *_get_<type>() functions. The only reason we'd care about this is if we using git_configset_get_value_multi() or similar.

We replace two callers of git_configset_get_value_multi() in this patch, but they didn't care about the -1 case anyway...

Show 20 quoted lines

> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -557,7 +557,7 @@ static int module_init(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);
>  
>  	info.prefix = prefix;
> @@ -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);
>  
>  		info.prefix = opt.prefix;

Here they are.

Show 15 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).
>   */
>  
> +RESULT_MUST_BE_USED
> +int git_configset_get(struct config_set *cs, const char *key);
> +

As Junio pointed out, git_configset_get() can now return -1, so this isn't so accurate any more. git_configset_get() is really the exception here, since all the other functions in this section are the git_configset_get_*() functions that use git_configset_get_value(). I'd prefer returning only 0 or 1 for consistency.

Show 15 quoted lines

>  /*
>   * Finds the highest-priority value for the configuration variable `key`
>   * and config set `cs`, stores the pointer to it in `value` and returns 0.
> @@ -494,6 +497,14 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
>  /* Functions for reading a repository's config */
>  struct repository;
>  void repo_config(struct repository *repo, config_fn_t fn, void *data);
> +
> +/**
> + * Run only the discover part of the repo_config_get_*() functions
> + * below, in addition to 1 if not found, returns negative values on
> + * error (e.g. if the key itself is invalid).
> + */
> +RESULT_MUST_BE_USED
> +int repo_config_get(struct repository *repo, const char *key);

This comment is quite a welcome addition. I've found myself losing track of this information quite often