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@xxxxxxxxx> 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.
--
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