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:33 UTC
Message-ID: <20170303103319.uei446qkt6jadsn5@sigill.intra.peff.net>
In-Reply-To: <CACsJy8CDOBUe7S3sYz=_tAJy2ajM5h2v+ZypD6Cr1uJo7XAnGQ@mail.gmail.com>

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

Show 10 quoted lines

> On Fri, Mar 3, 2017 at 5:15 PM, Jeff King <peff@peff.net> 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.
> 
> 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...

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