Restore hostname logging in inetd mode

Restore hostname logging in inetd mode

[Jan Engelhardt]

The following changes since commit 0ce986446163b37c7f663ce7a408e7f94c31ba63:

  The fourth batch for 1.8.0 (2012-09-07 11:25:22 -0700)

are available in the git repository at:

  git://git.inai.de/git master

for you to fetch changes up to 864633738f6432574402afc43b6bd83c83fc8916:

  daemon: restore getpeername(0,...) use (2012-09-08 19:00:35 +0200)

----------------------------------------------------------------
Jan Engelhardt (1):
      daemon: restore getpeername(0,...) use

 daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

[PATCH] daemon: restore getpeername(0,...) use

[Jan Engelhardt]

This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,
because that commit broke logging of "Connection from ..." when
git-daemon is run under xinetd.

This patch here computes the text representation of the peer and then
copies that to environment variables such that the code in execute()
and subfunctions can stay as-is.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4602b46..eaf08c2 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,3 +1,4 @@
+#include <stdbool.h>
 #include "cache.h"
 #include "pkt-line.h"
 #include "exec_cmd.h"
@@ -1164,6 +1165,54 @@ static int serve(struct string_list *listen_addr, int listen_port,
 	return service_loop(&socklist);
 }
 
+static void inetd_mode_prepare(void)
+{
+	struct sockaddr_storage ss;
+	struct sockaddr *addr = (void *)&ss;
+	socklen_t slen = sizeof(ss);
+	char addrbuf[256], portbuf[6] = "";
+
+	if (!freopen("/dev/null", "w", stderr))
+		die_errno("failed to redirect stderr to /dev/null");
+
+	/*
+	 * Windows is said to not be able to handle this, so we will simply
+	 * ignore failure here. (It only affects a log message anyway.)
+	 */
+	if (getpeername(0, addr, &slen) < 0)
+		return;
+
+	if (addr->sa_family == AF_INET) {
+		const struct sockaddr_in *sin_addr = (void *)addr;
+
+		if (inet_ntop(addr->sa_family, &sin_addr->sin_addr,
+			      addrbuf, sizeof(addrbuf)) == NULL)
+			return;
+		snprintf(portbuf, sizeof(portbuf), "%hu",
+			 ntohs(sin_addr->sin_port));
+#ifndef NO_IPV6
+	} else if (addr->sa_family == AF_INET6) {
+		const struct sockaddr_in6 *sin6_addr = (void *)addr;
+
+		addrbuf[0] = '[';
+		addrbuf[1] = '\0';
+		if (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,
+			      sizeof(addrbuf) - 2) == NULL)
+			return;
+		strcat(addrbuf, "]");
+
+		snprintf(portbuf, sizeof(portbuf), "%hu",
+			 ntohs(sin6_addr->sin6_port));
+#endif
+	} else {
+		snprintf(addrbuf, sizeof(addrbuf), "<AF %d>",
+			 addr->sa_family);
+	}
+	if (setenv("REMOTE_ADDR", addrbuf, true) < 0)
+		return;
+	setenv("REMOTE_PORT", portbuf, true);
+}
+
 int main(int argc, char **argv)
 {
 	int listen_port = 0;
@@ -1341,10 +1390,8 @@ int main(int argc, char **argv)
 		die("base-path '%s' does not exist or is not a directory",
 		    base_path);
 
-	if (inetd_mode) {
-		if (!freopen("/dev/null", "w", stderr))
-			die_errno("failed to redirect stderr to /dev/null");
-	}
+	if (inetd_mode)
+		inetd_mode_prepare();
 
 	if (inetd_mode || serve_mode)
 		return execute();
-- 
1.7.10.4

Re: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

Jan Engelhardt wrote:
> This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,
> because that commit broke logging of "Connection from ..." when
> git-daemon is run under xinetd.
> 
> This patch here computes the text representation of the peer and then
> copies that to environment variables such that the code in execute()
> and subfunctions can stay as-is.
> 
> Signed-off-by: Jan Engelhardt <jengelh@inai.de>
> ---
> daemon.c |   55
> +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file
> changed, 51 insertions(+), 4 deletions(-) 
> 
> diff --git a/daemon.c b/daemon.c
> index 4602b46..eaf08c2 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1,3 +1,4 @@
> +#include <stdbool.h>
> #include "cache.h"
> #include "pkt-line.h"
> #include "exec_cmd.h"
> @@ -1164,6 +1165,54 @@ static int serve(struct string_list
>  *listen_addr, int listen_port, return service_loop(&socklist);
> }
> 
> +static void inetd_mode_prepare(void)
> +{
> + struct sockaddr_storage ss;
> + struct sockaddr *addr = (void *)&ss;
> + socklen_t slen = sizeof(ss);
> + char addrbuf[256], portbuf[6] = "";
> +
> + if (!freopen("/dev/null", "w", stderr))
> + die_errno("failed to redirect stderr to /dev/null");
> +
> + /*
> + * Windows is said to not be able to handle this, so we will simply
> + * ignore failure here. (It only affects a log message anyway.)
> + */
> + if (getpeername(0, addr, &slen) < 0)
> + return;
> +
> + if (addr->sa_family == AF_INET) {
> + const struct sockaddr_in *sin_addr = (void *)addr;
> +
> + if (inet_ntop(addr->sa_family, &sin_addr->sin_addr,
> +       addrbuf, sizeof(addrbuf)) == NULL)
> + return;
> + snprintf(portbuf, sizeof(portbuf), "%hu",
> + ntohs(sin_addr->sin_port));
> +#ifndef NO_IPV6
> + } else if (addr->sa_family == AF_INET6) {
> + const struct sockaddr_in6 *sin6_addr = (void *)addr;
> +
> + addrbuf[0] = '[';
> + addrbuf[1] = '\0';
> + if (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,
> +       sizeof(addrbuf) - 2) == NULL)
> + return;
> + strcat(addrbuf, "]");
> +
> + snprintf(portbuf, sizeof(portbuf), "%hu",
> + ntohs(sin6_addr->sin6_port));
> +#endif
> + } else {
> + snprintf(addrbuf, sizeof(addrbuf), "<AF %d>",
> + addr->sa_family);
> + }
> + if (setenv("REMOTE_ADDR", addrbuf, true) < 0)
> + return;
> + setenv("REMOTE_PORT", portbuf, true);

setenv() is not a function available on all plattfomrs.

Bye, Jojo

Re: Restore hostname logging in inetd mode

[Junio C Hamano]

Please don't throw a pull request for a patch whose worth hasn't
been justified in a discussion on the list.  Thanks.

Re: [PATCH] daemon: restore getpeername(0,...) use

[Junio C Hamano]

Jan Engelhardt <jengelh@inai.de> writes:

> This reverts f9c87be6b42dd0f8b31a4bb8c6a44326879fdd1a, in a sense,
> because that commit broke logging of "Connection from ..." when
> git-daemon is run under xinetd.
>
> This patch here computes the text representation of the peer and then
> copies that to environment variables such that the code in execute()
> and subfunctions can stay as-is.
>
> Signed-off-by: Jan Engelhardt <jengelh@inai.de>
> ---
>  daemon.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 4602b46..eaf08c2 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -1,3 +1,4 @@
> +#include <stdbool.h>
>  #include "cache.h"
>  #include "pkt-line.h"
>  #include "exec_cmd.h"

Platform agnostic parts of the code that use "git-compat-util.h"
(users of "cache.h" are indirectly users of it) are not allowed to
do platform specific include like this at their beginning.

This is the first use of stdbool.h; what do you need it for?

> @@ -1164,6 +1165,54 @@ static int serve(struct string_list *listen_addr, int listen_port,
>  	return service_loop(&socklist);
>  }
>  
> +static void inetd_mode_prepare(void)
> +{
> +	struct sockaddr_storage ss;
> +	struct sockaddr *addr = (void *)&ss;
> +	socklen_t slen = sizeof(ss);
> +	char addrbuf[256], portbuf[6] = "";
> +
> +	if (!freopen("/dev/null", "w", stderr))
> +		die_errno("failed to redirect stderr to /dev/null");
> +
> +	/*
> +	 * Windows is said to not be able to handle this, so we will simply
> +	 * ignore failure here. (It only affects a log message anyway.)
> +	 */
> +	if (getpeername(0, addr, &slen) < 0)
> +		return;
> +
> +	if (addr->sa_family == AF_INET) {
> +		const struct sockaddr_in *sin_addr = (void *)addr;
> +
> +		if (inet_ntop(addr->sa_family, &sin_addr->sin_addr,
> +			      addrbuf, sizeof(addrbuf)) == NULL)
> +			return;
> +		snprintf(portbuf, sizeof(portbuf), "%hu",
> +			 ntohs(sin_addr->sin_port));
> +#ifndef NO_IPV6
> +	} else if (addr->sa_family == AF_INET6) {
> +		const struct sockaddr_in6 *sin6_addr = (void *)addr;
> +
> +		addrbuf[0] = '[';
> +		addrbuf[1] = '\0';
> +		if (inet_ntop(AF_INET6, &sin6_addr->sin6_addr, addrbuf + 1,
> +			      sizeof(addrbuf) - 2) == NULL)
> +			return;
> +		strcat(addrbuf, "]");
> +
> +		snprintf(portbuf, sizeof(portbuf), "%hu",
> +			 ntohs(sin6_addr->sin6_port));
> +#endif
> +	} else {
> +		snprintf(addrbuf, sizeof(addrbuf), "<AF %d>",
> +			 addr->sa_family);
> +	}
> +	if (setenv("REMOTE_ADDR", addrbuf, true) < 0)
> +		return;
> +	setenv("REMOTE_PORT", portbuf, true);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	int listen_port = 0;
> @@ -1341,10 +1390,8 @@ int main(int argc, char **argv)
>  		die("base-path '%s' does not exist or is not a directory",
>  		    base_path);
>  
> -	if (inetd_mode) {
> -		if (!freopen("/dev/null", "w", stderr))
> -			die_errno("failed to redirect stderr to /dev/null");
> -	}
> +	if (inetd_mode)
> +		inetd_mode_prepare();
>  
>  	if (inetd_mode || serve_mode)
>  		return execute();

Re: [PATCH] daemon: restore getpeername(0,...) use

[Junio C Hamano]

"Joachim Schmitz" <jojo@schmitz-digital.de> writes:

>> + setenv("REMOTE_PORT", portbuf, true);
>
> setenv() is not a function available on all plattfomrs.

Please do some homework before adding irrelevant noise.  At the
minimum, run "git grep" to see if we already use it in other places,
and investigate why we can use it safely across platforms we already
support.

Re: Restore hostname logging in inetd mode

[Jan Engelhardt]

On Saturday 2012-09-08 20:57, Junio C Hamano wrote:

>Please don't throw a pull request for a patch whose worth hasn't
>been justified in a discussion on the list.  Thanks.

Let me postulate that people like to get cover letters with the
git:// URL so they can fetch+look at it, a diffstat and shortlog.

And 'lo, that is exactly what git-request-pull thankfully generates.

In my defense: Just because the command is called "request-pull",
does not mean you absolutely have to merge/pull it. In fact, it does
not even mention merge/pull at all.

"
The following changes since commit [SHA]:
  [Commit Message]
are available in the git repository at:
  git://[...]
for you to fetch changes up to [SHA]:
  [Commit Message]
[diffstat,shortlog]
"

In contrast to many a LKML postings which explicitly state the pull
intent:

"
Hi [Maintainer],

Please pull from the git repository at
  [URL]
to receive [...]
  [SHA]
  [Commit Message]
on top of commit [SHA]
  [Commit Message]
[diffstat,shortlog]
"

RE: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

> From: Junio C Hamano [mailto:gitster@pobox.com]
> Sent: Saturday, September 08, 2012 9:04 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] daemon: restore getpeername(0,...) use
> 
> "Joachim Schmitz" <jojo@schmitz-digital.de> writes:
> 
> >> + setenv("REMOTE_PORT", portbuf, true);
> >
> > setenv() is not a function available on all plattfomrs.
> 
> Please do some homework before adding irrelevant noise.  At the
> minimum, run "git grep" to see if we already use it in other places,
> and investigate why we can use it safely across platforms we already
> support.

Hmm, guess I missed the indirect inclusion of git-compat-util.h
And didn't know about 'git grep', so thanks for the hint

Will look closer next time ;-)

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jan Engelhardt]

