Re: [PATCH v9r2 1/2] add `config_set` API for caching config-like files

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 16, 2014, 16:06 UTC
Message-ID: <vpqha2h9tjw.fsf@anie.imag.fr>
In-Reply-To: <53C66E70.3050507@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 8 quoted lines

> implemented as a thin wrapper around the `config_set` API.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>  Documentation/technical/api-config.txt | 137 +++++++++++++++++
>  cache.h                                |  30 ++++
>  config.c                               | 264 +++++++++++++++++++++++++++++++++
>  3 files changed, 431 insertions(+)

(you broke the patch by removing the ---)

Show 7 quoted lines

> +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);
> +}

So, you're now ignoring the return value of git_config. What is the rationale for this? In particular, why did you reject the "die" possibility (I understood that you were inclined to take this option, so I'm curious why you changed your mind).

OTOH, you're transmitting the return value without dying here:

+int git_configset_add_file(struct config_set *cs, const char *filename) +{ + return git_config_from_file(config_set_callback, filename, cs); +}

and I think this one is correct, as we cannot tell in advance how serious an error would be for any callers. And we do test this (I think we can improve a bit, I'll send a fixup patch).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/