{"thread":{"id":"50379","subject":"[PATCH] http: cast result to FILE *","startedAt":"2019-02-01T19:40:36Z","lastAt":"2019-05-08T10:16:21Z","messageCount":16,"participants":["Dan McGregor","Junio C Hamano","Duy Nguyen","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"368365","messageId":"20190201193004.88736-1-dan.mcgregor@usask.ca","threadId":"50379","inReplyTo":null,"subject":"[PATCH] http: cast result to FILE *","fromName":"Dan McGregor","fromEmail":"dan.mcgregor@usask.ca","sentAt":"2019-02-01T19:30:04Z","receivedAt":"2019-02-01T19:40:36Z","isPatch":true,"sender":{"key":"dan.mcgregor@usask.ca","avatar":null},"body":"Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n2019-01-10) introduced an implicit assumption that rewind, fileno, and\nfflush are functions. At least on FreeBSD fileno is not, and as such\npassing a void * failed.\n\nExplicitly cast result to a FILE * when using standard functions that\nmay ultimately be macros.\n\nSigned-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n---\n http.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 954bebf684..8b9476b151 100644\n--- a/http.c\n+++ b/http.c\n@@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,\n \t\tstrbuf_reset(result);\n \t\tbreak;\n \tcase HTTP_REQUEST_FILE:\n-\t\tif (fflush(result)) {\n+\t\tif (fflush((FILE *)result)) {\n \t\t\terror_errno(\"unable to flush a file\");\n \t\t\treturn HTTP_START_FAILED;\n \t\t}\n-\t\trewind(result);\n-\t\tif (ftruncate(fileno(result), 0) < 0) {\n+\t\trewind((FILE *)result);\n+\t\tif (ftruncate(fileno((FILE *)result), 0) < 0) {\n \t\t\terror_errno(\"unable to truncate a file\");\n \t\t\treturn HTTP_START_FAILED;\n \t\t}\n-- \n2.20.1\n\n"},{"id":"368374","messageId":"xmqqr2crxl7o.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"20190201193004.88736-1-dan.mcgregor@usask.ca","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-01T21:17:15Z","receivedAt":"2019-02-01T21:17:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan McGregor <dan.mcgregor@usask.ca> writes:\n\n> Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> fflush are functions. At least on FreeBSD fileno is not, and as such\n> passing a void * failed.\n\nI am not strongly opposed to this patch, but shouldn't you be filing\na bug report against FreeBSD instead?  The implementation is free to\ndefine fileno(fh) as a macro, but it shouldn't force such a change\nto conformant programs.\n\nhttps://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146\n\n> Explicitly cast result to a FILE * when using standard functions that\n> may ultimately be macros.\n>\n> Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n> ---\n>  http.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 954bebf684..8b9476b151 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,\n>  \t\tstrbuf_reset(result);\n>  \t\tbreak;\n>  \tcase HTTP_REQUEST_FILE:\n> -\t\tif (fflush(result)) {\n> +\t\tif (fflush((FILE *)result)) {\n>  \t\t\terror_errno(\"unable to flush a file\");\n>  \t\t\treturn HTTP_START_FAILED;\n>  \t\t}\n> -\t\trewind(result);\n> -\t\tif (ftruncate(fileno(result), 0) < 0) {\n> +\t\trewind((FILE *)result);\n> +\t\tif (ftruncate(fileno((FILE *)result), 0) < 0) {\n>  \t\t\terror_errno(\"unable to truncate a file\");\n>  \t\t\treturn HTTP_START_FAILED;\n>  \t\t}\n"},{"id":"368405","messageId":"CACsJy8B_=qbeimp5=RS-r2gwEjVV9rDE_2_tk_DDqz6rJazvFw@mail.gmail.com","threadId":"50379","inReplyTo":"xmqqr2crxl7o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-02T11:21:10Z","receivedAt":"2019-02-02T11:21:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Dan McGregor <dan.mcgregor@usask.ca> writes:\n>\n> > Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> > 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> > fflush are functions. At least on FreeBSD fileno is not, and as such\n> > passing a void * failed.\n>\n> I am not strongly opposed to this patch,\n\nEven if this is needed, should it be done behind git-compat-util.h\ninstead? That way if fileno(void*) is used elsewhere, we don't have to\ndo the casting again.\n\n> but shouldn't you be filing\n> a bug report against FreeBSD instead?  The implementation is free to\n> define fileno(fh) as a macro, but it shouldn't force such a change\n> to conformant programs.\n>\n> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146\n>\n> > Explicitly cast result to a FILE * when using standard functions that\n> > may ultimately be macros.\n> >\n> > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n> > ---\n> >  http.c | 6 +++---\n> >  1 file changed, 3 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/http.c b/http.c\n> > index 954bebf684..8b9476b151 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,\n> >               strbuf_reset(result);\n> >               break;\n> >       case HTTP_REQUEST_FILE:\n> > -             if (fflush(result)) {\n> > +             if (fflush((FILE *)result)) {\n> >                       error_errno(\"unable to flush a file\");\n> >                       return HTTP_START_FAILED;\n> >               }\n> > -             rewind(result);\n> > -             if (ftruncate(fileno(result), 0) < 0) {\n> > +             rewind((FILE *)result);\n> > +             if (ftruncate(fileno((FILE *)result), 0) < 0) {\n> >                       error_errno(\"unable to truncate a file\");\n> >                       return HTTP_START_FAILED;\n> >               }\n\n\n\n-- \nDuy\n"},{"id":"368453","messageId":"nycvar.QRO.7.76.6.1902041243220.41@tvgsbejvaqbjf.bet","threadId":"50379","inReplyTo":"CACsJy8B_=qbeimp5=RS-r2gwEjVV9rDE_2_tk_DDqz6rJazvFw@mail.gmail.com","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-02-04T11:44:52Z","receivedAt":"2019-02-04T11:45:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Sat, 2 Feb 2019, Duy Nguyen wrote:\n\n> On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Dan McGregor <dan.mcgregor@usask.ca> writes:\n> >\n> > > Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> > > fflush are functions. At least on FreeBSD fileno is not, and as such\n> > > passing a void * failed.\n> >\n> > I am not strongly opposed to this patch,\n> \n> Even if this is needed, should it be done behind git-compat-util.h\n> instead? That way if fileno(void*) is used elsewhere, we don't have to\n> do the casting again.\n\nThe disadvantage, of course, would be that other call sites would not\nbenefit from a manual auditing whether the argument has side effects (and\nthus, whether a macro using the argument multiple times would result in\nvery unexpected multiple side effects).\n\nI'd rather not paper over this issue and thereby open even larger\nproblems.\n\nCiao,\nDscho\n\n> \n> > but shouldn't you be filing\n> > a bug report against FreeBSD instead?  The implementation is free to\n> > define fileno(fh) as a macro, but it shouldn't force such a change\n> > to conformant programs.\n> >\n> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146\n> >\n> > > Explicitly cast result to a FILE * when using standard functions that\n> > > may ultimately be macros.\n> > >\n> > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n> > > ---\n> > >  http.c | 6 +++---\n> > >  1 file changed, 3 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/http.c b/http.c\n> > > index 954bebf684..8b9476b151 100644\n> > > --- a/http.c\n> > > +++ b/http.c\n> > > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,\n> > >               strbuf_reset(result);\n> > >               break;\n> > >       case HTTP_REQUEST_FILE:\n> > > -             if (fflush(result)) {\n> > > +             if (fflush((FILE *)result)) {\n> > >                       error_errno(\"unable to flush a file\");\n> > >                       return HTTP_START_FAILED;\n> > >               }\n> > > -             rewind(result);\n> > > -             if (ftruncate(fileno(result), 0) < 0) {\n> > > +             rewind((FILE *)result);\n> > > +             if (ftruncate(fileno((FILE *)result), 0) < 0) {\n> > >                       error_errno(\"unable to truncate a file\");\n> > >                       return HTTP_START_FAILED;\n> > >               }\n> \n> \n> \n> -- \n> Duy\n> \n"},{"id":"368454","messageId":"CACsJy8BtaxMRTG4-r3iJfUuR9k-=r=4QTRxCkFt3k3p7826Z9A@mail.gmail.com","threadId":"50379","inReplyTo":"nycvar.QRO.7.76.6.1902041243220.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-04T12:13:06Z","receivedAt":"2019-02-04T12:13:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 4, 2019 at 6:44 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Duy,\n>\n> On Sat, 2 Feb 2019, Duy Nguyen wrote:\n>\n> > On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > >\n> > > Dan McGregor <dan.mcgregor@usask.ca> writes:\n> > >\n> > > > Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> > > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> > > > fflush are functions. At least on FreeBSD fileno is not, and as such\n> > > > passing a void * failed.\n> > >\n> > > I am not strongly opposed to this patch,\n> >\n> > Even if this is needed, should it be done behind git-compat-util.h\n> > instead? That way if fileno(void*) is used elsewhere, we don't have to\n> > do the casting again.\n>\n> The disadvantage, of course, would be that other call sites would not\n> benefit from a manual auditing whether the argument has side effects (and\n> thus, whether a macro using the argument multiple times would result in\n> very unexpected multiple side effects).\n\nThat's just a better reason to \"fix\" it in compat/. If you define a\ngit_fileno() function and map fileno to it, then you won't have to\ndeal with side effects of FreeBSD's fileno() macro. All evaluation\nhappens before git_fileno() is called.\n\n> I'd rather not paper over this issue and thereby open even larger\n> problems.\n>\n> Ciao,\n> Dscho\n>\n> >\n> > > but shouldn't you be filing\n> > > a bug report against FreeBSD instead?  The implementation is free to\n> > > define fileno(fh) as a macro, but it shouldn't force such a change\n> > > to conformant programs.\n> > >\n> > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146\n> > >\n> > > > Explicitly cast result to a FILE * when using standard functions that\n> > > > may ultimately be macros.\n> > > >\n> > > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n> > > > ---\n> > > >  http.c | 6 +++---\n> > > >  1 file changed, 3 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/http.c b/http.c\n> > > > index 954bebf684..8b9476b151 100644\n> > > > --- a/http.c\n> > > > +++ b/http.c\n> > > > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url,\n> > > >               strbuf_reset(result);\n> > > >               break;\n> > > >       case HTTP_REQUEST_FILE:\n> > > > -             if (fflush(result)) {\n> > > > +             if (fflush((FILE *)result)) {\n> > > >                       error_errno(\"unable to flush a file\");\n> > > >                       return HTTP_START_FAILED;\n> > > >               }\n> > > > -             rewind(result);\n> > > > -             if (ftruncate(fileno(result), 0) < 0) {\n> > > > +             rewind((FILE *)result);\n> > > > +             if (ftruncate(fileno((FILE *)result), 0) < 0) {\n> > > >                       error_errno(\"unable to truncate a file\");\n> > > >                       return HTTP_START_FAILED;\n> > > >               }\n> >\n> >\n> >\n> > --\n> > Duy\n> >\n\n\n\n-- \nDuy\n"},{"id":"368464","messageId":"xmqq1s4nwhjl.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"CACsJy8BtaxMRTG4-r3iJfUuR9k-=r=4QTRxCkFt3k3p7826Z9A@mail.gmail.com","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-04T18:10:54Z","receivedAt":"2019-02-04T18:10:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> The disadvantage, of course, would be that other call sites would not\n>> benefit from a manual auditing whether the argument has side effects (and\n>> thus, whether a macro using the argument multiple times would result in\n>> very unexpected multiple side effects).\n>\n> That's just a better reason to \"fix\" it in compat/. If you define a\n> git_fileno() function and map fileno to it, then you won't have to\n> deal with side effects of FreeBSD's fileno() macro. All evaluation\n> happens before git_fileno() is called.\n\nHmph, so the idea is to have\n\n\t/* do not include git-compat-util.h here */\n\tint wrapped_fileno(FILE *f)\n\t{\n\t\treturn fileno(f);\n\t}\n\nin compat/fileno.c and then do something like this\n\n\t#ifdef fileno\n\t#undef fileno\n\t#define fileno(x) wrapped_fileno(x)\n\t#endif\n\nfor FreeBSD in git-compat-util.h or something like that?\n\nI think I can buy that.\n"},{"id":"368500","messageId":"49B9198C-53E5-42BD-8834-B1EDEB3332CB@usask.ca","threadId":"50379","inReplyTo":"xmqq1s4nwhjl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http: cast result to FILE *","fromName":"Dan McGregor","fromEmail":"dkm560@usask.ca","sentAt":"2019-02-05T01:36:34Z","receivedAt":"2019-02-05T01:36:38Z","isPatch":true,"sender":{"key":"dkm560@usask.ca","avatar":null},"body":"\n\nOn February 4, 2019 12:10:54 p.m. CST, Junio C Hamano <gitster@pobox.com> wrote:\n>Duy Nguyen <pclouds@gmail.com> writes:\n>\n>>> The disadvantage, of course, would be that other call sites would\n>not\n>>> benefit from a manual auditing whether the argument has side effects\n>(and\n>>> thus, whether a macro using the argument multiple times would result\n>in\n>>> very unexpected multiple side effects).\n>>\n>> That's just a better reason to \"fix\" it in compat/. If you define a\n>> git_fileno() function and map fileno to it, then you won't have to\n>> deal with side effects of FreeBSD's fileno() macro. All evaluation\n>> happens before git_fileno() is called.\n>\n>Hmph, so the idea is to have\n>\n>\t/* do not include git-compat-util.h here */\n>\tint wrapped_fileno(FILE *f)\n>\t{\n>\t\treturn fileno(f);\n>\t}\n>\n>in compat/fileno.c and then do something like this\n>\n>\t#ifdef fileno\n>\t#undef fileno\n>\t#define fileno(x) wrapped_fileno(x)\n>\t#endif\n>\n>for FreeBSD in git-compat-util.h or something like that?\n>\n>I think I can buy that.\n\nWorks for me too. FreeBSD's not alone in this, either. It looks like NetBSD does the same thing.\nI'll send a v2 tomorrow.\n"},{"id":"368904","messageId":"20190209023621.75255-1-dan.mcgregor@usask.ca","threadId":"50379","inReplyTo":"20190201193004.88736-1-dan.mcgregor@usask.ca","subject":"[PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Dan McGregor","fromEmail":"dan.mcgregor@usask.ca","sentAt":"2019-02-09T02:36:21Z","receivedAt":"2019-02-09T02:36:48Z","isPatch":true,"sender":{"key":"dan.mcgregor@usask.ca","avatar":null},"body":"Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n2019-01-10) introduced an implicit assumption that rewind, fileno, and\nfflush are functions. At least on FreeBSD fileno is not, and as such\npassing a void * failed.\n\nAll systems tested (FreeBSD and NetBSD) that define fineo as a macro\nalso have a function defined. Undefine the macro on these systems so\nthat the function is used.\n\nSigned-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n---\n git-compat-util.h | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 29a19902aa..b5489bbcf2 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -764,6 +764,15 @@ char *gitstrdup(const char *s);\n extern FILE *git_fopen(const char*, const char*);\n #endif\n \n+/* Some systems (the various BSDs for eaxmple) define\n+ * fileno as a macro as an optimization. All systems I\n+ * know about also define it as a real funcion, so use\n+ * the real function to allow passing void *s to fileno.\n+ */\n+#ifdef fileno\n+# undef fileno\n+#endif\n+\n #ifdef SNPRINTF_RETURNS_BOGUS\n #ifdef snprintf\n #undef snprintf\n-- \n2.20.1\n\n"},{"id":"368933","messageId":"xmqqsgwwizf6.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"20190209023621.75255-1-dan.mcgregor@usask.ca","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-09T18:34:21Z","receivedAt":"2019-02-09T18:34:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan McGregor <dan.mcgregor@usask.ca> writes:\n\n> Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> fflush are functions. At least on FreeBSD fileno is not, and as such\n> passing a void * failed.\n>\n> All systems tested (FreeBSD and NetBSD) that define fineo as a macro\n\ns/fineo/fileno/\n\n> also have a function defined. Undefine the macro on these systems so\n> that the function is used.\n\nThat smells like the patch is assuming a bit too much.  A platform\nyou did not inspect may have it as a macro but the implementation\nmay still be usable without breakage like the one we saw on FreeBSD,\nfor example.\n\nIt also robs us the possibility of overriding fileno() with our own\nmacro earlier in this file, like we do for many functions you see in\nthe output from this:\n\n $ git grep '#define .* git' git-compat-util.h\n\nIn general, I'd like to see people thinking twice about an overly\nsimple approach when they justify it with \"this should work\neverywhere I know of\".\n\n> Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>\n> ---\n>  git-compat-util.h | 9 +++++++++\n>  1 file changed, 9 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 29a19902aa..b5489bbcf2 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -764,6 +764,15 @@ char *gitstrdup(const char *s);\n>  extern FILE *git_fopen(const char*, const char*);\n>  #endif\n>  \n> +/* Some systems (the various BSDs for eaxmple) define\n\nStyle:\n\n/*\n * Our multi-line comment starts with a lone slash-star\n * and ends with a lone star-slash, like this.\n */\n\n> + * fileno as a macro as an optimization. All systems I\n> + * know about also define it as a real funcion, so use\n> + * the real function to allow passing void *s to fileno.\n> + */\n> +#ifdef fileno\n> +# undef fileno\n> +#endif\n> +\n>  #ifdef SNPRINTF_RETURNS_BOGUS\n>  #ifdef snprintf\n>  #undef snprintf\n"},{"id":"369160","messageId":"20190212134537.GA26137@ash","threadId":"50379","inReplyTo":"20190209023621.75255-1-dan.mcgregor@usask.ca","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-12T13:45:37Z","receivedAt":"2019-02-12T13:45:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:\n> Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> fflush are functions. At least on FreeBSD fileno is not, and as such\n> passing a void * failed.\n> \n> All systems tested (FreeBSD and NetBSD) that define fineo as a macro\n\nOpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while\nNetBSD compiles ok (and fails to run some tests)\n\n[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures\n\n> also have a function defined. Undefine the macro on these systems so\n> that the function is used.\n\nI don't think this is enough. At least fbsd defines this\n\n#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))\n\nso one of the two functions will be used depending on __isthreaded\nflag. Your forcing to use fileno, ignoring __sfileno, is technically\nnot correct.\n\nFor the record, at least fbsd also defines feof, ferror, clearerr,\ngetc and putc in the same way. But at least I don't see how something\nlike feof(fp++) could cause bad side effects.\n\nSo, how about something like this? A teeny bit longer than your\nversion, but I think it's easier to control long term.\n\n-- 8< --\ndiff --git a/Makefile b/Makefile\nindex 0e13a5b469..44cfc63fc4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -433,6 +433,8 @@ all::\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\n #\n+# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real function.\n+#\n # Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n # default environment variables to be passed when a pager is spawned, e.g.\n #\n@@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR\n \tBASIC_CFLAGS += -DHAVE_WPGMPTR\n endif\n \n+ifdef FILENO_IS_A_MACRO\n+\tCOMPAT_CFLAGS += -DFILENO_IS_A_MACRO\n+\tCOMPAT_OBJS += compat/fileno.o\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\ndiff --git a/compat/fileno.c b/compat/fileno.c\nnew file mode 100644\nindex 0000000000..7b105f4cd7\n--- /dev/null\n+++ b/compat/fileno.c\n@@ -0,0 +1,7 @@\n+#define COMPAT_CODE\n+#include \"../git-compat-util.h\"\n+\n+int git_fileno(FILE *stream)\n+{\n+\treturn fileno(stream);\n+}\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 786bb2f913..01f62674a4 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD)\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tFILENO_IS_A_MACRO = UnfortunatelyYes\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD)\n \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tFILENO_IS_A_MACRO = UnfortunatelyYes\n endif\n ifeq ($(uname_S),MirBSD)\n \tNO_STRCASESTR = YesPlease\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 29a19902aa..6573808ebd 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n+#ifdef FILENO_IS_A_MACRO\n+int git_fileno(FILE *stream);\n+# ifndef COMPAT_CODE\n+#  undef fileno\n+#  define fileno(p) git_fileno(p)\n+# endif\n+#endif\n+\n /*\n  * Our code often opens a path to an optional file, to work on its\n  * contents when we can successfully open it.  We can ignore a failure\n-- 8< --\n\n--\nDuy\n"},{"id":"369162","messageId":"CACsJy8Dp+PV-Y=WGbr4bfNdK6zi+qBE5b6Etc+8ZvgDBYWBLjA@mail.gmail.com","threadId":"50379","inReplyTo":"20190212134537.GA26137@ash","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-12T14:14:41Z","receivedAt":"2019-02-12T14:15:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 12, 2019 at 8:45 PM Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:\n> > Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n> > 2019-01-10) introduced an implicit assumption that rewind, fileno, and\n> > fflush are functions. At least on FreeBSD fileno is not, and as such\n> > passing a void * failed.\n> >\n> > All systems tested (FreeBSD and NetBSD) that define fineo as a macro\n>\n> OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while\n> NetBSD compiles ok (and fails to run some tests)\n\nAccording to this [2] looks like NetBSD is affected too, but only in\nno-thread mode. With pthreads, fileno is not redefined as a macro.\nThat probably explains why it's ok from git-ci.\n\n#define fileno(p) __sfileno(p)\n\n#define __sfileno(p) \\\n    ((p)->_file == -1 ? -1 : (int)(unsigned short)(p)->_file)\n\n[2] http://cvsweb.netbsd.org/bsdweb.cgi/src/include/stdio.h?rev=1.97&content-type=text/x-cvsweb-markup\n\n> For the record, at least fbsd also defines feof, ferror, clearerr,\n> getc and putc in the same way. But at least I don't see how something\n> like feof(fp++) could cause bad side effects.\n\n... and __sfileno (and other friends) can cause bad side effects,\nsigh. It's probably best to avoid fancy function calls.\n-- \nDuy\n"},{"id":"369166","messageId":"xmqqwom5djyk.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"20190212134537.GA26137@ash","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-12T16:56:19Z","receivedAt":"2019-02-12T16:56:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> So, how about something like this? A teeny bit longer than your\n> version, but I think it's easier to control long term.\n>\n> -- 8< --\n> diff --git a/Makefile b/Makefile\n> index 0e13a5b469..44cfc63fc4 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -433,6 +433,8 @@ all::\n>  #\n>  # Define HAVE_GETDELIM if your system has the getdelim() function.\n>  #\n> +# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real function.\n\ns/is fileno() is/if filno() is/;\n\nI agree with the direction of this patch takes us.\n\nI have a mixed feeling about the COMPAT_CODE macro trick (presumably\nyou did not want to include stdio.h directly for \"FILE *\" and\ninstead follow the usual \"the first header included is the\ncompat-util\" pattern; I was imagining that we'd liberally include\nsystem headers inside compat/ directory.  If we can agree on reusing\nthis single macro to control conditional in git-compat-util.h when a\nsimilar need arises in the future, this may be a good solution.\n\n> +#\n>  # Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n>  # default environment variables to be passed when a pager is spawned, e.g.\n>  #\n> @@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR\n>  \tBASIC_CFLAGS += -DHAVE_WPGMPTR\n>  endif\n>  \n> +ifdef FILENO_IS_A_MACRO\n> +\tCOMPAT_CFLAGS += -DFILENO_IS_A_MACRO\n> +\tCOMPAT_OBJS += compat/fileno.o\n> +endif\n> +\n>  ifeq ($(TCLTK_PATH),)\n>  NO_TCLTK = NoThanks\n>  endif\n> diff --git a/compat/fileno.c b/compat/fileno.c\n> new file mode 100644\n> index 0000000000..7b105f4cd7\n> --- /dev/null\n> +++ b/compat/fileno.c\n> @@ -0,0 +1,7 @@\n> +#define COMPAT_CODE\n> +#include \"../git-compat-util.h\"\n> +\n> +int git_fileno(FILE *stream)\n> +{\n> +\treturn fileno(stream);\n> +}\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 786bb2f913..01f62674a4 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD)\n>  \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n>  \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n>  \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n> +\tFILENO_IS_A_MACRO = UnfortunatelyYes\n>  endif\n>  ifeq ($(uname_S),OpenBSD)\n>  \tNO_STRCASESTR = YesPlease\n> @@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD)\n>  \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n>  \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n>  \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n> +\tFILENO_IS_A_MACRO = UnfortunatelyYes\n>  endif\n>  ifeq ($(uname_S),MirBSD)\n>  \tNO_STRCASESTR = YesPlease\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 29a19902aa..6573808ebd 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n>  #define getc_unlocked(fh) getc(fh)\n>  #endif\n>  \n> +#ifdef FILENO_IS_A_MACRO\n> +int git_fileno(FILE *stream);\n> +# ifndef COMPAT_CODE\n> +#  undef fileno\n> +#  define fileno(p) git_fileno(p)\n> +# endif\n> +#endif\n> +\n>  /*\n>   * Our code often opens a path to an optional file, to work on its\n>   * contents when we can successfully open it.  We can ignore a failure\n> -- 8< --\n>\n> --\n> Duy\n"},{"id":"369425","messageId":"D8E7C7D0-04E5-4802-80FA-2477F2C0D240@usask.ca","threadId":"50379","inReplyTo":"20190212134537.GA26137@ash","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Dan McGregor","fromEmail":"dkm560@usask.ca","sentAt":"2019-02-16T02:33:27Z","receivedAt":"2019-02-16T02:33:32Z","isPatch":true,"sender":{"key":"dkm560@usask.ca","avatar":null},"body":"\n\nOn February 12, 2019 7:45:37 a.m. CST, Duy Nguyen <pclouds@gmail.com> wrote:\n>On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote:\n>> Commit 8dd2e88a92 (\"http: support file handles for HTTP_KEEP_ERROR\",\n>> 2019-01-10) introduced an implicit assumption that rewind, fileno,\n>and\n>> fflush are functions. At least on FreeBSD fileno is not, and as such\n>> passing a void * failed.\n>> \n>> All systems tested (FreeBSD and NetBSD) that define fineo as a macro\n>\n>OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while\n>NetBSD compiles ok (and fails to run some tests)\n>\n>[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures\n>\n>> also have a function defined. Undefine the macro on these systems so\n>> that the function is used.\n>\n>I don't think this is enough. At least fbsd defines this\n>\n>#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))\n>\n>so one of the two functions will be used depending on __isthreaded\n>flag. Your forcing to use fileno, ignoring __sfileno, is technically\n>not correct.\n>\n>For the record, at least fbsd also defines feof, ferror, clearerr,\n>getc and putc in the same way. But at least I don't see how something\n>like feof(fp++) could cause bad side effects.\n>\n>So, how about something like this? A teeny bit longer than your\n>version, but I think it's easier to control long term.\n\nYes, this looks pretty reasonable to me too.\n>\n>-- 8< --\n>diff --git a/Makefile b/Makefile\n>index 0e13a5b469..44cfc63fc4 100644\n>--- a/Makefile\n>+++ b/Makefile\n>@@ -433,6 +433,8 @@ all::\n> #\n> # Define HAVE_GETDELIM if your system has the getdelim() function.\n> #\n>+# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real\n>function.\n>+#\n> # Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n># default environment variables to be passed when a pager is spawned,\n>e.g.\n> #\n>@@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR\n> \tBASIC_CFLAGS += -DHAVE_WPGMPTR\n> endif\n> \n>+ifdef FILENO_IS_A_MACRO\n>+\tCOMPAT_CFLAGS += -DFILENO_IS_A_MACRO\n>+\tCOMPAT_OBJS += compat/fileno.o\n>+endif\n>+\n> ifeq ($(TCLTK_PATH),)\n> NO_TCLTK = NoThanks\n> endif\n>diff --git a/compat/fileno.c b/compat/fileno.c\n>new file mode 100644\n>index 0000000000..7b105f4cd7\n>--- /dev/null\n>+++ b/compat/fileno.c\n>@@ -0,0 +1,7 @@\n>+#define COMPAT_CODE\n>+#include \"../git-compat-util.h\"\n>+\n>+int git_fileno(FILE *stream)\n>+{\n>+\treturn fileno(stream);\n>+}\n>diff --git a/config.mak.uname b/config.mak.uname\n>index 786bb2f913..01f62674a4 100644\n>--- a/config.mak.uname\n>+++ b/config.mak.uname\n>@@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD)\n> \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n> \tPAGER_ENV = LESS=FRX LV=-c MORE=FRX\n> \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n>+\tFILENO_IS_A_MACRO = UnfortunatelyYes\n> endif\n> ifeq ($(uname_S),OpenBSD)\n> \tNO_STRCASESTR = YesPlease\n>@@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD)\n> \tHAVE_BSD_KERN_PROC_SYSCTL = YesPlease\n> \tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n> \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n>+\tFILENO_IS_A_MACRO = UnfortunatelyYes\n> endif\n> ifeq ($(uname_S),MirBSD)\n> \tNO_STRCASESTR = YesPlease\n>diff --git a/git-compat-util.h b/git-compat-util.h\n>index 29a19902aa..6573808ebd 100644\n>--- a/git-compat-util.h\n>+++ b/git-compat-util.h\n>@@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct\n>tm *);\n> #define getc_unlocked(fh) getc(fh)\n> #endif\n> \n>+#ifdef FILENO_IS_A_MACRO\n>+int git_fileno(FILE *stream);\n>+# ifndef COMPAT_CODE\n>+#  undef fileno\n>+#  define fileno(p) git_fileno(p)\n>+# endif\n>+#endif\n>+\n> /*\n>  * Our code often opens a path to an optional file, to work on its\n>  * contents when we can successfully open it.  We can ignore a failure\n>-- 8< --\n>\n>--\n>Duy\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"375097","messageId":"xmqqk1f1gzgs.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"D8E7C7D0-04E5-4802-80FA-2477F2C0D240@usask.ca","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-08T07:46:59Z","receivedAt":"2019-05-08T07:47:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan McGregor <dkm560@usask.ca> writes:\n\n>>I don't think this is enough. At least fbsd defines this\n>>\n>>#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))\n>>\n>>so one of the two functions will be used depending on __isthreaded\n>>flag. Your forcing to use fileno, ignoring __sfileno, is technically\n>>not correct.\n>>\n>>For the record, at least fbsd also defines feof, ferror, clearerr,\n>>getc and putc in the same way. But at least I don't see how something\n>>like feof(fp++) could cause bad side effects.\n>>\n>>So, how about something like this? A teeny bit longer than your\n>>version, but I think it's easier to control long term.\n>\n> Yes, this looks pretty reasonable to me too.\n\nSorry for pinging this ancient thread, but while reviewing the\nstalled topics, this one caught my attention.  The very original\n<20190201193004.88736-1-dan.mcgregor@usask.ca> said that the problem\nit wants to solve was that the code that passes (void*) parameter to\nfileno(), fflush() and rewind() misbehaved, as these are all macros\non your system.\n\nWe solved the problem for fileno() being a macro eventually with\n18a4f6be (\"git-compat-util: work around fileno(fp) that is a macro\",\n2019-02-12), but what about the other two?\n\nHere comes a weather-balloon to see if we should pursue tying this\nloose end.\n\nOne thing to note is that this reveals that the build rule for\nvcs-svn stuff may be cleaned up before we do this as a separate\nstep, but I think I saw another topics to move it out of the main\nbuild so perhaps I'll leave it out and leave it to be worried about\nseparately ;-)\n\n-- >8 --\nFrom: Junio C Hamano <gitster@pobox.com>\nDate: Wed, 8 May 2019 16:12:20 +0900\nSubject: [PATCH] git-compat-util: rewind(fp)/fflush(fp) could be macros\n\nOn various BSD's, rewind(fp) is implemented as a macro that directly\naccesses the fields in the FILE * object, which breaks a function\nthat accepts a \"void *fp\" parameter and calls rewind(fp) and expect\nit to work.  The same issue has been resolved for fileno(fp) earlier,\nwhich is shared by fflush(fp) as well.\n\nWork it around by adding a compile-time knob REWIND_IS_A_MACRO and\nFFLUSH_IS_A_MACRO for these two other functions that tells us to\ninsert a real helper function in the middle of the callchain.\n\nI'll leave it up to those who do work on affected platforms to add\nentries to config.mak.uname.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile          | 18 ++++++++++++++++--\n compat/fflush.c   |  7 +++++++\n compat/rewind.c   |  7 +++++++\n git-compat-util.h | 16 ++++++++++++++++\n 4 files changed, 46 insertions(+), 2 deletions(-)\n create mode 100644 compat/fflush.c\n create mode 100644 compat/rewind.c\n\ndiff --git a/Makefile b/Makefile\nindex 6e8d017e8e..1e18092583 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -433,6 +433,10 @@ all::\n #\n # Define HAVE_GETDELIM if your system has the getdelim() function.\n #\n+# Define FFLUSH_IS_A_MACRO if fflush() is a macro, not a real function.\n+#\n+# Define REWIND_IS_A_MACRO if rewind() is a macro, not a real function.\n+#\n # Define PAGER_ENV to a SP separated VAR=VAL pairs to define\n # default environment variables to be passed when a pager is spawned, e.g.\n #\n@@ -1795,6 +1799,16 @@ ifdef HAVE_WPGMPTR\n \tBASIC_CFLAGS += -DHAVE_WPGMPTR\n endif\n \n+ifdef FFLUSH_IS_A_MACRO\n+\tCOMPAT_CFLAGS += -DFFLUSH_IS_A_MACRO\n+\tCOMPAT_OBJS += compat/fflush.o\n+endif\n+\n+ifdef REWIND_IS_A_MACRO\n+\tCOMPAT_CFLAGS += -DREWIND_IS_A_MACRO\n+\tCOMPAT_OBJS += compat/rewind.o\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\n@@ -2404,8 +2418,8 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS)\n \t\t$(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS)\n \n git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB)\n-\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \\\n-\t$(VCSSVN_LIB)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \\\n+\t$(VCSSVN_LIB) $(LIBS)\n \n $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)\n \t$(QUIET_LNCP)$(RM) $@ && \\\ndiff --git a/compat/fflush.c b/compat/fflush.c\nnew file mode 100644\nindex 0000000000..55ac3e5542\n--- /dev/null\n+++ b/compat/fflush.c\n@@ -0,0 +1,7 @@\n+#define COMPAT_CODE_FFLUSH\n+#include \"../git-compat-util.h\"\n+\n+int git_fflush(FILE *stream)\n+{\n+\treturn fflush(stream);\n+}\ndiff --git a/compat/rewind.c b/compat/rewind.c\nnew file mode 100644\nindex 0000000000..e56656ff96\n--- /dev/null\n+++ b/compat/rewind.c\n@@ -0,0 +1,7 @@\n+#define COMPAT_CODE_REWIND\n+#include \"../git-compat-util.h\"\n+\n+void git_rewind(FILE *stream)\n+{\n+\trewind(stream);\n+}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 29a19902aa..f35a857785 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1234,6 +1234,22 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);\n #define getc_unlocked(fh) getc(fh)\n #endif\n \n+#ifdef FFLUSH_IS_A_MACRO\n+int git_fflush(FILE *stream);\n+# ifndef COMPAT_CODE_FFLUSH\n+#  undef fflush\n+#  define fflush(p) git_fflush(p)\n+# endif\n+#endif\n+\n+#ifdef REWIND_IS_A_MACRO\n+void git_rewind(FILE *stream);\n+# ifndef COMPAT_CODE_REWIND\n+#  undef rewind\n+#  define rewind(p) git_rewind(p)\n+# endif\n+#endif\n+\n /*\n  * Our code often opens a path to an optional file, to work on its\n  * contents when we can successfully open it.  We can ignore a failure\n-- \n2.21.0-777-g83232e3864\n\n\n\n"},{"id":"375101","messageId":"CACsJy8BcyD199L4qGv6-TP-8HD+GS+ZDNN5jspkh5uVaWekkoQ@mail.gmail.com","threadId":"50379","inReplyTo":"xmqqk1f1gzgs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-08T10:09:05Z","receivedAt":"2019-05-08T10:09:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 8, 2019 at 2:47 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Dan McGregor <dkm560@usask.ca> writes:\n>\n> >>I don't think this is enough. At least fbsd defines this\n> >>\n> >>#define    fileno(p)    (!__isthreaded ? __sfileno(p) : (fileno)(p))\n> >>\n> >>so one of the two functions will be used depending on __isthreaded\n> >>flag. Your forcing to use fileno, ignoring __sfileno, is technically\n> >>not correct.\n> >>\n> >>For the record, at least fbsd also defines feof, ferror, clearerr,\n> >>getc and putc in the same way. But at least I don't see how something\n> >>like feof(fp++) could cause bad side effects.\n> >>\n> >>So, how about something like this? A teeny bit longer than your\n> >>version, but I think it's easier to control long term.\n> >\n> > Yes, this looks pretty reasonable to me too.\n>\n> Sorry for pinging this ancient thread, but while reviewing the\n> stalled topics, this one caught my attention.  The very original\n> <20190201193004.88736-1-dan.mcgregor@usask.ca> said that the problem\n> it wants to solve was that the code that passes (void*) parameter to\n> fileno(), fflush() and rewind() misbehaved, as these are all macros\n> on your system.\n>\n> We solved the problem for fileno() being a macro eventually with\n> 18a4f6be (\"git-compat-util: work around fileno(fp) that is a macro\",\n> 2019-02-12), but what about the other two?\n\nI don't think the other two were the problem. Even in the mail you\npointed to, only fileno() is acknowledged the problem in the commit\nmessage.\n\nAt least for BSDs fflush() and rewind() are not macros. BSDs optimize\na couple functions for the no-pthread case by inlining some short\nexpressions directly [1]. This works well for read-only functions such\nas fileno(), feof(), ferror().. and pushing the limits a bit with\nfputc() and fgetc(), but fflush() and rewind() are (I think) just too\ncomplicated to do it this way.\n\n[1] https://github.com/freebsd/freebsd/blob/master/include/stdio.h#L451-L519\n[2] https://public-inbox.org/git/20190209023621.75255-1-dan.mcgregor@usask.ca/\n\n> Here comes a weather-balloon to see if we should pursue tying this\n> loose end.\n\nLet's see if there are still problems with more exotic platforms. I'm\nreluctant of adding unused compat/ code because to me compat/ is\nscary. I often don't have the right systems to test whenever I need to\nmake changes in compat/\n-- \nDuy\n"},{"id":"375103","messageId":"xmqqv9ylfdzj.fsf@gitster-ct.c.googlers.com","threadId":"50379","inReplyTo":"CACsJy8BcyD199L4qGv6-TP-8HD+GS+ZDNN5jspkh5uVaWekkoQ@mail.gmail.com","subject":"Re: [PATCH v2] git-compat-util: undefine fileno if defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-08T10:16:16Z","receivedAt":"2019-05-08T10:16:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> I don't think the other two were the problem. Even in the mail you\n> pointed to, only fileno() is acknowledged the problem in the commit\n> message.\n>\n> At least for BSDs fflush() and rewind() are not macros.\n\nI can drop the dm/some-stdio-functions-are-macro-on-freebsd topic\nsafely then.  That was the most important thing I wanted to know\nabout ;-)\n\n> Let's see if there are still problems with more exotic platforms. I'm\n> reluctant of adding unused compat/ code because to me compat/ is\n> scary.\n\nRight.  Thanks.\n\n"}]}