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

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

Tanay Abhra <tanayabh@gmail.com> writes:

> I checked the whole codebase and in all of the cases if they cannot read a file
> they return -1 and continue running.

More precisely: in git_config_from_file, any fopen failure results in
"return -1". But in git_config_early (one caller of
git_config_from_file()), the file is checked before access, e.g.:

	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;
	}

Essentially, if a config file does not exist, it's skipped (obviously
desirable), but if some really weird error occur (if "err == ENOENT ||
err == ENOTDIR || ((flag & ACCESS_EACCES_OK) && err == EACCES" is false,
from access_eacces_ok() in wrapper.c), then the process dies.

"Permission denied" errors are allowed for user-wide config, but not for
others. Read the log for commit 4698c8feb for more details.

Anyway, this is the part of the code you're not touching.

(I actually consider it as a bug that "git config --file no-such-file
foo.bar" and "git config --file unreadable-file foo.bar" fail without
error message, but your commit does not change this).

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

In the case of git_config_check_init(), I think it makes sense to die,
because file accesses are protected with access_or_die(), so the return
value can be negative only if something really went wrong.

If you chose not to die, then you should check the return value in the
callers of git_config_check_init().

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/