On 7/16/2014 9:36 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@xxxxxxxxx> writes:
>
>> implemented as a thin wrapper around the `config_set` API.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@xxxxxxx>
>> Signed-off-by: Tanay Abhra <tanayabh@xxxxxxxxx>
>> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html