From: Jeff King <peff@peff.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 1/2] config: check if config path is a file before parsing it
Date: Fri, 3 Mar 2017 04:53:51 -0500 [thread overview]
Message-ID: <20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net> (raw)
In-Reply-To: <20170303094252.11706-2-pclouds@gmail.com>
On Fri, Mar 03, 2017 at 04:42:51PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 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.
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
next prev parent reply other threads:[~2017-03-03 11:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-03 9:42 [PATCH 0/2] Improve error messages when opening a directory as file Nguyễn Thái Ngọc Duy
2017-03-03 9:42 ` [PATCH 1/2] config: check if config path is a file before parsing it Nguyễn Thái Ngọc Duy
2017-03-03 9:53 ` Jeff King [this message]
2017-03-03 10:06 ` Duy Nguyen
2017-03-03 10:15 ` Jeff King
2017-03-03 10:29 ` Duy Nguyen
2017-03-03 10:33 ` Jeff King
2017-03-03 10:31 ` Jeff King
2017-03-03 21:05 ` Junio C Hamano
2017-03-03 20:21 ` Ramsay Jones
2017-03-03 9:42 ` [PATCH 2/2] attr.c: check if .gitattributes " Nguyễn Thái Ngọc Duy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).