Re: [PATCH v8 1/2] add `config_set` API for caching config-like files
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jul 11, 2014, 14:21 UTC
- Message-ID
- <vpqlhs02cz7.fsf@anie.imag.fr>
- In-Reply-To
- <1405049655-4265-2-git-send-email-tanayabh@gmail.com>
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).
Show 15 quoted lines
> +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/