On Saturday 2012-09-08 20:59, Junio C Hamano wrote:
>> diff --git a/daemon.c b/daemon.c
>> index 4602b46..eaf08c2 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -1,3 +1,4 @@
>> +#include <stdbool.h>
>>  #include "cache.h"
>>  #include "pkt-line.h"
>>  #include "exec_cmd.h"
>
>Platform agnostic parts of the code that use "git-compat-util.h"
>(users of "cache.h" are indirectly users of it) are not allowed to
>do platform specific include like this at their beginning.
>
>This is the first use of stdbool.h; what do you need it for?

For the use in setenv(,,true). It was not entirely obvious in which .h 
to add it; the most reasonable place was daemon.c itself, since the 
other .c files do not seem to need it.

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jeff King]

On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote:

> On Saturday 2012-09-08 20:59, Junio C Hamano wrote:
> >> diff --git a/daemon.c b/daemon.c
> >> index 4602b46..eaf08c2 100644
> >> --- a/daemon.c
> >> +++ b/daemon.c
> >> @@ -1,3 +1,4 @@
> >> +#include <stdbool.h>
> >>  #include "cache.h"
> >>  #include "pkt-line.h"
> >>  #include "exec_cmd.h"
> >
> >Platform agnostic parts of the code that use "git-compat-util.h"
> >(users of "cache.h" are indirectly users of it) are not allowed to
> >do platform specific include like this at their beginning.
> >
> >This is the first use of stdbool.h; what do you need it for?
> 
> For the use in setenv(,,true). It was not entirely obvious in which .h 
> to add it; the most reasonable place was daemon.c itself, since the 
> other .c files do not seem to need it.

