Re: [PATCH 1/2] config: check if config path is a file before parsing it

From

Jeff King <peff@peff.net>

Date

Mar 3, 2017, 10:31 UTC

Message-ID

<20170303103115.an5dg2pacest4zwo@sigill.intra.peff.net>

In-Reply-To

<20170303101503.lf2ub2c7i6w7kg3t@sigill.intra.peff.net>

On Fri, Mar 03, 2017 at 05:15:03AM -0500, Jeff King wrote:

> But I do think option (a) is cleaner. The only trick is that for errno
> to be valid, we need to make sure we check ferror() soon after seeing
> the EOF return value. I suspect it would work OK in practice for the
> git_config_from_file() case.

Something like this is a big improvement, I think:

diff --git a/config.c b/config.c
index c6b874a7b..27b410dfe 100644
--- a/config.c
+++ b/config.c
@@ -156,15 +156,14 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		path = buf.buf;
 	}
 
-	if (!access_or_die(path, R_OK, 0)) {
-		if (++inc->depth > MAX_INCLUDE_DEPTH)
-			die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
-			    !cf ? "<unknown>" :
-			    cf->name ? cf->name :
-			    "the command line");
-		ret = git_config_from_file(git_config_include, path, inc);
-		inc->depth--;
-	}
+	if (++inc->depth > MAX_INCLUDE_DEPTH)
+		die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+		    !cf ? "<unknown>" :
+		    cf->name ? cf->name :
+		    "the command line");
+	ret = git_config_from_file(git_config_include, path, inc);
+	inc->depth--;
+
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;
@@ -1213,10 +1212,18 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 	FILE *f;
 
 	f = fopen(filename, "r");
-	if (f) {
+	if (!f) {
+		/* a missing file is silently treated as an empty one */
+		if (errno == ENOENT || errno == EISDIR)
+			ret = 0;
+		else
+			ret = error_errno("unable to open %s", filename);
+	} else {
 		flockfile(f);
 		ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
 		funlockfile(f);
+		if (!ret && ferror(f))
+			ret = error_errno("unable to read from %s", filename);
 		fclose(f);
 	}
 	return ret;

Then if you do:

  cd repo.git
  git config include.path this-is-broken

you get useful errors for a variety of situations:

  $ mkdir this-is-broken
  $ git rev-parse
  error: unable to read from this-is-broken: Is a directory
  fatal: bad config line 7 in file config

  $ rmdir this-is-broken
  $ ln -s this-is-broken this-is-broken
  $ git rev-parse
  error: unable to open this-is-broken: Too many levels of symbolic links
  fatal: bad config line 7 in file config

and so on. The two caveats are:

  1. A few of the callers treat EACCES specially, so we'd potentially
     want a flag for that (or alternatively, everybody should just fopen
     the file themselves and pass in the handle).

  2. The call in read_repository_format() does not check the return
     value at all. It measures errors only as "did the parser find a
     core.repositoryformatversion field I can look at", though arguably
     it should check for other errors, too (if we read "version=2", but
     then got a read error before we were able to read the extensions,
     that would be wrong and bad).

     But either way I suspect it probably prefers the current "quiet"
     behavior, since it is used to speculatively look for repositories.

So probably git_config_from_file() needs a flags parameter, and both
"quiet" and EACCES handling can go in there.

-Peff