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

On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote:

quoted

But if we do, I think we'd either want to:

       a. actually check ferror() after getting EOF and report the read
          error. That catches EISDIR, along with any other unexpected
          errors.

       b. use an fopen wrapper that checks fstat(fileno(fh)) after the
          open, and turns fopen(some_dir) into an error.

If you don't like extra check, I guess you're negative on b as well
since it is an extra check on Windows. That leaves us with option a.

I don't mind _doing_ the extra check that much. I don't think we fopen
so many files that an extra fstat on each would kill us. I mostly just
don't like having to sprinkle the explicit call to it everywhere. I'd be
OK with:

  FILE *xfopen(const char *path, const char *mode)
  {
	FILE *ret = fopen(path, mode);
  #ifdef FOPEN_OPENS_DIRECTORIES
	if (ret) {
		struct stat st;
		if (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) {
			fclose(ret);
			ret = NULL;
		}
	}
  #endif
	return ret;
  }

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.

-Peff