Re: [PATCH] index-pack usage of mmap() is unacceptably slower on many OSes other than Linux

Nicolas Pitre [off-list ref] writes:

OK looks like this has been sorted out while I was away.  Good!

This is Linus's patch plus a few cosmetic changes.

Not a complaint but rather a request for free education ;-).

Most of the rest of the sources seem to do our includes first
and source-file specific system includes at the end.  What's the
rationale for this change?

Do we need _XOPEN_SOURCE=500 because pread() is XSI?

Also nobody other than convert-objects.c has _XOPEN_SOURCE level
specified.  If _XOPEN_SOURCE matters I wonder if we should do so
in some central place to make it consistent across source files?

quoted

quoted

quoted

quoted

"Linus" == Linus Torvalds [off-list ref] writes:

Linus> May I actually suggest we handle _all_ of these issues in one central 
Linus> place, namely "git-compat-util.h"

I can't remember now, but a couple of patches I had to submit were because
sys/types.h was included either too early or too late on OSX, so let's be sure
to get that right.  Surely, my patch can be observed somewhere, perhaps in a
git repository. :)

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
[off-list ref] <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.

Linus Torvalds wrote:

If you are going to do this, you have to audit -every- file, to make 
sure git-compat-util.h is -always- the first header.

For example, builtin-mailinfo.c includes git-compat-util.h after ctype.h 
and iconv.h, which renders your #define _XOPEN_SOURCE 600 useless. 
/usr/include/features.h has already been included at that point.

	Jeff

quoted

quoted

quoted

quoted

"Randal" == Randal L Schwartz [off-list ref] writes:

Randal> I can't remember now, but a couple of patches I had to submit were
Randal> because sys/types.h was included either too early or too late on OSX,
Randal> so let's be sure to get that right.  Surely, my patch can be observed
Randal> somewhere, perhaps in a git repository. :)

Here's one that might be relevant for OSX:

        979e32fa1483a32faa4ec331e29b357e5eb5ef25

And this is an ordering issue for OpenBSD:

        ed1795fcc5f2aa3f105630429bcbed49c50053fa


-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
[off-list ref] <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.

On Tue, 19 Dec 2006, Junio C Hamano wrote:

quoted

diff --git a/index-pack.c b/index-pack.c
index 6d6c92b..e08a687 100644
--- a/index-pack.c
+++ b/index-pack.c

@@ -1,3 +1,8 @@
+#define _XOPEN_SOURCE 500
+#include <unistd.h>
+#include <sys/time.h>
+#include <signal.h>
+
 #include "cache.h"
 #include "delta.h"
 #include "pack.h"

@@ -6,8 +11,6 @@
 #include "commit.h"
 #include "tag.h"
 #include "tree.h"
-#include <sys/time.h>
-#include <signal.h>

Most of the rest of the sources seem to do our includes first
and source-file specific system includes at the end.  What's the
rationale for this change?

Because _XOPEN_SOURCE must be defined before including unistd.h 
otherwise pread is not declared and a warning is issued.

Do we need _XOPEN_SOURCE=500 because pread() is XSI?

The pread man page says Unix98.

Also nobody other than convert-objects.c has _XOPEN_SOURCE level
specified.  If _XOPEN_SOURCE matters I wonder if we should do so
in some central place to make it consistent across source files?

Your call I guess.

On Tue, 19 Dec 2006, Nicolas Pitre wrote:

Because _XOPEN_SOURCE must be defined before including unistd.h 
otherwise pread is not declared and a warning is issued.

May I actually suggest we handle _all_ of these issues in one central 
place, namely "git-compat-util.h"

It's nice to have just one single file that tries to hide the details of 
all the differences between systems.

Sure, that file ends up having to include a lot of standard header files 
that some of the .c files don't actually _need_, but git compiles 
reasonably quickly, so I don't think we need to try to optimize compile 
speed much.

It's the C++ people who tend to have sucky compile times.

So how about something like the appended? And then just have the rule that 
we try to include "cache.h" early, because that brings in ALL the really 
basic system header files?

		Linus

---

diff --git a/convert-objects.c b/convert-objects.c
index 8812583..a630132 100644
--- a/convert-objects.c
+++ b/convert-objects.c

@@ -1,7 +1,3 @@
-#define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */
-#define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
-#define _GNU_SOURCE
-#include <time.h>
 #include "cache.h"
 #include "blob.h"
 #include "commit.h"

diff --git a/git-compat-util.h b/git-compat-util.h
index 0272d04..e619e29 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -11,6 +11,10 @@
 
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
+#define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */
+#define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */
+#define _GNU_SOURCE
+
 #include <unistd.h>
 #include <stdio.h>
 #include <sys/stat.h>

@@ -25,6 +29,10 @@
 #include <netinet/in.h>
 #include <sys/types.h>
 #include <dirent.h>
+#include <sys/time.h>
+#include <time.h>
+#include <signal.h>
+#include <sys/wait.h>
 
 /* On most systems <limits.h> would have given us this, but

On 12/19/06, Linus Torvalds [off-list ref] wrote:

It's the C++ people who tend to have sucky compile times.

Always looking for a way to bash on C++, eh?