Kyle Evans <kevans@xxxxxxxxxxx> writes: > On Sun, Jun 7, 2020 at 12:05 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> Kyle Evans <kevans@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxx/ 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. #
