From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Wed, 16 Jul 2014 16:06:59 GMT
Subject: Re: [PATCH v9r2 1/2] add `config_set` API for caching config-like files
Message-ID: <vpqha2h9tjw.fsf@anie.imag.fr>
In-Reply-To: <53C66E70.3050507@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

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

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