Re: [PATCH 2/7] rewrite git_config() to use the config-set API

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

Tanay Abhra <tanayabh@gmail.com> writes:

> Of all the functions in `git_config*()` family, `git_config()` has the
> most invocations in the whole code base. Each `git_config()` invocation
> causes config file rereads which can be avoided using the config-set API.
>
> Use the config-set API to rewrite `git_config()` to use the config caching
> layer to avoid config file rereads on each invocation during a git process
> lifetime. First invocation constructs the cache, and after that for each
> successive invocation, `git_config()` feeds values from the config cache
> instead of rereading the configuration files.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  config.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)

This is the natural logical conclusion ;-)

> diff --git a/config.c b/config.c
> index 6db8f97..a7fb9a4 100644
> --- a/config.c
> +++ b/config.c
> @@ -1232,11 +1232,36 @@ int git_config_with_options(config_fn_t fn, void *data,
>  	return ret;
>  }
>  
> -int git_config(config_fn_t fn, void *data)
> +static int git_config_raw(config_fn_t fn, void *data)
>  {
>  	return git_config_with_options(fn, data, NULL, 1);
>  }
>  
> +static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
> +{
> +	int i;
> +	struct string_list *strptr;

We know string_list would hold strings, so naming the variable
strptr does not give us much information.  As this is a list of
values you would get by querying for a variable (or "key"), perhaps
name it "values" or something?

> +	struct config_set_element *entry;
> +	struct hashmap_iter iter;

Have a blank line between the local variable definitions (above) and
the first executable statement (below); it would make it easier to
read, especially because out codebase do not have decl-after-stmt.

> +	hashmap_iter_init(&cs->config_hash, &iter);
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		strptr = &entry->value_list;
> +		for (i = 0; i < strptr->nr; i++) {
> +			if (fn(entry->key, strptr->items[i].string, data) < 0)
> +				die("bad config file line in (TODO: file/line info)");
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void git_config_check_init(void);
> +
> +int git_config(config_fn_t fn, void *data)
> +{
> +	git_config_check_init();
> +	return configset_iter(&the_config_set, fn, data);
> +}

Perhaps if you define this function at the right place in the file
you do not have to add an forward decl of git_config_check_init()?

>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>  {
>  	struct config_set_element k;
> @@ -1418,7 +1443,7 @@ static void git_config_check_init(void)
>  	if (the_config_set.hash_initialized)
>  		return;
>  	git_configset_init(&the_config_set);
> -	git_config(config_set_callback, &the_config_set);
> +	git_config_raw(config_set_callback, &the_config_set);
>  }
>  
>  void git_config_clear(void)