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

From: Glen Choo <chooglen@google.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Elijah Newren" <newren@gmail.com>, "Jeff King" <peff@peff.net>,
	"Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Calvin Wan" <calvinwan@google.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	raymond@heliax.dev, zweiss@equinix.com,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v4 3/9] config API: add and use a "git_config_get()" family of functions
Date: Mon, 06 Feb 2023 20:37:07 +0800	[thread overview]
Message-ID: <kl6lttzzgebw.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
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:

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

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

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

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

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