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

Jeff King writes: >a. actually check ferror() after getting EOF and report the read > error. That catches EISDIR, along with any other unexpected > errors. That is the most sensible, I would think (assuming that we really get EISDIR instead of silent

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

On 03/03/17 09:42, 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

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

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 >

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

On Fri, Mar 3, 2017 at 5:15 PM, 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 >

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

On Fri, Mar 03, 2017 at 05:29:47PM +0700, Duy Nguyen wrote: > On Fri, Mar 3, 2017 at 5:15 PM, 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

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

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 >

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: > > 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. > > > >

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

On Fri, Mar 3, 2017 at 4:53 PM, Jeff King 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

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

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.