RE: fread reading directories

["Randall S. Becker" <rsbecker@nexbridge.com>]

On June 8, 2020 3:08 PM, Brandon Casey wrote:
> To: Junio C Hamano <gitster@pobox.com>
> Cc: git <git@vger.kernel.org>; Kyle Evans <kevans@freebsd.org>; Jeff King
> <peff@peff.net>
> Subject: Re: fread reading directories
> 
> On Mon, Jun 8, 2020 at 10:18 AM Junio C Hamano <gitster@pobox.com>
> wrote:
> >
> > It may make sense to do one of the two things:
> >
> >  - The lighter weight one is to rename the macro to the reflect the
> >    trait we are trying to capture more faithfully: "fopen opens
> >    directories" and leave the code and performance characteristics
> >    as-is.
> >
> >  - Heavier weight one is to audit callers of fopen() and only let
> >    those that know they do not have a directory directly call
> >    fopen().  The other callers would call our wrapper under a
> >    different name.  This way, the former won't have to pay the
> >    overhead of checking for "you gave me a directory but I only take
> >    a file" error twice.  This is what Brandon proposed in the
> >    thread.
> >
> > Doing neither would leave this seed of confusion for later readers,
> > which is not ideal.  I am tempted to say that we for now should do an
> > even lighter variant of the former, which is to give a comment.
> >
> > Thoughts?
> 
> I'd suggest a medium weight approach which would be to introduce a new
> function with an appropriate name (fopen_file_only()?) that behaves the way
> we want it to, and replace every existing fopen() call with this new function.
> We could introduce a new macro, which I think would only be used on
> Windows, to say "fopen already fails to open directories"
> (FOPEN_FAILS_ON_DIRECTORIES?) so that fopen_file_only could be
> simplified to just a bare fopen there. That way it's clear to the reader, at the
> callsite, that the call does not have the standard behavior of fopen.
> 
> Then, FREAD_READS_DIRECTORIES could be removed from all but the 1 or 2
> platforms that it was originally set for. I'd imagine that we'd basically just
> promote the git_fopen() function from compat to become the
> implementation of the first tier fopen_file_only() function.  On the
> FREAD_READS_DIRECTORIES platforms, a bare fopen would also become
> fopen_file_only(). The call to fopen() within fopen_file_only() would
> obviously need to take this into account to ensure that it calls the real
> fopen().
> 
> I think this would put the pieces in place for someone to audit all of the
> existing uses of fopen_file_only() and potentially replace them with a straight
> fopen() if appropriate. And it would allow future code to explicitly make the
> choice between fopen_file_only() or just fopen().
> 
> None of this would produce any functional change on any of our platforms,
> but I think it would make things more clear.

Please keep me on the loop on this one. The NonStop platforms have FREAD_READS_DIRECTORIES UnfortunatelyYes. We will want to move to the new structure as soon as we can, so compat makes me comfortable.

Thanks,
Randall