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

[Tanay Abhra <tanayabh@gmail.com>]

On 7/11/2014 7:51 PM, Matthieu Moy wrote:
> Hi,
> 
> I had a closer look at error management (once more, sorry: I should have
> done this earlier...), and it seems to me that:
> 
> * Not all errors are managed properly
> 
> * Most error cases are untested
> 
> Among the cases I can think of:
> 
> * Syntax error when parsing the file
> 
> * Non-existant file
> 
> * Unreadable file (chmod -r)
>

I had seen that there were checks for Syntax error or Non-existant files in
t1300-repo-config, for example,

# malformed configuration files
test_expect_success 'barf on syntax error' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section]
	key garbage
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 3 " error
'

test_expect_success 'barf on incomplete section header' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section
	key = value
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 2 " error
'

test_expect_success 'barf on incomplete string' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section]
	key = "value string
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 3 " error
'
test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
	test_must_fail git config --file non-existing-config -l
'

They cover the same parsing code which I used for constructing the cache.
Still, more tests will not harm anyone, I will add more testcases accordingly
for the corner cases you raised. :)

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
>> +
>> +	Parses the file and adds the variable-value pairs to the `config_set`,
>> +	dies if there is an error in parsing the file.
> 
> The return value is undocumented.
> 
> If I read correctly, the only codepath from this to a syntax error sets
> die_on_error, hence "dies if there is an error in parsing the file" is
> correct.
> 
> Still, there are errors like "unreadable file" or "no such file" that do
> not die (nor emit any error message, which is not very good for the
> user), and lead to returning -1 here.
> 
> I'm not sure this distinction is right (why die on syntax error and
> continue running on unreadable file?).
> 
> In any case, it should be documented and tested. I'll send a fixup patch
> with a few more example tests (probably insufficient).
>

I am not sure of this myself, I will have to look into it.

>> +static int git_config_check_init(void)
>> +{
>> +	int ret = 0;
>> +	if (the_config_set.hash_initialized)
>> +		return 0;
>> +	configset_init(&the_config_set);
>> +	ret = git_config(config_hash_callback, &the_config_set);
>> +	if (ret >= 0)
>> +		return 0;
>> +	else {
>> +		hashmap_free(&the_config_set.config_hash, 1);
>> +		the_config_set.hash_initialized = 0;
>> +		return -1;
>> +	}
>> +}
> 
> We have the same convention for errors here. But a more serious issue is
> that the return value of this function is ignored most of the time.
> 
> It seems git_config should never return a negative value, as it calls
> git_config_with_options -> git_config_early, which checks for file
> existance and permission before calling git_config_from_file. Indeed,
> Git's tests still pass after this:
> 
> --- a/config.c
> +++ b/config.c
> @@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
>  
>  int git_config(config_fn_t fn, void *data)
>  {
> -       return git_config_with_options(fn, data, NULL, 1);
> +       int ret = git_config_with_options(fn, data, NULL, 1);
> +       if (ret < 0)
> +               die("Negative return value in git_config");
> +       return ret;
>  }
> 
> Still, we can imagine cases like race condition between access_or_die()
> and git_config_from_file() in
> 
> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
> 					    data);
> 		found += 1;
> 	}
> 
> where the function would indeed return -1. In any case, either we
> consider that git_config should never return -1, and we should die in
> this case, or we consider that it may happen, and that the "else" branch
> of the if/else above is not dead code, and then we can't just ignore the
> return value.
> 
> I think we should just do something like this:
> 
> diff --git a/config.c b/config.c
> index 74adbbd..5c023e8 100644
> --- a/config.c
> +++ b/config.c
> @@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
>                 return 1;
>  }
>  
> -static int git_config_check_init(void)
> +static void git_config_check_init(void)
>  {
>         int ret = 0;
>         if (the_config_set.hash_initialized)
> @@ -1437,11 +1437,8 @@ static int git_config_check_init(void)
>         ret = git_config(config_hash_callback, &the_config_set);
>         if (ret >= 0)
>                 return 0;
> -       else {
> -               hashmap_free(&the_config_set.config_hash, 1);
> -               the_config_set.hash_initialized = 0;
> -               return -1;
> -       }
> +       else
> +               die("Unknown error when parsing one of the configuration files.");
>  }
>  
> If not, a comment should explain what the "else" branch corresponds to,
> and why/when the return value can be safely ignored.
>

Yes, this is much better, I will make the necessary changes, thanks.