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