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

[Tanay Abhra <tanayabh@gmail.com>]

On 7/16/2014 9:36 PM, Matthieu Moy wrote:
> 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.

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

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