It would go in git-compat-util.h. However, it really is not needed; you
can simply pass "1" to setenv, as every other callsite in git does.

More importantly, though, is it actually portable? I thought it was
added in C99, and we try to stick to C89 to support older compilers and
systems. My copy of C99 is vague (it says only that the "bool" macro was
added via stdbool.h in C99, but nothing about the "true" and "false"
macros), and I don't have a copy of C89 handy.  Wikipedia does claim the
header wasn't standardized at all until C99:

  https://en.wikipedia.org/wiki/C_standard_library

-Peff

Re: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

Jeff King wrote:
> On Sat, Sep 08, 2012 at 09:20:48PM +0200, Jan Engelhardt wrote:
>
>> On Saturday 2012-09-08 20:59, Junio C Hamano wrote:
>>>> diff --git a/daemon.c b/daemon.c
>>>> index 4602b46..eaf08c2 100644
>>>> --- a/daemon.c
>>>> +++ b/daemon.c
>>>> @@ -1,3 +1,4 @@
>>>> +#include <stdbool.h>
>>>>  #include "cache.h"
>>>>  #include "pkt-line.h"
>>>>  #include "exec_cmd.h"
>>>
>>> Platform agnostic parts of the code that use "git-compat-util.h"
>>> (users of "cache.h" are indirectly users of it) are not allowed to
>>> do platform specific include like this at their beginning.
>>>
>>> This is the first use of stdbool.h; what do you need it for?
>>
>> For the use in setenv(,,true). It was not entirely obvious in which
>> .h to add it; the most reasonable place was daemon.c itself, since
>> the other .c files do not seem to need it.
>
> It would go in git-compat-util.h. However, it really is not needed;
> you can simply pass "1" to setenv, as every other callsite in git
> does.
>
> More importantly, though, is it actually portable? I thought it was
> added in C99, and we try to stick to C89 to support older compilers
> and systems. My copy of C99 is vague (it says only that the "bool"
> macro was added via stdbool.h in C99, but nothing about the "true"
> and "false" macros), and I don't have a copy of C89 handy.  Wikipedia
> does claim the header wasn't standardized at all until C99:
>
>  https://en.wikipedia.org/wiki/C_standard_library

