patch, 2 partsattr.c: check if .gitattributes is a file before parsing it

The topic nd/conditional-config-include hit a problem on Windows [1]. The test basically does this (much simplified)

    echo '[include]path=foo' >~/.gitconfig
    cd ~ && git init foo

At some point in 'git init' after 'foo' directory has been created, we request to include ~/foo as an extra config file. But it's a directory and we get some error like this

    fatal: bad config line 2 in file ~/.gitconfig

The message gives no clue that 'foo' is a directory (and probably wasted a good chunk of time of Johannes). This series tells the user about that.

The other half of the problem is, the same test runs without error on Linux because it looks like fopen(dir) returns NULL on Windows, but non-NULL on Linux and only subsequent read() returns EISDIR. Unfortunately the config parser conflates errors with eof, I think. And it simply sees <dir> as an empty config file, ie. a valid config file. So no "bad config line..."

I'm making sure even Linux now reports loud and clear that config files should be _files_. The same treatment is done for .gitattributes. I'm not so sure about .gitignore because it uses open(), not fopen() and I don't know if open() behaves differently on Windows.

I briefly considered fopen() and open() wrappers that always rejects directories (if you need to open a directory, do it without wrappers), but discarded it because it adds an extra stat() call everywhere.

[1] https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33#commitcomment-21121210

Nguyễn Thái Ngọc Duy (2):
  config: check if config path is a file before parsing it
  attr.c: check if .gitattributes is a file before parsing it

 abspath.c              | 7 +++++++
 attr.c                 | 8 +++++++-
 cache.h                | 1 +
 config.c               | 9 +++++++++
 t/t1300-repo-config.sh | 5 +++++
 5 files changed, 29 insertions(+), 1 deletion(-)

Similar to the previous patch, this is about better error messages when .gitattributes happens to be a directory.

FWIW .gitignore code is also checked. There open() is used instead and
open("dir") does not fail on Linux. But the next read should fail with
EISDIR, which is a pretty good clue already. No idea how open() on
Windows behaves.
---
 attr.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

If a directory is given as a config file by accident, we keep open it as a file. The behavior of fopen() in this case seems to be undefined.

On Linux, we open a directory as a file ok, then get error (which we consider eof) on the first read. So the config parser sees this "file" as empty (i.e. valid config). All is well and we don't complain anything (but we should).

The situation is slighly different on Windows. I think fopen() returns NULL. And we get a very unhelpful message:

    $ cat >abc <<EOF
    [include]
        path = /tmp/foo
    EOF
    $ mkdir /tmp/foo
    $ git config --includes --file=abc core.bare
    fatal: bad config line 3 in file abc

Opening a directory is wrong in the first place. Avoid it. If caught, print something better. With this patch, we have

    $ git config --includes --file=abc core.bare
    error: '/tmp/foo' is not a file
    fatal: bad config line 3 in file abc

It's not perfect (line should be 2 instead of 3). But it's definitely improving.

The new test is only relevant on linux where we blindly open the
directory and consider it an empty file. On Windows, the test should
pass even without this patch.
---
 abspath.c              | 7 +++++++
 cache.h                | 1 +
 config.c               | 9 +++++++++
 t/t1300-repo-config.sh | 5 +++++
 4 files changed, 22 insertions(+)

On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

I'm mildly negative on this approach for two reasons:

  1. It requires doing an _extra_ check anywhere we want to care about
     this. So if we care about file/directory confusion, we're going to
     sprinkle these is_not_file() checks all over the code base.

     I think we're much better to just do the thing we want to do (like
     open the file), and deal with the error results. I'm on the fence
     on whether we want to care about the fopen behavior on Linux here
     (where reading a directory essentially behaves like an empty file,
     because the first read() gives an error and we don't distinguish
     between error and EOF).  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.

  2. It doesn't address the root problem for git_config_from_file(),
     which is that it is quiet when fopen fails, even if the reason is
     something interesting besides ENOENT. The caller can't check errno
     because it doesn't know if fopen() failed, or if the config
     callback returned an error.

     There's an attempt to protect the call to git_config_from_file() by
     checking access(), but that breaks down when access() and fopen()
     have two different results (which is exactly what happens on
     Windows in this case).

-Peff

On Fri, Mar 3, 2017 at 4:53 PM, Jeff King <peff@peff.net> wrote:

I can't fix problems of my series on Windows because I don't use Windows (because I will not be able to verify it). So I'm definitely on the side that makes behavior consistent across platforms. Then I can at least verify some (assuming that the consistent behavior is the right one).

I didn't go with yours because I would have to handle two separate code paths (fopen returns NULL and read returns EISDIR). But yeah it should be that way even if it takes more time and effort. At least we're now back on the mailing list and I didn't have to hurry to get something out, to get off github.

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.

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

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

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

Something like this is a big improvement, I think:

On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> wrote:

stdio error handling is a pain. Maybe we're better of with open() and mmap() (or even read_in_full)? I/O error handling would be at the beginning, not buried deep in the parser. Hmm.. since we already have "fgetc' version for config blobs, this could kill some code...

On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote:

Yeah, I don't mind a read_in_full() version. Config isn't _supposed_ to be big (and if it is you're in trouble anyway, because I'm pretty sure we still parse it several times per command invocation).

I don't think that removes the issues I've mentioned with git_config_from_file() being too quiet. But it solves the ferror() question (though I think we pretty much return immediately from the parser on EOF, so it's _probably_ OK to use it like in the diff I just sent).

-Peff

Jeff King <peff@peff.net> writes:

That is the most sensible, I would think (assuming that we really get EISDIR instead of silent EOF).

That's already an option with FREAD_READS_DIRECTORIES, I think.

Perhaps like this one as a starting point, with FREAD_READS_DIRECTORIES?

 config.c | 3 +++
 1 file changed, 3 insertions(+)

On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:

Something is a bit odd here - nothing in this commit (that I can see, anyway) would require the addition of this include. Also, this include is already there in the 'pu' branch, brought in by your conditional include changes. So, ...

ATB, Ramsay Jones