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

From: Tanay Abhra <tanayabh@gmail.com>
Date: Jul 16, 2014, 16:44 UTC
Message-ID: <53C6ABEB.3060205@gmail.com>
In-Reply-To: <vpqha2h9tjw.fsf@anie.imag.fr>

On 7/16/2014 9:36 PM, Matthieu Moy wrote:

Show 13 quoted lines

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

Yikes, sorry about that.

Show 13 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).
>

The errors (non accessible, non existent files etc) were already being caught by git_config_early(). Since git_config() only returns positive values except the weird race case you mentioned, I thought the die confused the reader of the patch more than it provided error checking. I also tried myself simulating the race condition but failed. All the callers of git_config() also ignore the return value, so I ended up ignoring the return value myself.

Show 11 quoted lines

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

After reading the commit log that you mentioned and some previous ones before that I surmised that the official slant was to silently ignore nonexistent files. Though an access_or_warn() check was placed on most of the files like git attributes, since non accessible file errors may be a user configuration error. So, I decided to ignore the return value.

But I do think that an access_or_warn() check should be put on git config --file and git_configset_add_file since other parts of git follow it. What do you think about it, still I will send followup patch correcting the git config --file condition where it silently ignores the file access error and continues?