{"thread":{"id":"53285","subject":"[PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","startedAt":"2020-04-22T15:34:06Z","lastAt":"2020-04-24T05:51:08Z","messageCount":12,"participants":["Jessica Clarke","Jonathan Nieder","Junio C Hamano","Brandon Casey","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395905","messageId":"20200422153347.40018-1-jrtc27@jrtc27.com","threadId":"53285","inReplyTo":null,"subject":"[PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Jessica Clarke","fromEmail":"jrtc27@jrtc27.com","sentAt":"2020-04-22T15:33:47Z","receivedAt":"2020-04-22T15:34:06Z","isPatch":true,"sender":{"key":"jrtc27@jrtc27.com","avatar":"https://avatars.githubusercontent.com/u/816232?v=4"},"body":"GNU/Hurd is another platform that behaves like this. Set it to\nUnfortunatelyYes so that config directory files are correctly processed.\nThis fixes the corresponding 'proper error on directory \"files\"' test in\nt1308-config-set.sh.\n\nThanks-to: Jeff King <peff@peff.net>\nSigned-off-by: Jessica Clarke <jrtc27@jrtc27.com>\n---\n config.mak.uname | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 0ab8e00938..3e526f6b9f 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -308,6 +308,7 @@ ifeq ($(uname_S),GNU)\n \tNO_STRLCPY = YesPlease\n \tHAVE_PATHS_H = YesPlease\n \tLIBC_CONTAINS_LIBINTL = YesPlease\n+\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n endif\n ifeq ($(uname_S),IRIX)\n \tNO_SETENV = YesPlease\n-- \n2.20.1\n\n"},{"id":"395912","messageId":"20200422164150.GA140314@google.com","threadId":"53285","inReplyTo":"20200422153347.40018-1-jrtc27@jrtc27.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T16:41:50Z","receivedAt":"2020-04-22T16:41:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jessica Clarke wrote:\n\n> GNU/Hurd is another platform that behaves like this. Set it to\n> UnfortunatelyYes so that config directory files are correctly processed.\n> This fixes the corresponding 'proper error on directory \"files\"' test in\n> t1308-config-set.sh.\n>\n> Thanks-to: Jeff King <peff@peff.net>\n> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>\n> ---\n>  config.mak.uname | 1 +\n>  1 file changed, 1 insertion(+)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\nThanks.\n\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 0ab8e00938..3e526f6b9f 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -308,6 +308,7 @@ ifeq ($(uname_S),GNU)\n>  \tNO_STRLCPY = YesPlease\n>  \tHAVE_PATHS_H = YesPlease\n>  \tLIBC_CONTAINS_LIBINTL = YesPlease\n> +\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n>  endif\n\nI wonder why we set up this knob this way.  A lot of operating systems\nsupport fopen(..., \"r\") of a directory --- wouldn't it make sense for\nFREAD_READS_DIRECTORIES to be the default and for users on stricter\nplatforms to be able to set FREAD_DOES_NOT_READ_DIRECTORIES if they\nwant to speed Git up by taking advantage of their saner fread?\n\nThanks,\nJonathan\n"},{"id":"395916","messageId":"xmqqblnjjv63.fsf@gitster.c.googlers.com","threadId":"53285","inReplyTo":"20200422153347.40018-1-jrtc27@jrtc27.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T17:18:44Z","receivedAt":"2020-04-22T17:18:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jessica Clarke <jrtc27@jrtc27.com> writes:\n\n> GNU/Hurd is another platform that behaves like this. Set it to\n> UnfortunatelyYes so that config directory files are correctly processed.\n> This fixes the corresponding 'proper error on directory \"files\"' test in\n> t1308-config-set.sh.\n>\n> Thanks-to: Jeff King <peff@peff.net>\n> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>\n> ---\n>  config.mak.uname | 1 +\n>  1 file changed, 1 insertion(+)\n\nI'd tweak s/Thanks-to:/Helped-by:/ while queuing.\n\nThanks for a quick turnaround after reporting the issue and getting\na response.  The way collaboration is working feels wonderful.\n\n\n\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 0ab8e00938..3e526f6b9f 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -308,6 +308,7 @@ ifeq ($(uname_S),GNU)\n>  \tNO_STRLCPY = YesPlease\n>  \tHAVE_PATHS_H = YesPlease\n>  \tLIBC_CONTAINS_LIBINTL = YesPlease\n> +\tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n>  endif\n>  ifeq ($(uname_S),IRIX)\n>  \tNO_SETENV = YesPlease\n"},{"id":"395919","messageId":"xmqq368vjtsr.fsf@gitster.c.googlers.com","threadId":"53285","inReplyTo":"20200422164150.GA140314@google.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T17:48:20Z","receivedAt":"2020-04-22T17:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> I wonder why we set up this knob this way.  A lot of operating systems\n> support fopen(..., \"r\") of a directory --- wouldn't it make sense for\n> FREAD_READS_DIRECTORIES to be the default and for users on stricter\n> platforms to be able to set FREAD_DOES_NOT_READ_DIRECTORIES if they\n> want to speed Git up by taking advantage of their saner fread?\n\nIt would have been helped to hear that when we accepted cba22528\n(Add compat/fopen.c which returns NULL on attempt to open directory,\n2008-02-08).  Perhaps back then it was more common not to allow\nfopen() on a directory?  I dunno.\n\nBecause we do not very often hear \"oops, this system also needs the\nREADS_DIRECTORIES knob set\" these days, I consider it a fair game to\ntoggle the polarity of it, once the Hurd patch that started this\nthread lands, as the vicinity of the code would become quiescent\nagain.\n"},{"id":"395926","messageId":"CA+sFfMfre6W5GcPh1pWcroFD9S9OPj_uLp5CK11yh-UhqgDs2w@mail.gmail.com","threadId":"53285","inReplyTo":"20200422164150.GA140314@google.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-04-22T18:48:23Z","receivedAt":"2020-04-22T18:48:39Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Apr 22, 2020 at 9:41 AM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Jessica Clarke wrote:\n>\n> > GNU/Hurd is another platform that behaves like this. Set it to\n> > UnfortunatelyYes so that config directory files are correctly processed.\n> > This fixes the corresponding 'proper error on directory \"files\"' test in\n> > t1308-config-set.sh.\n> >\n> > Thanks-to: Jeff King <peff@peff.net>\n> > Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>\n> > ---\n> >  config.mak.uname | 1 +\n> >  1 file changed, 1 insertion(+)\n>\n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> Thanks.\n>\n> > diff --git a/config.mak.uname b/config.mak.uname\n> > index 0ab8e00938..3e526f6b9f 100644\n> > --- a/config.mak.uname\n> > +++ b/config.mak.uname\n> > @@ -308,6 +308,7 @@ ifeq ($(uname_S),GNU)\n> >       NO_STRLCPY = YesPlease\n> >       HAVE_PATHS_H = YesPlease\n> >       LIBC_CONTAINS_LIBINTL = YesPlease\n> > +     FREAD_READS_DIRECTORIES = UnfortunatelyYes\n> >  endif\n>\n> I wonder why we set up this knob this way.  A lot of operating systems\n> support fopen(..., \"r\") of a directory --- wouldn't it make sense for\n> FREAD_READS_DIRECTORIES to be the default and for users on stricter\n> platforms to be able to set FREAD_DOES_NOT_READ_DIRECTORIES if they\n> want to speed Git up by taking advantage of their saner fread?\n\nAt the time it was written, the assumption in the code was that an\nfread() on a directory would produce a failure, and that that was the\nsane and common behavior. fopen(..., \"r\") on a directory was expected\nto be successful on most platforms, but does fail on some. I don't\nrecall if it failed on any of the platforms I had access to at the\ntime (Solaris, IRIX), but it does fail on Windows. So I introduced\nthis feature that would make fopen() fail when opening a directory for\nuse on the platforms where fread() of a directory did not fail,\ninstead of trying to wrap fread().\n\nI just looked in config.mak.uname, and I'm surprised to see\nFREAD_READS_DIRECTORIES set for so many platforms. And it's set for\nLinux and Darwin?!?!? Junio added it for Darwin\n(8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy\nadded it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but\nalso seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as\nbeing about the fopen(..., \"r\") of a directory rather than about an\nfread() of a directory.\n\nI just wrote a test program and tested on Linux, Darwin, and Windows.\nLinux and Darwin both succeed to fopen() a directory and fail to\nfread() it, as expected. Windows fails to fopen() a directory.\n\nI notice this earlier commit mentions a failure of t1308\n(4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the\nreason FREAD_READS_DIRECTORIES was added to so many platforms?\n\n-Brandon\n"},{"id":"395927","messageId":"452EC7D1-BE96-4172-8F70-3E7384C87280@jrtc27.com","threadId":"53285","inReplyTo":"CA+sFfMfre6W5GcPh1pWcroFD9S9OPj_uLp5CK11yh-UhqgDs2w@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Jessica Clarke","fromEmail":"jrtc27@jrtc27.com","sentAt":"2020-04-22T18:50:44Z","receivedAt":"2020-04-22T18:50:49Z","isPatch":true,"sender":{"key":"jrtc27@jrtc27.com","avatar":"https://avatars.githubusercontent.com/u/816232?v=4"},"body":"On 22 Apr 2020, at 19:48, Brandon Casey <drafnel@gmail.com> wrote:\n> On Wed, Apr 22, 2020 at 9:41 AM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>> \n>> Jessica Clarke wrote:\n>> \n>>> GNU/Hurd is another platform that behaves like this. Set it to\n>>> UnfortunatelyYes so that config directory files are correctly processed.\n>>> This fixes the corresponding 'proper error on directory \"files\"' test in\n>>> t1308-config-set.sh.\n>>> \n>>> Thanks-to: Jeff King <peff@peff.net>\n>>> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>\n>>> ---\n>>> config.mak.uname | 1 +\n>>> 1 file changed, 1 insertion(+)\n>> \n>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n>> Thanks.\n>> \n>>> diff --git a/config.mak.uname b/config.mak.uname\n>>> index 0ab8e00938..3e526f6b9f 100644\n>>> --- a/config.mak.uname\n>>> +++ b/config.mak.uname\n>>> @@ -308,6 +308,7 @@ ifeq ($(uname_S),GNU)\n>>>      NO_STRLCPY = YesPlease\n>>>      HAVE_PATHS_H = YesPlease\n>>>      LIBC_CONTAINS_LIBINTL = YesPlease\n>>> +     FREAD_READS_DIRECTORIES = UnfortunatelyYes\n>>> endif\n>> \n>> I wonder why we set up this knob this way.  A lot of operating systems\n>> support fopen(..., \"r\") of a directory --- wouldn't it make sense for\n>> FREAD_READS_DIRECTORIES to be the default and for users on stricter\n>> platforms to be able to set FREAD_DOES_NOT_READ_DIRECTORIES if they\n>> want to speed Git up by taking advantage of their saner fread?\n> \n> At the time it was written, the assumption in the code was that an\n> fread() on a directory would produce a failure, and that that was the\n> sane and common behavior. fopen(..., \"r\") on a directory was expected\n> to be successful on most platforms, but does fail on some. I don't\n> recall if it failed on any of the platforms I had access to at the\n> time (Solaris, IRIX), but it does fail on Windows. So I introduced\n> this feature that would make fopen() fail when opening a directory for\n> use on the platforms where fread() of a directory did not fail,\n> instead of trying to wrap fread().\n> \n> I just looked in config.mak.uname, and I'm surprised to see\n> FREAD_READS_DIRECTORIES set for so many platforms. And it's set for\n> Linux and Darwin?!?!? Junio added it for Darwin\n> (8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy\n> added it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but\n> also seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as\n> being about the fopen(..., \"r\") of a directory rather than about an\n> fread() of a directory.\n> \n> I just wrote a test program and tested on Linux, Darwin, and Windows.\n> Linux and Darwin both succeed to fopen() a directory and fail to\n> fread() it, as expected. Windows fails to fopen() a directory.\n> \n> I notice this earlier commit mentions a failure of t1308\n> (4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the\n> reason FREAD_READS_DIRECTORIES was added to so many platforms?\n\nThen the current autoconf test is wrong and likely causing confusion:\n\n> AC_RUN_IFELSE(\n>         [AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],\n>                 [[\n>                 FILE *f = fopen(\".\", \"r\");\n>                 return f != NULL;]])],\n>         [ac_cv_fread_reads_directories=no],\n>         [ac_cv_fread_reads_directories=yes])\n> ])\n\nJess\n\n"},{"id":"395928","messageId":"CA+sFfMdPDuZze76OnwPDm7VjcvSu-nW03+kthHM4trs5KRJ6SQ@mail.gmail.com","threadId":"53285","inReplyTo":"CA+sFfMfre6W5GcPh1pWcroFD9S9OPj_uLp5CK11yh-UhqgDs2w@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-04-22T18:54:31Z","receivedAt":"2020-04-22T18:54:46Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Apr 22, 2020 at 11:48 AM Brandon Casey <drafnel@gmail.com> wrote:\n\n> I just wrote a test program and tested on Linux, Darwin, and Windows.\n> Linux and Darwin both succeed to fopen() a directory and fail to\n> fread() it, as expected. Windows fails to fopen() a directory.\n\nI just want to be clear here, so based on the above, Linux and Darwin\nshould _not_ need to have FREAD_READS_DIRECTORIES set.\n\nWindows, as always, is a stickier subject. Based on the above, Windows\nshould not need FREAD_READS_DIRECTORIES either. It's not set when MSVC\nis the compiler, but it is set for cygwin.\n\n-Brandon\n"},{"id":"395929","messageId":"CA+sFfMdudQ-cWbXajQ_92LZLOOOnTxmQmmsUo3dY1eKCL5-Y6Q@mail.gmail.com","threadId":"53285","inReplyTo":"452EC7D1-BE96-4172-8F70-3E7384C87280@jrtc27.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-04-22T19:05:57Z","receivedAt":"2020-04-22T19:06:13Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Apr 22, 2020 at 11:50 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:\n>\n> On 22 Apr 2020, at 19:48, Brandon Casey <drafnel@gmail.com> wrote:\n\n> > introduced\n> > this feature that would make fopen() fail when opening a directory for\n> > use on the platforms where fread() of a directory did not fail,\n> > instead of trying to wrap fread().\n>\n> Then the current autoconf test is wrong and likely causing confusion:\n>\n> > AC_RUN_IFELSE(\n> >         [AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],\n> >                 [[\n> >                 FILE *f = fopen(\".\", \"r\");\n> >                 return f != NULL;]])],\n> >         [ac_cv_fread_reads_directories=no],\n> >         [ac_cv_fread_reads_directories=yes])\n> > ])\n\nYes, we should attempt to call fread() there. If either the fopen()\nfails or the fread() fails, then that should mean that\nFREAD_READS_DIRECTORIES is not necessary.\n\n-Brandon\n"},{"id":"395930","messageId":"CA+sFfMf=NZOV41-4oJTLF34rvELr7EvfEHOr90X_h8pSFC3AJQ@mail.gmail.com","threadId":"53285","inReplyTo":"CA+sFfMfre6W5GcPh1pWcroFD9S9OPj_uLp5CK11yh-UhqgDs2w@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-04-22T19:13:50Z","receivedAt":"2020-04-22T19:14:05Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Apr 22, 2020 at 11:48 AM Brandon Casey <drafnel@gmail.com> wrote:\n>\n> I just looked in config.mak.uname, and I'm surprised to see\n> FREAD_READS_DIRECTORIES set for so many platforms. And it's set for\n> Linux and Darwin?!?!? Junio added it for Darwin\n> (8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy\n> added it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but\n> also seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as\n> being about the fopen(..., \"r\") of a directory rather than about an\n> fread() of a directory.\n>\n> I just wrote a test program and tested on Linux, Darwin, and Windows.\n> Linux and Darwin both succeed to fopen() a directory and fail to\n> fread() it, as expected. Windows fails to fopen() a directory.\n>\n> I notice this earlier commit mentions a failure of t1308\n> (4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the\n> reason FREAD_READS_DIRECTORIES was added to so many platforms?\n\nWhoops, I got the order of e2d90fd1c33ae57e4a6da5729ae53876107b3463\nand 4e3ecbd43958b1400d6cb85fe5529beda1630e3a wrong. Looks like the\nmisunderstanding of FREAD_READS_DIRECTORIES in e2d90fd could have been\nthe cause of all of this. That commit introduced the test t1308 and\nadded FREAD_READS... to Linux, kFreeBSD, and FreeBSD, and the other\nadditions followed shortly after.\n\n-Brandon\n"},{"id":"395938","messageId":"20200422195839.GE558336@coredump.intra.peff.net","threadId":"53285","inReplyTo":"CA+sFfMf=NZOV41-4oJTLF34rvELr7EvfEHOr90X_h8pSFC3AJQ@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T19:58:39Z","receivedAt":"2020-04-22T19:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 12:13:50PM -0700, Brandon Casey wrote:\n\n> On Wed, Apr 22, 2020 at 11:48 AM Brandon Casey <drafnel@gmail.com> wrote:\n> >\n> > I just looked in config.mak.uname, and I'm surprised to see\n> > FREAD_READS_DIRECTORIES set for so many platforms. And it's set for\n> > Linux and Darwin?!?!? Junio added it for Darwin\n> > (8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy\n> > added it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but\n> > also seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as\n> > being about the fopen(..., \"r\") of a directory rather than about an\n> > fread() of a directory.\n> >\n> > I just wrote a test program and tested on Linux, Darwin, and Windows.\n> > Linux and Darwin both succeed to fopen() a directory and fail to\n> > fread() it, as expected. Windows fails to fopen() a directory.\n> >\n> > I notice this earlier commit mentions a failure of t1308\n> > (4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the\n> > reason FREAD_READS_DIRECTORIES was added to so many platforms?\n> \n> Whoops, I got the order of e2d90fd1c33ae57e4a6da5729ae53876107b3463\n> and 4e3ecbd43958b1400d6cb85fe5529beda1630e3a wrong. Looks like the\n> misunderstanding of FREAD_READS_DIRECTORIES in e2d90fd could have been\n> the cause of all of this. That commit introduced the test t1308 and\n> added FREAD_READS... to Linux, kFreeBSD, and FreeBSD, and the other\n> additions followed shortly after.\n\nI think the code is actually doing the right thing (or at least\nsomething useful), and the \"FREAD\" in the name is the confusing part.\nBecause there's now code doing:\n\n  fh = fopen(fn, \"r\");\n  if (!fh) {\n    if (errno == ENOENT || errno == EISDIR) {\n       /* not actually a file; treat as a gentle noop */\n       return 0;\n    } else {\n       die_errno(\"omg, a real open error\");\n    }\n  }\n  if (!fread(..., fh))\n       die_errno(\"omg, a real read error\");\n\nwhich is exactly what the failing test in t1308 is doing.\n\nI know that wasn't the original intent of the flag, but I think it was a\nconscious decision to build on around the time of e2d90fd1c, when we\nstarted actually checking fopen() return values (as opposed to just\nsegfaulting).\n\nAnd in practice, do we really care about cases that can fopen a\ndirectory but refuse to read from it? It's simpler and more efficient to\ncatch this case up front.\n\nSo I think the knob has de facto become \"do we need to use our compat\nwrapper to make opening a directory fail with EISDIR\". And any attempt\nto change that will mean adapting all of the callers to handle that case\nthemselves. I think what we have now is the useful knob we want; it's\njust misnamed (and obviously I don't blame your original patch; it was\nadapted over time).\n\n-Peff\n"},{"id":"395948","messageId":"CA+sFfMcsiXcEV=oq-cdm1zVm2gvn=Ae1BU264puFU=E2JBQVCw@mail.gmail.com","threadId":"53285","inReplyTo":"20200422195839.GE558336@coredump.intra.peff.net","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2020-04-22T21:18:15Z","receivedAt":"2020-04-22T21:18:30Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Apr 22, 2020 at 12:58 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Apr 22, 2020 at 12:13:50PM -0700, Brandon Casey wrote:\n>\n> > On Wed, Apr 22, 2020 at 11:48 AM Brandon Casey <drafnel@gmail.com> wrote:\n> > >\n> > > I just looked in config.mak.uname, and I'm surprised to see\n> > > FREAD_READS_DIRECTORIES set for so many platforms. And it's set for\n> > > Linux and Darwin?!?!? Junio added it for Darwin\n> > > (8e178ec4d072da4cd8f4449e17aef3aff5b57f6a) and Nguyễn Thái Ngọc Duy\n> > > added it for Linux (e2d90fd1c33ae57e4a6da5729ae53876107b3463), but\n> > > also seemed to mistake the intention of FREAD_FREADS_DIRECTORIES as\n> > > being about the fopen(..., \"r\") of a directory rather than about an\n> > > fread() of a directory.\n> > >\n> > > I just wrote a test program and tested on Linux, Darwin, and Windows.\n> > > Linux and Darwin both succeed to fopen() a directory and fail to\n> > > fread() it, as expected. Windows fails to fopen() a directory.\n> > >\n> > > I notice this earlier commit mentions a failure of t1308\n> > > (4e3ecbd43958b1400d6cb85fe5529beda1630e3a). I wonder if this is the\n> > > reason FREAD_READS_DIRECTORIES was added to so many platforms?\n> >\n> > Whoops, I got the order of e2d90fd1c33ae57e4a6da5729ae53876107b3463\n> > and 4e3ecbd43958b1400d6cb85fe5529beda1630e3a wrong. Looks like the\n> > misunderstanding of FREAD_READS_DIRECTORIES in e2d90fd could have been\n> > the cause of all of this. That commit introduced the test t1308 and\n> > added FREAD_READS... to Linux, kFreeBSD, and FreeBSD, and the other\n> > additions followed shortly after.\n>\n> I think the code is actually doing the right thing (or at least\n> something useful), and the \"FREAD\" in the name is the confusing part.\n> Because there's now code doing:\n>\n>   fh = fopen(fn, \"r\");\n>   if (!fh) {\n>     if (errno == ENOENT || errno == EISDIR) {\n>        /* not actually a file; treat as a gentle noop */\n>        return 0;\n>     } else {\n>        die_errno(\"omg, a real open error\");\n>     }\n>   }\n>   if (!fread(..., fh))\n>        die_errno(\"omg, a real read error\");\n>\n> which is exactly what the failing test in t1308 is doing.\n>\n> I know that wasn't the original intent of the flag, but I think it was a\n> conscious decision to build on around the time of e2d90fd1c, when we\n> started actually checking fopen() return values (as opposed to just\n> segfaulting).\n\nOur policy has always been to check return values hasn't it?\n\n> And in practice, do we really care about cases that can fopen a\n> directory but refuse to read from it? It's simpler and more efficient to\n> catch this case up front.\n\nI'm not sure I agree that it's simpler and more efficient to catch\nthis up front. Catching the case where a directory is supplied when\nonly a file is valid is an error path which we generally do not\noptimize for, and it requires us to add an extra stat to every fopen()\ncall. We should have always been checking both the fopen() and any\nreads and handle a failure in either one. So normally we wouldn't have\nto do anything special to produce an error for the case of a directory\nbeing supplied.\n\nNow, if you want to ignore when a directory is supplied, and not\nproduce an error, I would expect the code to actually check for that\nexplicitly/clearly and not depend on fopen() failing, since that is\nnot a common behavior.\n\n> So I think the knob has de facto become \"do we need to use our compat\n> wrapper to make opening a directory fail with EISDIR\". And any attempt\n> to change that will mean adapting all of the callers to handle that case\n> themselves. I think what we have now is the useful knob we want; it's\n> just misnamed (and obviously I don't blame your original patch; it was\n> adapted over time).\n\nWe've generally taken the approach that there is an expected \"normal\"\nbehavior for the c library, generally the linux/glibc behavior. Then,\nfor platforms that behaved differently from what we've defined as\n\"normal\", we'd introduce a compatibility function to make them behave\nthe way we wanted them to behave, or as close to that as possible. But\nwhat you're suggesting here seems different. You're suggesting that we\nshould modify the behavior of fopen from what is commonly considered\n\"normal\" so that it behaves in a new uncommon way. That doesn't seem\nlike the right thing to do.\n\nInstead, I would think it would be better to introduce a new function\nthat has the behavior we want and to explicitly call that function\ninstead of pretending that we're calling fopen(). Otherwise it just\nleads to confusion since _our_ fopen() doesn't actually work the way\nfopen() \"normally\" works on our common platform.  Maybe call it\nfopen_file_only() or something. I've been away from git development\nfor too long to know whether most fopen callsites would need to use an\nfopen_file_only() function or whether it would just be a few special\ninstances, and the rest could just use a regular fopen().\n\n-Brandon\n"},{"id":"396111","messageId":"20200424055106.GG1648190@coredump.intra.peff.net","threadId":"53285","inReplyTo":"CA+sFfMcsiXcEV=oq-cdm1zVm2gvn=Ae1BU264puFU=E2JBQVCw@mail.gmail.com","subject":"Re: [PATCH] config.mak.uname: Define FREAD_READS_DIRECTORIES for GNU/Hurd","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-24T05:51:06Z","receivedAt":"2020-04-24T05:51:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 02:18:15PM -0700, Brandon Casey wrote:\n\n> > I know that wasn't the original intent of the flag, but I think it was a\n> > conscious decision to build on around the time of e2d90fd1c, when we\n> > started actually checking fopen() return values (as opposed to just\n> > segfaulting).\n> \n> Our policy has always been to check return values hasn't it?\n\nPolicy perhaps, but practice didn't always follow it. :) At any rate, my\npoint was just that as part of a larger cleanup, we found it useful at\nthat time to have an fopen() which behaved predictably on all platforms.\n\n> > And in practice, do we really care about cases that can fopen a\n> > directory but refuse to read from it? It's simpler and more efficient to\n> > catch this case up front.\n> \n> I'm not sure I agree that it's simpler and more efficient to catch\n> this up front. Catching the case where a directory is supplied when\n> only a file is valid is an error path which we generally do not\n> optimize for, and it requires us to add an extra stat to every fopen()\n> call. We should have always been checking both the fopen() and any\n> reads and handle a failure in either one. So normally we wouldn't have\n> to do anything special to produce an error for the case of a directory\n> being supplied.\n\nTrue, I guess it's not really more efficient (I was thinking that we had\nto deal with it only once, not on each fread). I do think it's simpler,\nthough, as it means fopen() behaves the same everywhere.\n\n> Now, if you want to ignore when a directory is supplied, and not\n> produce an error, I would expect the code to actually check for that\n> explicitly/clearly and not depend on fopen() failing, since that is\n> not a common behavior.\n\nSure, we could be stat()-ing each file before fopen(). And if you think\nit's worth going back to that direction, you can try to work up a patch.\nMy suspicion is that it will end up making the callers worse, for no\nreal gain.\n\n> We've generally taken the approach that there is an expected \"normal\"\n> behavior for the c library, generally the linux/glibc behavior. Then,\n> for platforms that behaved differently from what we've defined as\n> \"normal\", we'd introduce a compatibility function to make them behave\n> the way we wanted them to behave, or as close to that as possible. But\n> what you're suggesting here seems different. You're suggesting that we\n> should modify the behavior of fopen from what is commonly considered\n> \"normal\" so that it behaves in a new uncommon way. That doesn't seem\n> like the right thing to do.\n\nI think this is just defining a different \"normal\". And FWIW, I'm not\nsuggesting any particular change now. I think we went in this direction\na few years ago, and I'm just trying to explain the current state in\nterms of that decision.\n\n> Instead, I would think it would be better to introduce a new function\n> that has the behavior we want and to explicitly call that function\n> instead of pretending that we're calling fopen(). Otherwise it just\n> leads to confusion since _our_ fopen() doesn't actually work the way\n> fopen() \"normally\" works on our common platform.  Maybe call it\n> fopen_file_only() or something. I've been away from git development\n> for too long to know whether most fopen callsites would need to use an\n> fopen_file_only() function or whether it would just be a few special\n> instances, and the rest could just use a regular fopen().\n\nYeah, I think that would be fine. Though again, I'm really not sure that\nit gains us all that much, and it would require auditing each fopen()\nsite to see whether it's depending on the current compat behavior or\nnot.\n\n-Peff\n"}]}