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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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

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