{"thread":{"id":"31479","subject":"Restore hostname logging in inetd mode","startedAt":"2012-09-08T17:09:32Z","lastAt":"2012-09-10T20:15:45Z","messageCount":16,"participants":["Jan Engelhardt","Joachim Schmitz","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"198572","messageId":"1347124173-14460-1-git-send-email-jengelh@inai.de","threadId":"31479","inReplyTo":null,"subject":"Restore hostname logging in inetd mode","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2012-09-08T17:09:32Z","receivedAt":"2012-09-08T17:09:32Z","isPatch":false,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"\nThe following changes since commit 0ce986446163b37c7f663ce7a408e7f94c31ba63:\n\n  The fourth batch for 1.8.0 (2012-09-07 11:25:22 -0700)\n\nare available in the git repository at:\n\n  git://git.inai.de/git master\n\nfor you to fetch changes up to 864633738f6432574402afc43b6bd83c83fc8916:\n\n  daemon: restore getpeername(0,...) use (2012-09-08 19:00:35 +0200)\n\n----------------------------------------------------------------\nJan Engelhardt (1):\n      daemon: restore getpeername(0,...) use\n\n daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 51 insertions(+), 4 deletions(-)\n"},{"id":"198573","messageId":"1347124173-14460-2-git-send-email-jengelh@inai.de","threadId":"31479","inReplyTo":"1347124173-14460-1-git-send-email-jengelh@inai.de","subject":"[PATCH] daemon: restore getpeername(0,...) use","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2012-09-08T17:09:33Z","receivedAt":"2012-09-08T17:09:33Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,\nbecause that commit broke logging of \"Connection from ...\" when\ngit-daemon is run under xinetd.\n\nThis patch here computes the text representation of the peer and then\ncopies that to environment variables such that the code in execute()\nand subfunctions can stay as-is.\n\nSigned-off-by: Jan Engelhardt <jengelh@inai.de>\n---\n daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 51 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4602b46..eaf08c2 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1,3 +1,4 @@\n+#include <stdbool.h>\n #include \"cache.h\"\n #include \"pkt-line.h\"\n #include \"exec_cmd.h\"\n@@ -1164,6 +1165,54 @@ static int serve(struct string_list *listen_addr, int listen_port,\n \treturn service_loop(&socklist);\n }\n \n+static void inetd_mode_prepare(void)\n+{\n+\tstruct sockaddr_storage ss;\n+\tstruct sockaddr *addr = (void *)&ss;\n+\tsocklen_t slen = sizeof(ss);\n+\tchar addrbuf[256], portbuf[6] = \"\";\n+\n+\tif (!freopen(\"/dev/null\", \"w\", stderr))\n+\t\tdie_errno(\"failed to redirect stderr to /dev/null\");\n+\n+\t/*\n+\t * Windows is said to not be able to handle this, so we will simply\n+\t * ignore failure here. (It only affects a log message anyway.)\n+\t */\n+\tif (getpeername(0, addr, &slen) < 0)\n+\t\treturn;\n+\n+\tif (addr->sa_family == AF_INET) {\n+\t\tconst struct sockaddr_in *sin_addr = (void *)addr;\n+\n+\t\tif (inet_ntop(addr->sa_family, &sin_addr->sin_addr,\n+\t\t\t      addrbuf, sizeof(addrbuf)) == NULL)\n+\t\t\treturn;\n+\t\tsnprintf(portbuf, sizeof(portbuf), \"%hu\",\n+\t\t\t ntohs(sin_addr->sin_port));\n+#ifndef NO_IPV6\n+\t} else if (addr->sa_family == AF_INET6) {\n+\t\tconst struct sockaddr_in6 *sin6_addr = (void *)addr;\n+\n+\t\taddrbuf[0] = '[';\n+\t\taddrbuf[1] = '\\0';\n+\t\tif (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,\n+\t\t\t      sizeof(addrbuf) - 2) == NULL)\n+\t\t\treturn;\n+\t\tstrcat(addrbuf, \"]\");\n+\n+\t\tsnprintf(portbuf, sizeof(portbuf), \"%hu\",\n+\t\t\t ntohs(sin6_addr->sin6_port));\n+#endif\n+\t} else {\n+\t\tsnprintf(addrbuf, sizeof(addrbuf), \"<AF %d>\",\n+\t\t\t addr->sa_family);\n+\t}\n+\tif (setenv(\"REMOTE_ADDR\", addrbuf, true) < 0)\n+\t\treturn;\n+\tsetenv(\"REMOTE_PORT\", portbuf, true);\n+}\n+\n int main(int argc, char **argv)\n {\n \tint listen_port = 0;\n@@ -1341,10 +1390,8 @@ int main(int argc, char **argv)\n \t\tdie(\"base-path '%s' does not exist or is not a directory\",\n \t\t    base_path);\n \n-\tif (inetd_mode) {\n-\t\tif (!freopen(\"/dev/null\", \"w\", stderr))\n-\t\t\tdie_errno(\"failed to redirect stderr to /dev/null\");\n-\t}\n+\tif (inetd_mode)\n+\t\tinetd_mode_prepare();\n \n \tif (inetd_mode || serve_mode)\n \t\treturn execute();\n-- \n1.7.10.4\n"},{"id":"198574","messageId":"k2g0up$28h$1@ger.gmane.org","threadId":"31479","inReplyTo":"1347124173-14460-2-git-send-email-jengelh@inai.de","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-08T17:57:33Z","receivedAt":"2012-09-08T17:57:33Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Jan Engelhardt wrote:\n> This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,\n> because that commit broke logging of \"Connection from ...\" when\n> git-daemon is run under xinetd.\n> \n> This patch here computes the text representation of the peer and then\n> copies that to environment variables such that the code in execute()\n> and subfunctions can stay as-is.\n> \n> Signed-off-by: Jan Engelhardt <jengelh@inai.de>\n> ---\n> daemon.c |   55\n> +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file\n> changed, 51 insertions(+), 4 deletions(-) \n> \n> diff --git a/daemon.c b/daemon.c\n> index 4602b46..eaf08c2 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1,3 +1,4 @@\n> +#include <stdbool.h>\n> #include \"cache.h\"\n> #include \"pkt-line.h\"\n> #include \"exec_cmd.h\"\n> @@ -1164,6 +1165,54 @@ static int serve(struct string_list\n>  *listen_addr, int listen_port, return service_loop(&socklist);\n> }\n> \n> +static void inetd_mode_prepare(void)\n> +{\n> + struct sockaddr_storage ss;\n> + struct sockaddr *addr = (void *)&ss;\n> + socklen_t slen = sizeof(ss);\n> + char addrbuf[256], portbuf[6] = \"\";\n> +\n> + if (!freopen(\"/dev/null\", \"w\", stderr))\n> + die_errno(\"failed to redirect stderr to /dev/null\");\n> +\n> + /*\n> + * Windows is said to not be able to handle this, so we will simply\n> + * ignore failure here. (It only affects a log message anyway.)\n> + */\n> + if (getpeername(0, addr, &slen) < 0)\n> + return;\n> +\n> + if (addr->sa_family == AF_INET) {\n> + const struct sockaddr_in *sin_addr = (void *)addr;\n> +\n> + if (inet_ntop(addr->sa_family, &sin_addr->sin_addr,\n> +       addrbuf, sizeof(addrbuf)) == NULL)\n> + return;\n> + snprintf(portbuf, sizeof(portbuf), \"%hu\",\n> + ntohs(sin_addr->sin_port));\n> +#ifndef NO_IPV6\n> + } else if (addr->sa_family == AF_INET6) {\n> + const struct sockaddr_in6 *sin6_addr = (void *)addr;\n> +\n> + addrbuf[0] = '[';\n> + addrbuf[1] = '\\0';\n> + if (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,\n> +       sizeof(addrbuf) - 2) == NULL)\n> + return;\n> + strcat(addrbuf, \"]\");\n> +\n> + snprintf(portbuf, sizeof(portbuf), \"%hu\",\n> + ntohs(sin6_addr->sin6_port));\n> +#endif\n> + } else {\n> + snprintf(addrbuf, sizeof(addrbuf), \"<AF %d>\",\n> + addr->sa_family);\n> + }\n> + if (setenv(\"REMOTE_ADDR\", addrbuf, true) < 0)\n> + return;\n> + setenv(\"REMOTE_PORT\", portbuf, true);\n\nsetenv() is not a function available on all plattfomrs.\n\nBye, Jojo\n"},{"id":"198577","messageId":"7v627ouyun.fsf@alter.siamese.dyndns.org","threadId":"31479","inReplyTo":"1347124173-14460-1-git-send-email-jengelh@inai.de","subject":"Re: Restore hostname logging in inetd mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-08T18:57:20Z","receivedAt":"2012-09-08T18:57:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Please don't throw a pull request for a patch whose worth hasn't\nbeen justified in a discussion on the list.  Thanks.\n"},{"id":"198578","messageId":"7v1uicuyqi.fsf@alter.siamese.dyndns.org","threadId":"31479","inReplyTo":"1347124173-14460-2-git-send-email-jengelh@inai.de","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-08T18:59:49Z","receivedAt":"2012-09-08T18:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jan Engelhardt <jengelh@inai.de> writes:\n\n> This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,\n> because that commit broke logging of \"Connection from ...\" when\n> git-daemon is run under xinetd.\n>\n> This patch here computes the text representation of the peer and then\n> copies that to environment variables such that the code in execute()\n> and subfunctions can stay as-is.\n>\n> Signed-off-by: Jan Engelhardt <jengelh@inai.de>\n> ---\n>  daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----\n>  1 file changed, 51 insertions(+), 4 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 4602b46..eaf08c2 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -1,3 +1,4 @@\n> +#include <stdbool.h>\n>  #include \"cache.h\"\n>  #include \"pkt-line.h\"\n>  #include \"exec_cmd.h\"\n\nPlatform agnostic parts of the code that use \"git-compat-util.h\"\n(users of \"cache.h\" are indirectly users of it) are not allowed to\ndo platform specific include like this at their beginning.\n\nThis is the first use of stdbool.h; what do you need it for?\n\n> @@ -1164,6 +1165,54 @@ static int serve(struct string_list *listen_addr, int listen_port,\n>  \treturn service_loop(&socklist);\n>  }\n>  \n> +static void inetd_mode_prepare(void)\n> +{\n> +\tstruct sockaddr_storage ss;\n> +\tstruct sockaddr *addr = (void *)&ss;\n> +\tsocklen_t slen = sizeof(ss);\n> +\tchar addrbuf[256], portbuf[6] = \"\";\n> +\n> +\tif (!freopen(\"/dev/null\", \"w\", stderr))\n> +\t\tdie_errno(\"failed to redirect stderr to /dev/null\");\n> +\n> +\t/*\n> +\t * Windows is said to not be able to handle this, so we will simply\n> +\t * ignore failure here. (It only affects a log message anyway.)\n> +\t */\n> +\tif (getpeername(0, addr, &slen) < 0)\n> +\t\treturn;\n> +\n> +\tif (addr->sa_family == AF_INET) {\n> +\t\tconst struct sockaddr_in *sin_addr = (void *)addr;\n> +\n> +\t\tif (inet_ntop(addr->sa_family, &sin_addr->sin_addr,\n> +\t\t\t      addrbuf, sizeof(addrbuf)) == NULL)\n> +\t\t\treturn;\n> +\t\tsnprintf(portbuf, sizeof(portbuf), \"%hu\",\n> +\t\t\t ntohs(sin_addr->sin_port));\n> +#ifndef NO_IPV6\n> +\t} else if (addr->sa_family == AF_INET6) {\n> +\t\tconst struct sockaddr_in6 *sin6_addr = (void *)addr;\n> +\n> +\t\taddrbuf[0] = '[';\n> +\t\taddrbuf[1] = '\\0';\n> +\t\tif (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,\n> +\t\t\t      sizeof(addrbuf) - 2) == NULL)\n> +\t\t\treturn;\n> +\t\tstrcat(addrbuf, \"]\");\n> +\n> +\t\tsnprintf(portbuf, sizeof(portbuf), \"%hu\",\n> +\t\t\t ntohs(sin6_addr->sin6_port));\n> +#endif\n> +\t} else {\n> +\t\tsnprintf(addrbuf, sizeof(addrbuf), \"<AF %d>\",\n> +\t\t\t addr->sa_family);\n> +\t}\n> +\tif (setenv(\"REMOTE_ADDR\", addrbuf, true) < 0)\n> +\t\treturn;\n> +\tsetenv(\"REMOTE_PORT\", portbuf, true);\n> +}\n> +\n>  int main(int argc, char **argv)\n>  {\n>  \tint listen_port = 0;\n> @@ -1341,10 +1390,8 @@ int main(int argc, char **argv)\n>  \t\tdie(\"base-path '%s' does not exist or is not a directory\",\n>  \t\t    base_path);\n>  \n> -\tif (inetd_mode) {\n> -\t\tif (!freopen(\"/dev/null\", \"w\", stderr))\n> -\t\t\tdie_errno(\"failed to redirect stderr to /dev/null\");\n> -\t}\n> +\tif (inetd_mode)\n> +\t\tinetd_mode_prepare();\n>  \n>  \tif (inetd_mode || serve_mode)\n>  \t\treturn execute();\n"},{"id":"198580","messageId":"7vwr04tjzo.fsf@alter.siamese.dyndns.org","threadId":"31479","inReplyTo":"k2g0up$28h$1@ger.gmane.org","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-08T19:03:39Z","receivedAt":"2012-09-08T19:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n\n>> + setenv(\"REMOTE_PORT\", portbuf, true);\n>\n> setenv() is not a function available on all plattfomrs.\n\nPlease do some homework before adding irrelevant noise.  At the\nminimum, run \"git grep\" to see if we already use it in other places,\nand investigate why we can use it safely across platforms we already\nsupport.\n"},{"id":"198581","messageId":"alpine.LNX.2.01.1209082112040.18369@frira.zrqbmnf.qr","threadId":"31479","inReplyTo":"7v627ouyun.fsf@alter.siamese.dyndns.org","subject":"Re: Restore hostname logging in inetd mode","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2012-09-08T19:18:56Z","receivedAt":"2012-09-08T19:18:56Z","isPatch":false,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"On Saturday 2012-09-08 20:57, Junio C Hamano wrote:\n\n>Please don't throw a pull request for a patch whose worth hasn't\n>been justified in a discussion on the list.  Thanks.\n\nLet me postulate that people like to get cover letters with the\ngit:// URL so they can fetch+look at it, a diffstat and shortlog.\n\nAnd 'lo, that is exactly what git-request-pull thankfully generates.\n\nIn my defense: Just because the command is called \"request-pull\",\ndoes not mean you absolutely have to merge/pull it. In fact, it does\nnot even mention merge/pull at all.\n\n\"\nThe following changes since commit [SHA]:\n  [Commit Message]\nare available in the git repository at:\n  git://[...]\nfor you to fetch changes up to [SHA]:\n  [Commit Message]\n[diffstat,shortlog]\n\"\n\nIn contrast to many a LKML postings which explicitly state the pull\nintent:\n\n\"\nHi [Maintainer],\n\nPlease pull from the git repository at\n  [URL]\nto receive [...]\n  [SHA]\n  [Commit Message]\non top of commit [SHA]\n  [Commit Message]\n[diffstat,shortlog]\n\"\n"},{"id":"198582","messageId":"002101cd8df6$f7cab2a0$e76017e0$@schmitz-digital.de","threadId":"31479","inReplyTo":"7vwr04tjzo.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-08T19:20:11Z","receivedAt":"2012-09-08T19:20:11Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Junio C Hamano [mailto:gitster@pobox.com]\n> Sent: Saturday, September 08, 2012 9:04 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH] daemon: restore getpeername(0,...) use\n> \n> \"Joachim Schmitz\" <jojo@schmitz-digital.de> writes:\n> \n> >> + setenv(\"REMOTE_PORT\", portbuf, true);\n> >\n> > setenv() is not a function available on all plattfomrs.\n> \n> Please do some homework before adding irrelevant noise.  At the\n> minimum, run \"git grep\" to see if we already use it in other places,\n> and investigate why we can use it safely across platforms we already\n> support.\n\nHmm, guess I missed the indirect inclusion of git-compat-util.h\nAnd didn't know about 'git grep', so thanks for the hint\n\nWill look closer next time ;-)\n"},{"id":"198583","messageId":"alpine.LNX.2.01.1209082119320.18369@frira.zrqbmnf.qr","threadId":"31479","inReplyTo":"7v1uicuyqi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Jan Engelhardt","fromEmail":"jengelh@inai.de","sentAt":"2012-09-08T19:20:48Z","receivedAt":"2012-09-08T19:20:48Z","isPatch":true,"sender":{"key":"jengelh@inai.de","avatar":"https://avatars.githubusercontent.com/u/8861948?v=4"},"body":"\nOn Saturday 2012-09-08 20:59, Junio C Hamano wrote:\n>> diff --git a/daemon.c b/daemon.c\n>> index 4602b46..eaf08c2 100644\n>> --- a/daemon.c\n>> +++ b/daemon.c\n>> @@ -1,3 +1,4 @@\n>> +#include <stdbool.h>\n>>  #include \"cache.h\"\n>>  #include \"pkt-line.h\"\n>>  #include \"exec_cmd.h\"\n>\n>Platform agnostic parts of the code that use \"git-compat-util.h\"\n>(users of \"cache.h\" are indirectly users of it) are not allowed to\n>do platform specific include like this at their beginning.\n>\n>This is the first use of stdbool.h; what do you need it for?\n\nFor the use in setenv(,,true). It was not entirely obvious in which .h \nto add it; the most reasonable place was daemon.c itself, since the \nother .c files do not seem to need it.\n"},{"id":"198676","messageId":"20120910142100.GB7906@sigill.intra.peff.net","threadId":"31479","inReplyTo":"alpine.LNX.2.01.1209082119320.18369@frira.zrqbmnf.qr","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T14:21:00Z","receivedAt":"2012-09-10T14:21:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote:\n\n> On Saturday 2012-09-08 20:59, Junio C Hamano wrote:\n> >> diff --git a/daemon.c b/daemon.c\n> >> index 4602b46..eaf08c2 100644\n> >> --- a/daemon.c\n> >> +++ b/daemon.c\n> >> @@ -1,3 +1,4 @@\n> >> +#include <stdbool.h>\n> >>  #include \"cache.h\"\n> >>  #include \"pkt-line.h\"\n> >>  #include \"exec_cmd.h\"\n> >\n> >Platform agnostic parts of the code that use \"git-compat-util.h\"\n> >(users of \"cache.h\" are indirectly users of it) are not allowed to\n> >do platform specific include like this at their beginning.\n> >\n> >This is the first use of stdbool.h; what do you need it for?\n> \n> For the use in setenv(,,true). It was not entirely obvious in which .h \n> to add it; the most reasonable place was daemon.c itself, since the \n> other .c files do not seem to need it.\n\nIt would go in git-compat-util.h. However, it really is not needed; you\ncan simply pass \"1\" to setenv, as every other callsite in git does.\n\nMore importantly, though, is it actually portable? I thought it was\nadded in C99, and we try to stick to C89 to support older compilers and\nsystems. My copy of C99 is vague (it says only that the \"bool\" macro was\nadded via stdbool.h in C99, but nothing about the \"true\" and \"false\"\nmacros), and I don't have a copy of C89 handy.  Wikipedia does claim the\nheader wasn't standardized at all until C99:\n\n  https://en.wikipedia.org/wiki/C_standard_library\n\n-Peff\n"},{"id":"198677","messageId":"k2ku26$jld$1@ger.gmane.org","threadId":"31479","inReplyTo":"20120910142100.GB7906@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-10T14:38:58Z","receivedAt":"2012-09-10T14:38:58Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Jeff King wrote:\n> On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote:\n>\n>> On Saturday 2012-09-08 20:59, Junio C Hamano wrote:\n>>>> diff --git a/daemon.c b/daemon.c\n>>>> index 4602b46..eaf08c2 100644\n>>>> --- a/daemon.c\n>>>> +++ b/daemon.c\n>>>> @@ -1,3 +1,4 @@\n>>>> +#include <stdbool.h>\n>>>>  #include \"cache.h\"\n>>>>  #include \"pkt-line.h\"\n>>>>  #include \"exec_cmd.h\"\n>>>\n>>> Platform agnostic parts of the code that use \"git-compat-util.h\"\n>>> (users of \"cache.h\" are indirectly users of it) are not allowed to\n>>> do platform specific include like this at their beginning.\n>>>\n>>> This is the first use of stdbool.h; what do you need it for?\n>>\n>> For the use in setenv(,,true). It was not entirely obvious in which\n>> .h to add it; the most reasonable place was daemon.c itself, since\n>> the other .c files do not seem to need it.\n>\n> It would go in git-compat-util.h. However, it really is not needed;\n> you can simply pass \"1\" to setenv, as every other callsite in git\n> does.\n>\n> More importantly, though, is it actually portable? I thought it was\n> added in C99, and we try to stick to C89 to support older compilers\n> and systems. My copy of C99 is vague (it says only that the \"bool\"\n> macro was added via stdbool.h in C99, but nothing about the \"true\"\n> and \"false\" macros), and I don't have a copy of C89 handy.  Wikipedia\n> does claim the header wasn't standardized at all until C99:\n>\n>  https://en.wikipedia.org/wiki/C_standard_library\n\nIndeed stdbool is not part of C89, but inline isn't either and used \nextensively in git (could possible be defined away), as are non-const array \nintializers, e.g.:\n\n\n                const char *args[] = { editor, path, NULL };\n                                               ^\n\".../git/editor.c\", line 39: error(122): expression must have a constant \nvalue\n\nSo git source is not plain C89 code (anymore?)\n\nBye, Jojo \n"},{"id":"198678","messageId":"20120910155006.GA8737@sigill.intra.peff.net","threadId":"31479","inReplyTo":"k2ku26$jld$1@ger.gmane.org","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T15:50:06Z","receivedAt":"2012-09-10T15:50:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:\n\n> >More importantly, though, is it actually portable? I thought it was\n> >added in C99, and we try to stick to C89 to support older compilers\n> >and systems. My copy of C99 is vague (it says only that the \"bool\"\n> >macro was added via stdbool.h in C99, but nothing about the \"true\"\n> >and \"false\" macros), and I don't have a copy of C89 handy.  Wikipedia\n> >does claim the header wasn't standardized at all until C99:\n> >\n> > https://en.wikipedia.org/wiki/C_standard_library\n> \n> Indeed stdbool is not part of C89, but inline isn't either and used\n> extensively in git (could possible be defined away),\n\nYou can define INLINE in the Makefile to disable it (or set it to\nsomething more appropriate for your system).\n\n> as are non-const array intializers, e.g.:\n> \n>                const char *args[] = { editor, path, NULL };\n>                                               ^\n> \".../git/editor.c\", line 39: error(122): expression must have a\n> constant value\n>\n> So git source is not plain C89 code (anymore?)\n\nI remember we excised a whole bunch of non-constant initializers at some\npoint because somebody's compiler was complaining. But I suppose this\none has slipped back in, because non-constant initializers are so damn\nuseful. And nobody has complained, which I imagine means nobody has\nbothered building lately on those older systems that complained.\n\nMy \"we stick to C89\" is a little bit of a lie. We do not care about\nspecific standards. We do care about running everywhere on reasonable\nsystems. So something that is C99 might be OK if realistically everybody\nhas it. And something that is POSIX is not automatically OK if there are\nmany real-world systems that do not have it.\n\nSince there is no written standard, there tends to be an organic ebb and\nflow in which features we use. Somebody will use a feature that is not\nportable because it's useful to them, and then somebody whose system\nwill no longer build git will complain, and then we'll fix it and move\non. As reviewers, we try to anticipate those breakages and stop them\nearly (especially if they are ones we have seen before and know are a\nproblem), but we do not always get it right. And sometimes it is even\ntime to revisit old decisions (the line you mentioned is 2 years old,\nand nobody has complained; maybe all of the old systems have become\nobsolete, and we no longer need to care about constant initializers).\n\nGetting back to the patch at hand, it may be that stdbool is practically\navailable everywhere. Or that we could trivially emulate it by defining\na \"true\" macro in this case. But it is also important to consider\nwhether that complexity is worth it. This would be the first and only\nspot in git to use \"true\". Why bother?\n\n-Peff\n"},{"id":"198700","messageId":"k2l7s5$gl9$1@ger.gmane.org","threadId":"31479","inReplyTo":"20120910155006.GA8737@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-10T17:26:26Z","receivedAt":"2012-09-10T17:26:26Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:\n>\n>>> More importantly, though, is it actually portable? I thought it was\n>>> added in C99, and we try to stick to C89 to support older compilers\n>>> and systems. My copy of C99 is vague (it says only that the \"bool\"\n>>> macro was added via stdbool.h in C99, but nothing about the \"true\"\n>>> and \"false\" macros), and I don't have a copy of C89 handy.\n>>> Wikipedia does claim the header wasn't standardized at all until\n>>> C99:\n>>>\n>>> https://en.wikipedia.org/wiki/C_standard_library\n>>\n>> Indeed stdbool is not part of C89, but inline isn't either and used\n>> extensively in git (could possible be defined away),\n>\n> You can define INLINE in the Makefile to disable it (or set it to\n> something more appropriate for your system).\n\nThat's what I meant.\n\n>> as are non-const array intializers, e.g.:\n>>\n>>                const char *args[] = { editor, path, NULL };\n>>                                               ^\n>> \".../git/editor.c\", line 39: error(122): expression must have a\n>> constant value\n>>\n>> So git source is not plain C89 code (anymore?)\n>\n> I remember we excised a whole bunch of non-constant initializers at\n> some point because somebody's compiler was complaining. But I suppose\n> this one has slipped back in, because non-constant initializers are\n> so damn useful. And nobody has complained, which I imagine means\n> nobody has bothered building lately on those older systems that\n> complained.\n\nOK, record my complaint then ;-)\nAt least some older release of HP NonStop only have C89 and are still in use\n\nAnd tying to compile in plain C89 mode revealed several other problems too \n(e.g. size_t seems not to be typedef'd?)\n\n> My \"we stick to C89\" is a little bit of a lie. We do not care about\n> specific standards. We do care about running everywhere on reasonable\n> systems. So something that is C99 might be OK if realistically\n> everybody has it. And something that is POSIX is not automatically OK\n> if there are many real-world systems that do not have it.\n>\n> Since there is no written standard, there tends to be an organic ebb\n> and flow in which features we use. Somebody will use a feature that\n> is not portable because it's useful to them, and then somebody whose\n> system will no longer build git will complain, and then we'll fix it\n> and move on. As reviewers, we try to anticipate those breakages and\n> stop them early (especially if they are ones we have seen before and\n> know are a problem), but we do not always get it right. And sometimes\n> it is even time to revisit old decisions (the line you mentioned is 2\n> years old, and nobody has complained; maybe all of the old systems\n> have become obsolete, and we no longer need to care about constant\n> initializers).\n>\n> Getting back to the patch at hand, it may be that stdbool is\n> practically available everywhere. Or that we could trivially emulate\n> it by defining a \"true\" macro in this case. But it is also important\n> to consider whether that complexity is worth it. This would be the\n> first and only spot in git to use \"true\". Why bother?\n>\n> -Peff \n"},{"id":"198705","messageId":"20120910175846.GA16791@sigill.intra.peff.net","threadId":"31479","inReplyTo":"k2l7s5$gl9$1@ger.gmane.org","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T17:58:46Z","receivedAt":"2012-09-10T17:58:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:\n\n> >>as are non-const array intializers, e.g.:\n> >>\n> >>               const char *args[] = { editor, path, NULL };\n> >>                                              ^\n> >>\".../git/editor.c\", line 39: error(122): expression must have a\n> >>constant value\n> >>\n> >>So git source is not plain C89 code (anymore?)\n> >\n> >I remember we excised a whole bunch of non-constant initializers at\n> >some point because somebody's compiler was complaining. But I suppose\n> >this one has slipped back in, because non-constant initializers are\n> >so damn useful. And nobody has complained, which I imagine means\n> >nobody has bothered building lately on those older systems that\n> >complained.\n> \n> OK, record my complaint then ;-)\n\nOops, did I say \"complained\"? I meant \"sent patches\". Hint, hint. :)\n\n> At least some older release of HP NonStop only have C89 and are still in use\n> \n> And tying to compile in plain C89 mode revealed several other\n> problems too (e.g. size_t seems not to be typedef'd?)\n\nI think it is a mistake to set -std=c89 (or whatever similar option your\ncompiler supports). Like I said, we are not interested in being strictly\nC89-compliant. We are interested in working on real-world systems.\n\nIf your compiler complains in the default mode (or when it is given some\nreasonable practical settings), then that's something worth fixing. But\nif your compiler is perfectly capable of compiling git, but you choose\nto cripple it by telling it to be pedantic about a standard, then that\nis not git's problem at all.\n\n-Peff\n"},{"id":"198706","messageId":"004901cd8f81$e2bb20c0$a8316240$@schmitz-digital.de","threadId":"31479","inReplyTo":"20120910175846.GA16791@sigill.intra.peff.net","subject":"RE: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2012-09-10T18:27:07Z","receivedAt":"2012-09-10T18:27:07Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"> From: Jeff King [mailto:peff@peff.net]\n> Sent: Monday, September 10, 2012 7:59 PM\n> To: Joachim Schmitz\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH] daemon: restore getpeername(0,...) use\n> \n> On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:\n> \n> > >>as are non-const array intializers, e.g.:\n> > >>\n> > >>               const char *args[] = { editor, path, NULL };\n> > >>                                              ^\n> > >>\".../git/editor.c\", line 39: error(122): expression must have a\n> > >>constant value\n> > >>\n> > >>So git source is not plain C89 code (anymore?)\n> > >\n> > >I remember we excised a whole bunch of non-constant initializers at\n> > >some point because somebody's compiler was complaining. But I suppose\n> > >this one has slipped back in, because non-constant initializers are\n> > >so damn useful. And nobody has complained, which I imagine means\n> > >nobody has bothered building lately on those older systems that\n> > >complained.\n> >\n> > OK, record my complaint then ;-)\n> \n> Oops, did I say \"complained\"? I meant \"sent patches\". Hint, hint. :)\n\nOops ;-)\n \n> > At least some older release of HP NonStop only have C89 and are still in use\n> >\n> > And tying to compile in plain C89 mode revealed several other\n> > problems too (e.g. size_t seems not to be typedef'd?)\n> \n> I think it is a mistake to set -std=c89 (or whatever similar option your\n> compiler supports). Like I said, we are not interested in being strictly\n> C89-compliant. We are interested in working on real-world systems.\n> \n> If your compiler complains in the default mode (or when it is given some\n> reasonable practical settings), then that's something worth fixing. But\n> if your compiler is perfectly capable of compiling git, but you choose\n> to cripple it by telling it to be pedantic about a standard, then that\n> is not git's problem at all.\n\nOlder version of HP NonStop only have a c89 compiler, newer have a -Wc99lite switch to that, which enables some C99 features and the latest additionally have a c99 compiler. There's no switch to cripple something, it is just a fact that older systems don't have c99 or only limited support for it. A whole series of machines (which is still in use!) cannot get upgraded to anything better than c89.\n\nBye, Jojo\n"},{"id":"198721","messageId":"20120910201545.GC32437@sigill.intra.peff.net","threadId":"31479","inReplyTo":"004901cd8f81$e2bb20c0$a8316240$@schmitz-digital.de","subject":"Re: [PATCH] daemon: restore getpeername(0,...) use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-10T20:15:45Z","receivedAt":"2012-09-10T20:15:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2012 at 08:27:07PM +0200, Joachim Schmitz wrote:\n\n> > I think it is a mistake to set -std=c89 (or whatever similar option your\n> > compiler supports). Like I said, we are not interested in being strictly\n> > C89-compliant. We are interested in working on real-world systems.\n> > \n> > If your compiler complains in the default mode (or when it is given some\n> > reasonable practical settings), then that's something worth fixing. But\n> > if your compiler is perfectly capable of compiling git, but you choose\n> > to cripple it by telling it to be pedantic about a standard, then that\n> > is not git's problem at all.\n> \n> Older version of HP NonStop only have a c89 compiler, newer have a\n> -Wc99lite switch to that, which enables some C99 features and the\n> latest additionally have a c99 compiler. There's no switch to cripple\n> something, it is just a fact that older systems don't have c99 or only\n> limited support for it. A whole series of machines (which is still in\n> use!) cannot get upgraded to anything better than c89.\n\nIf you are using a compiler switch to emulate a real environment, then\nmy comments above do not apply. I was speaking against standard pedantry\nfor its own sake, which I have no interest in.\n\nHowever, do be careful that your emulated environment (i.e., recent\nNonStop but using compiler flags to pretend you are the older version)\nis accurate, and not introducing new portability annoyances that do not\ntruly exist on the old system. In fact, I might go so far as to say if\nyou cannot actually come up with an instance of the older platform to\ntest it, it might not even be worth our time to care about.\n\n-Peff\n"}]}