Indeed stdbool is not part of C89, but inline isn't either and used 
extensively in git (could possible be defined away), as are non-const array 
intializers, e.g.:


                const char *args[] = { editor, path, NULL };
                                               ^
".../git/editor.c", line 39: error(122): expression must have a constant 
value

So git source is not plain C89 code (anymore?)

Bye, Jojo 

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jeff King]

On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:

> >More importantly, though, is it actually portable? I thought it was
> >added in C99, and we try to stick to C89 to support older compilers
> >and systems. My copy of C99 is vague (it says only that the "bool"
> >macro was added via stdbool.h in C99, but nothing about the "true"
> >and "false" macros), and I don't have a copy of C89 handy.  Wikipedia
> >does claim the header wasn't standardized at all until C99:
> >
> > https://en.wikipedia.org/wiki/C_standard_library
> 
> Indeed stdbool is not part of C89, but inline isn't either and used
> extensively in git (could possible be defined away),

You can define INLINE in the Makefile to disable it (or set it to
something more appropriate for your system).

> as are non-const array intializers, e.g.:
> 
>                const char *args[] = { editor, path, NULL };
>                                               ^
> ".../git/editor.c", line 39: error(122): expression must have a
> constant value
>
> So git source is not plain C89 code (anymore?)

I remember we excised a whole bunch of non-constant initializers at some
point because somebody's compiler was complaining. But I suppose this
one has slipped back in, because non-constant initializers are so damn
useful. And nobody has complained, which I imagine means nobody has
bothered building lately on those older systems that complained.

