Re: fread reading directories

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2020, 17:18 UTC
Message-ID: <xmqqlfkxlbn4.fsf@gitster.c.googlers.com>
In-Reply-To: <CACNAnaHBPeg1SMMGUdErKnn12bGo8t3O7LU6Yktw40B7bKfBGA@mail.gmail.com>

Kyle Evans <kevans@freebsd.org> writes:

Show 18 quoted lines

> On Sun, Jun 7, 2020 at 12:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Kyle Evans <kevans@freebsd.org> writes:
>>
>> > I was looking at FREAD_READS_DIRECTORIES to measure some performance
>> > differences, then stumbled upon [0] that dropped fread() from the
>> > autoconf test that causes git to use its git_fopen shim [1] even on
>> > Linux.
>>
>> I thought we saw this mentioned recently?  I do not recall if
>> any concrete improvement came out of it.
>>
>
> Ah, this is my bad. =-( I had searched the archives (I'm not typically
> subscribed to this list) and noticed the related patch for GNU/Hurd,
> but completely missed that a more active discussion had taken place
> within that thread. I have now read that, and have no further
> questions.

For the benefit of those who are watching from sidelines, the relevant thread ends at

https://lore.kernel.org/git/20200424055106.GG1648190@coredump.intra.peff.net/

In short, many callers of fopen() in our code rely on our variant of fopen() that notices that the caller fed us a directory for error reporting. Unless the caller somehow knows the argument it calls fopen() with is a file and not a directory, somebody needs to fopen() and fstat() (or stat() and then fopen()) to catch it as an error to give that caller a directory. In the current arrangement, we let our fopen() wrapper do that task, instead of the callers.

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?

 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 09f98b777c..a0bef206a9 100644
--- a/Makefile
+++ b/Makefile
@@ -19,8 +19,7 @@ all::
 # have been written to the final string if enough space had been available.
 #
 # Define FREAD_READS_DIRECTORIES if you are on a system which succeeds
-# when attempting to read from an fopen'ed directory (or even to fopen
-# it at all).
+# when an fopen() on a directory does not result in an error.
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 #