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)
Tanay Abhra [off-list ref] 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).
+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.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
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 [off-list ref] writes:
quoted
+`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.
quoted hunk
quoted
+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.
On 7/11/2014 7:51 PM, Matthieu Moy wrote:
quoted
+`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?).
I checked the whole codebase and in all of the cases if they cannot read a file
they return -1 and continue running. So, I have updated the API to document the
return value.
I think if the file is unreadable. we must continue running as no harm has been
done yet, worse is parsing a file with wrong syntax which can cause reading
wrong config values. So the decision to die on syntax error sounds alright
to me.
Cheers,
Tanay Abhra.