My "we stick to C89" is a little bit of a lie. We do not care about
specific standards. We do care about running everywhere on reasonable
systems. So something that is C99 might be OK if realistically everybody
has it. And something that is POSIX is not automatically OK if there are
many real-world systems that do not have it.

Since there is no written standard, there tends to be an organic ebb and
flow in which features we use. Somebody will use a feature that is not
portable because it's useful to them, and then somebody whose system
will no longer build git will complain, and then we'll fix it and move
on. As reviewers, we try to anticipate those breakages and stop them
early (especially if they are ones we have seen before and know are a
problem), but we do not always get it right. And sometimes it is even
time to revisit old decisions (the line you mentioned is 2 years old,
and nobody has complained; maybe all of the old systems have become
obsolete, and we no longer need to care about constant initializers).

Getting back to the patch at hand, it may be that stdbool is practically
available everywhere. Or that we could trivially emulate it by defining
a "true" macro in this case. But it is also important to consider
whether that complexity is worth it. This would be the first and only
spot in git to use "true". Why bother?

-Peff

Re: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

Jeff King wrote:
> On Mon, Sep 10, 2012 at 04:38:58PM +0200, Joachim Schmitz wrote:
>
>>> More importantly, though, is it actually portable? I thought it was
>>> added in C99, and we try to stick to C89 to support older compilers
>>> and systems. My copy of C99 is vague (it says only that the "bool"
>>> macro was added via stdbool.h in C99, but nothing about the "true"
>>> and "false" macros), and I don't have a copy of C89 handy.
>>> Wikipedia does claim the header wasn't standardized at all until
>>> C99:
>>>
>>> https://en.wikipedia.org/wiki/C_standard_library
>>
>> Indeed stdbool is not part of C89, but inline isn't either and used
>> extensively in git (could possible be defined away),
>
> You can define INLINE in the Makefile to disable it (or set it to
> something more appropriate for your system).

That's what I meant.

>> as are non-const array intializers, e.g.:
>>
>>                const char *args[] = { editor, path, NULL };
>>                                               ^
>> ".../git/editor.c", line 39: error(122): expression must have a
>> constant value
>>
>> So git source is not plain C89 code (anymore?)
>
> I remember we excised a whole bunch of non-constant initializers at
> some point because somebody's compiler was complaining. But I suppose
> this one has slipped back in, because non-constant initializers are
> so damn useful. And nobody has complained, which I imagine means
> nobody has bothered building lately on those older systems that
> complained.

OK, record my complaint then ;-)
At least some older release of HP NonStop only have C89 and are still in use

And tying to compile in plain C89 mode revealed several other problems too 
(e.g. size_t seems not to be typedef'd?)

> My "we stick to C89" is a little bit of a lie. We do not care about
> specific standards. We do care about running everywhere on reasonable
> systems. So something that is C99 might be OK if realistically
> everybody has it. And something that is POSIX is not automatically OK
> if there are many real-world systems that do not have it.
>
> Since there is no written standard, there tends to be an organic ebb
> and flow in which features we use. Somebody will use a feature that
> is not portable because it's useful to them, and then somebody whose
> system will no longer build git will complain, and then we'll fix it
> and move on. As reviewers, we try to anticipate those breakages and
> stop them early (especially if they are ones we have seen before and
> know are a problem), but we do not always get it right. And sometimes
> it is even time to revisit old decisions (the line you mentioned is 2
> years old, and nobody has complained; maybe all of the old systems
> have become obsolete, and we no longer need to care about constant
> initializers).
>
> Getting back to the patch at hand, it may be that stdbool is
> practically available everywhere. Or that we could trivially emulate
> it by defining a "true" macro in this case. But it is also important
> to consider whether that complexity is worth it. This would be the
> first and only spot in git to use "true". Why bother?
>
> -Peff 

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jeff King]

