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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 15, 2014, 11:32 UTC
Message-ID: <vpqiomy3li1.fsf@anie.imag.fr>
In-Reply-To: <53C50859.5070209@gmail.com>

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/