On Wed, Apr 22, 2020 at 12:58 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Wed, Apr 22, 2020 at 12:13:50PM -0700, Brandon Casey wrote:
>
> > On Wed, Apr 22, 2020 at 11:48 AM Brandon Casey <drafnel@xxxxxxxxx> wrote:
> > >
> > > I just looked in config.mak.uname, and I'm surprised to see
> > > FREAD_READS_DIRECTORIES set for so many platforms. And it's set for
> > > Linux and Darwin?!?!? Junio added it for Darwin
> > > (8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy
> > > added it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but
> > > also seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as
> > > being about the fopen(..., "r") of a directory rather than about an
> > > fread() of a directory.
> > >
> > > I just wrote a test program and tested on Linux, Darwin, and Windows.
> > > Linux and Darwin both succeed to fopen() a directory and fail to
> > > fread() it, as expected. Windows fails to fopen() a directory.
> > >
> > > I notice this earlier commit mentions a failure of t1308
> > > (4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the
> > > reason FREAD_READS_DIRECTORIES was added to so many platforms?
> >
> > Whoops, I got the order of e2d90fd1c33ae57e4a6da5729ae53876107b3463
> > and 4e3ecbd43958b1400d6cb85fe5529beda1630e3a wrong. Looks like the
> > misunderstanding of FREAD_READS_DIRECTORIES in e2d90fd could have been
> > the cause of all of this. That commit introduced the test t1308 and
> > added FREAD_READS... to Linux, kFreeBSD, and FreeBSD, and the other
> > additions followed shortly after.
>
> I think the code is actually doing the right thing (or at least
> something useful), and the "FREAD" in the name is the confusing part.
> Because there's now code doing:
>
> fh = fopen(fn, "r");
> if (!fh) {
> if (errno == ENOENT || errno == EISDIR) {
> /* not actually a file; treat as a gentle noop */
> return 0;
> } else {
> die_errno("omg, a real open error");
> }
> }
> if (!fread(..., fh))
> die_errno("omg, a real read error");
>
> which is exactly what the failing test in t1308 is doing.
>
> I know that wasn't the original intent of the flag, but I think it was a
> conscious decision to build on around the time of e2d90fd1c, when we
> started actually checking fopen() return values (as opposed to just
> segfaulting).
Our policy has always been to check return values hasn't it?
> And in practice, do we really care about cases that can fopen a
> directory but refuse to read from it? It's simpler and more efficient to
> catch this case up front.
I'm not sure I agree that it's simpler and more efficient to catch
this up front. Catching the case where a directory is supplied when
only a file is valid is an error path which we generally do not
optimize for, and it requires us to add an extra stat to every fopen()
call. We should have always been checking both the fopen() and any
reads and handle a failure in either one. So normally we wouldn't have
to do anything special to produce an error for the case of a directory
being supplied.
Now, if you want to ignore when a directory is supplied, and not
produce an error, I would expect the code to actually check for that
explicitly/clearly and not depend on fopen() failing, since that is
not a common behavior.
> So I think the knob has de facto become "do we need to use our compat
> wrapper to make opening a directory fail with EISDIR". And any attempt
> to change that will mean adapting all of the callers to handle that case
> themselves. I think what we have now is the useful knob we want; it's
> just misnamed (and obviously I don't blame your original patch; it was
> adapted over time).
We've generally taken the approach that there is an expected "normal"
behavior for the c library, generally the linux/glibc behavior. Then,
for platforms that behaved differently from what we've defined as
"normal", we'd introduce a compatibility function to make them behave
the way we wanted them to behave, or as close to that as possible. But
what you're suggesting here seems different. You're suggesting that we
should modify the behavior of fopen from what is commonly considered
"normal" so that it behaves in a new uncommon way. That doesn't seem
like the right thing to do.
Instead, I would think it would be better to introduce a new function
that has the behavior we want and to explicitly call that function
instead of pretending that we're calling fopen(). Otherwise it just
leads to confusion since _our_ fopen() doesn't actually work the way
fopen() "normally" works on our common platform. Maybe call it
fopen_file_only() or something. I've been away from git development
for too long to know whether most fopen callsites would need to use an
fopen_file_only() function or whether it would just be a few special
instances, and the rest could just use a regular fopen().
-Brandon