On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:

> >>as are non-const array intializers, e.g.:
> >>
> >>               const char *args[] = { editor, path, NULL };
> >>                                              ^
> >>".../git/editor.c", line 39: error(122): expression must have a
> >>constant value
> >>
> >>So git source is not plain C89 code (anymore?)
> >
> >I remember we excised a whole bunch of non-constant initializers at
> >some point because somebody's compiler was complaining. But I suppose
> >this one has slipped back in, because non-constant initializers are
> >so damn useful. And nobody has complained, which I imagine means
> >nobody has bothered building lately on those older systems that
> >complained.
> 
> OK, record my complaint then ;-)

Oops, did I say "complained"? I meant "sent patches". Hint, hint. :)

> At least some older release of HP NonStop only have C89 and are still in use
> 
> And tying to compile in plain C89 mode revealed several other
> problems too (e.g. size_t seems not to be typedef'd?)

I think it is a mistake to set -std=c89 (or whatever similar option your
compiler supports). Like I said, we are not interested in being strictly
C89-compliant. We are interested in working on real-world systems.

If your compiler complains in the default mode (or when it is given some
reasonable practical settings), then that's something worth fixing. But
if your compiler is perfectly capable of compiling git, but you choose
to cripple it by telling it to be pedantic about a standard, then that
is not git's problem at all.

-Peff

RE: [PATCH] daemon: restore getpeername(0,...) use

[Joachim Schmitz]

> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, September 10, 2012 7:59 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] daemon: restore getpeername(0,...) use
> 
> On Mon, Sep 10, 2012 at 07:26:26PM +0200, Joachim Schmitz wrote:
> 
> > >>as are non-const array intializers, e.g.:
> > >>
> > >>               const char *args[] = { editor, path, NULL };
> > >>                                              ^
> > >>".../git/editor.c", line 39: error(122): expression must have a
> > >>constant value
> > >>
> > >>So git source is not plain C89 code (anymore?)
> > >
> > >I remember we excised a whole bunch of non-constant initializers at
> > >some point because somebody's compiler was complaining. But I suppose
> > >this one has slipped back in, because non-constant initializers are
> > >so damn useful. And nobody has complained, which I imagine means
> > >nobody has bothered building lately on those older systems that
> > >complained.
> >
> > OK, record my complaint then ;-)
> 
> Oops, did I say "complained"? I meant "sent patches". Hint, hint. :)

Oops ;-)
 
> > At least some older release of HP NonStop only have C89 and are still in use
> >
> > And tying to compile in plain C89 mode revealed several other
> > problems too (e.g. size_t seems not to be typedef'd?)
> 
> I think it is a mistake to set -std=c89 (or whatever similar option your
> compiler supports). Like I said, we are not interested in being strictly
> C89-compliant. We are interested in working on real-world systems.
> 
> If your compiler complains in the default mode (or when it is given some
> reasonable practical settings), then that's something worth fixing. But
> if your compiler is perfectly capable of compiling git, but you choose
> to cripple it by telling it to be pedantic about a standard, then that
> is not git's problem at all.

Older 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.

Bye, Jojo

Re: [PATCH] daemon: restore getpeername(0,...) use

[Jeff King]

On Mon, Sep 10, 2012 at 08:27:07PM +0200, Joachim Schmitz wrote:

> > I think it is a mistake to set -std=c89 (or whatever similar option your
> > compiler supports). Like I said, we are not interested in being strictly
> > C89-compliant. We are interested in working on real-world systems.
> > 
> > If your compiler complains in the default mode (or when it is given some
> > reasonable practical settings), then that's something worth fixing. But
> > if your compiler is perfectly capable of compiling git, but you choose
> > to cripple it by telling it to be pedantic about a standard, then that
> > is not git's problem at all.
> 
> Older 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.

If you are using a compiler switch to emulate a real environment, then
my comments above do not apply. I was speaking against standard pedantry
for its own sake, which I have no interest in.

However, do be careful that your emulated environment (i.e., recent
NonStop but using compiler flags to pretend you are the older version)
is accurate, and not introducing new portability annoyances that do not
truly exist on the old system. In fact, I might go so far as to say if
you cannot actually come up with an instance of the older platform to
test it, it might not even be worth our time to care about.

-Peff