{"thread":{"id":"53625","subject":"fread reading directories","startedAt":"2020-06-06T22:37:15Z","lastAt":"2020-06-08T19:41:36Z","messageCount":6,"participants":["Kyle Evans","Junio C Hamano","Brandon Casey","Randall S. Becker"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"399235","messageId":"CACNAnaG19QD1PbVS93nFm3XY70CZCrRosmVq-_3j+puAKSPj9Q@mail.gmail.com","threadId":"53625","inReplyTo":null,"subject":"fread reading directories","fromName":"Kyle Evans","fromEmail":"kevans@freebsd.org","sentAt":"2020-06-06T22:36:58Z","receivedAt":"2020-06-06T22:37:15Z","isPatch":false,"sender":{"key":"kevans@freebsd.org","avatar":null},"body":"Hi,\n\nI was looking at FREAD_READS_DIRECTORIES to measure some performance\ndifferences, then stumbled upon [0] that dropped fread() from the\nautoconf test that causes git to use its git_fopen shim [1] even on\nLinux.\n\nI've read the commit message a couple of times, but I'm really not\nseeing the rationale for *why* git wants this knob to be set on Linux.\n\nUnless I'm missing something, this would seem to regress the almost\ncertainly much-more-common case of fopen() a file and fread() it with\nan unconditional fstat() from grep_fopen(), rather than just using two\nsyscalls at all times (directories and non-directories) and letting it\nget rejected in fread().\n\nThoughts?\n\nThanks,\n\nKyle Evans\n\n[0] https://github.com/git/git/commit/3adf9fdecfb0cd31a83ef3af1d8d631a1acd392b\n[1] https://github.com/git/git/blob/master/compat/fopen.c\n"},{"id":"399250","messageId":"xmqqd06an6wf.fsf@gitster.c.googlers.com","threadId":"53625","inReplyTo":"CACNAnaG19QD1PbVS93nFm3XY70CZCrRosmVq-_3j+puAKSPj9Q@mail.gmail.com","subject":"Re: fread reading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-07T17:05:20Z","receivedAt":"2020-06-07T17:05:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Evans writes:\n\n> I was looking at FREAD_READS_DIRECTORIES to measure some performance\n> differences, then stumbled upon [0] that dropped fread() from the\n> autoconf test that causes git to use its git_fopen shim [1] even on\n> Linux.\n\nI thought we saw this mentioned recently? I do not recall if\nany concrete improvement came out of it.\n\nThe Makefile defines the macro as such:\n\n# Define FREAD_READS_DIRECTORIES if you are on a system which succeeds\n# when attempting to read from an fopen'ed directory (or even to fopen\n# it at all).\n\nSo, the macro is expected to be set if a platform gives back FILE *\non a directory, whether it allows fread() on it or not.\n\nIf it is a good idea is entirely different story, though.\n"},{"id":"399252","messageId":"CACNAnaHBPeg1SMMGUdErKnn12bGo8t3O7LU6Yktw40B7bKfBGA@mail.gmail.com","threadId":"53625","inReplyTo":"xmqqd06an6wf.fsf@gitster.c.googlers.com","subject":"Re: fread reading directories","fromName":"Kyle Evans","fromEmail":"kevans@freebsd.org","sentAt":"2020-06-07T17:16:26Z","receivedAt":"2020-06-07T17:16:40Z","isPatch":false,"sender":{"key":"kevans@freebsd.org","avatar":null},"body":"On Sun, Jun 7, 2020 at 12:05 PM Junio C Hamano wrote:\n>\n> Kyle Evans writes:\n>\n> > I was looking at FREAD_READS_DIRECTORIES to measure some performance\n> > differences, then stumbled upon [0] that dropped fread() from the\n> > autoconf test that causes git to use its git_fopen shim [1] even on\n> > Linux.\n>\n> I thought we saw this mentioned recently? I do not recall if\n> any concrete improvement came out of it.\n>\n\nAh, this is my bad. =-( I had searched the archives (I'm not typically\nsubscribed to this list) and noticed the related patch for GNU/Hurd,\nbut completely missed that a more active discussion had taken place\nwithin that thread. I have now read that, and have no further\nquestions.\n\nThanks!\n\nKyle Evans\n"},{"id":"399306","messageId":"xmqqlfkxlbn4.fsf@gitster.c.googlers.com","threadId":"53625","inReplyTo":"CACNAnaHBPeg1SMMGUdErKnn12bGo8t3O7LU6Yktw40B7bKfBGA@mail.gmail.com","subject":"Re: fread reading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T17:18:07Z","receivedAt":"2020-06-08T17:18:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Evans writes:\n\n> On Sun, Jun 7, 2020 at 12:05 PM Junio C Hamano wrote:\n>>\n>> Kyle Evans writes:\n>>\n>> > I was looking at FREAD_READS_DIRECTORIES to measure some performance\n>> > differences, then stumbled upon [0] that dropped fread() from the\n>> > autoconf test that causes git to use its git_fopen shim [1] even on\n>> > Linux.\n>>\n>> I thought we saw this mentioned recently? I do not recall if\n>> any concrete improvement came out of it.\n>>\n>\n> Ah, this is my bad. =-( I had searched the archives (I'm not typically\n> subscribed to this list) and noticed the related patch for GNU/Hurd,\n> but completely missed that a more active discussion had taken place\n> within that thread. I have now read that, and have no further\n> questions.\n\nFor the benefit of those who are watching from sidelines, the\nrelevant thread ends at\n\nhttps://lore.kernel.org/git/20200424055106.GG1648190@coredump.intra.peff.net/\n\nIn short, many callers of fopen() in our code rely on our variant of\nfopen() that notices that the caller fed us a directory for error\nreporting. Unless the caller somehow knows the argument it calls\nfopen() with is a file and not a directory, somebody needs to\nfopen() and fstat() (or stat() and then fopen()) to catch it as an\nerror to give that caller a directory. In the current arrangement, \nwe let our fopen() wrapper do that task, instead of the callers.\n\nIt may make sense to do one of the two things:\n\n - The lighter weight one is to rename the macro to the reflect the\n trait we are trying to capture more faithfully: \"fopen opens\n directories\" and leave the code and performance characteristics\n as-is.\n\n - Heavier weight one is to audit callers of fopen() and only let\n those that know they do not have a directory directly call\n fopen(). The other callers would call our wrapper under a\n different name. This way, the former won't have to pay the\n overhead of checking for \"you gave me a directory but I only take\n a file\" error twice. This is what Brandon proposed in the\n thread.\n\nDoing neither would leave this seed of confusion for later readers,\nwhich is not ideal. I am tempted to say that we for now should do\nan even lighter variant of the former, which is to give a comment.\n\nThoughts?\n\n Makefile | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 09f98b777c..a0bef206a9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -19,8 +19,7 @@ all::\n # have been written to the final string if enough space had been available.\n #\n # Define FREAD_READS_DIRECTORIES if you are on a system which succeeds\n-# when attempting to read from an fopen'ed directory (or even to fopen\n-# it at all).\n+# when an fopen() on a directory does not result in an error.\n #\n # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n #\n"},{"id":"399310","messageId":"CA+sFfMcQ+HQPk3SMsBhWjfLiVLzfhHSv9OpzPHAJt5b50TEPeQ@mail.gmail.com","threadId":"53625","inReplyTo":"xmqqlfkxlbn4.fsf@gitster.c.googlers.com","subject":"Re: fread reading directories","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-06-08T19:08:21Z","receivedAt":"2020-06-08T19:08:35Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Jun 8, 2020 at 10:18 AM Junio C Hamano wrote:\n>\n> It may make sense to do one of the two things:\n>\n> - The lighter weight one is to rename the macro to the reflect the\n> trait we are trying to capture more faithfully: \"fopen opens\n> directories\" and leave the code and performance characteristics\n> as-is.\n>\n> - Heavier weight one is to audit callers of fopen() and only let\n> those that know they do not have a directory directly call\n> fopen(). The other callers would call our wrapper under a\n> different name. This way, the former won't have to pay the\n> overhead of checking for \"you gave me a directory but I only take\n> a file\" error twice. This is what Brandon proposed in the\n> thread.\n>\n> Doing neither would leave this seed of confusion for later readers,\n> which is not ideal. I am tempted to say that we for now should do\n> an even lighter variant of the former, which is to give a comment.\n>\n> Thoughts?\n\nI'd suggest a medium weight approach which would be to introduce a new\nfunction with an appropriate name (fopen_file_only()?) that behaves\nthe way we want it to, and replace every existing fopen() call with\nthis new function. We could introduce a new macro, which I think\nwould only be used on Windows, to say \"fopen already fails to open\ndirectories\" (FOPEN_FAILS_ON_DIRECTORIES?) so that fopen_file_only\ncould be simplified to just a bare fopen there. That way it's clear to\nthe reader, at the callsite, that the call does not have the standard\nbehavior of fopen.\n\nThen, FREAD_READS_DIRECTORIES could be removed from all but the 1 or 2\nplatforms that it was originally set for. I'd imagine that we'd\nbasically just promote the git_fopen() function from compat to become\nthe implementation of the first tier fopen_file_only() function. On\nthe FREAD_READS_DIRECTORIES platforms, a bare fopen would also become\nfopen_file_only(). The call to fopen() within fopen_file_only() would\nobviously need to take this into account to ensure that it calls the\nreal fopen().\n\nI think this would put the pieces in place for someone to audit all of\nthe existing uses of fopen_file_only() and potentially replace them\nwith a straight fopen() if appropriate. And it would allow future code\nto explicitly make the choice between fopen_file_only() or just\nfopen().\n\nNone of this would produce any functional change on any of our\nplatforms, but I think it would make things more clear.\n\n-Brandon\n"},{"id":"399313","messageId":"013101d63dcc$c5fc5740$51f505c0$@nexbridge.com","threadId":"53625","inReplyTo":"CA+sFfMcQ+HQPk3SMsBhWjfLiVLzfhHSv9OpzPHAJt5b50TEPeQ@mail.gmail.com","subject":"RE: fread reading directories","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-06-08T19:41:10Z","receivedAt":"2020-06-08T19:41:36Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On June 8, 2020 3:08 PM, Brandon Casey wrote:\n> To: Junio C Hamano \n> Cc: git ; Kyle Evans ; Jeff King\n> \n> Subject: Re: fread reading directories\n> \n> On Mon, Jun 8, 2020 at 10:18 AM Junio C Hamano \n> wrote:\n> >\n> > It may make sense to do one of the two things:\n> >\n> > - The lighter weight one is to rename the macro to the reflect the\n> > trait we are trying to capture more faithfully: \"fopen opens\n> > directories\" and leave the code and performance characteristics\n> > as-is.\n> >\n> > - Heavier weight one is to audit callers of fopen() and only let\n> > those that know they do not have a directory directly call\n> > fopen(). The other callers would call our wrapper under a\n> > different name. This way, the former won't have to pay the\n> > overhead of checking for \"you gave me a directory but I only take\n> > a file\" error twice. This is what Brandon proposed in the\n> > thread.\n> >\n> > Doing neither would leave this seed of confusion for later readers,\n> > which is not ideal. I am tempted to say that we for now should do an\n> > even lighter variant of the former, which is to give a comment.\n> >\n> > Thoughts?\n> \n> I'd suggest a medium weight approach which would be to introduce a new\n> function with an appropriate name (fopen_file_only()?) that behaves the way\n> we want it to, and replace every existing fopen() call with this new function.\n> We could introduce a new macro, which I think would only be used on\n> Windows, to say \"fopen already fails to open directories\"\n> (FOPEN_FAILS_ON_DIRECTORIES?) so that fopen_file_only could be\n> simplified to just a bare fopen there. That way it's clear to the reader, at the\n> callsite, that the call does not have the standard behavior of fopen.\n> \n> Then, FREAD_READS_DIRECTORIES could be removed from all but the 1 or 2\n> platforms that it was originally set for. I'd imagine that we'd basically just\n> promote the git_fopen() function from compat to become the\n> implementation of the first tier fopen_file_only() function. On the\n> FREAD_READS_DIRECTORIES platforms, a bare fopen would also become\n> fopen_file_only(). The call to fopen() within fopen_file_only() would\n> obviously need to take this into account to ensure that it calls the real\n> fopen().\n> \n> I think this would put the pieces in place for someone to audit all of the\n> existing uses of fopen_file_only() and potentially replace them with a straight\n> fopen() if appropriate. And it would allow future code to explicitly make the\n> choice between fopen_file_only() or just fopen().\n> \n> None of this would produce any functional change on any of our platforms,\n> but I think it would make things more clear.\n\nPlease 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.\n\nThanks,\nRandall\n\n"}]}