threads / discuss / 6253

problem with git clone on cygwin

Subject: problem with git clone on cygwin

## tl;dr

12 messages between Jan 6, 2007 and Jan 7, 2007.

replies: 11people: 4as markdown or json

Stefan-W. Hahn· Jan 6, 2007, 17:03 UTC · lore
Hi,

running git on Cygwin I have a problem with git clone on local disk, while packing data.

The problem comes with v1.5.0-rc0. I bisected the problem down to commit 6d2fa7 as the first bad commit.

It seems to be a problem with cygwin.dll prior v1.5.22 and pread(), if using an offset!=0. (I'm running cygwin.dll v1.5.21 build date 2006-07-27 and I can't update because of other compatibility problems).

So I tried:
- not to set NO_MMAP to use real mmap
- changing get_data_from_pack() from index-pack.c to used mmap() as
  in 042aea8. (I did this because it directly uses pread().)
This solved the problem for my testcase.

The added testcase also succeeds on Linux but just with v1.4.4 or my patch on Cygwin.

Is there anybody else having the same problem in git universe?
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			



diff --git a/Makefile b/Makefile
index 180e1e0..afa5d08 100644
--- a/Makefile
+++ b/Makefile
@@ -368,7 +368,7 @@ ifeq ($(uname_O),Cygwin)
 	# There are conflicting reports about this.
 	# On some boxes NO_MMAP is needed, and not so elsewhere.
 	# Try commenting this out if you suspect MMAP is more efficient
-	NO_MMAP = YesPlease
+	#NO_MMAP = YesPlease
 	NO_IPV6 = YesPlease
 	X = .exe
 endif
diff --git a/index-pack.c b/index-pack.c
index 5f6d128..f1d11a0 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -277,25 +277,27 @@ static void *get_data_from_pack(struct object_entry *obj)
 {
 	unsigned long from = obj[0].offset + obj[0].hdr_size;
 	unsigned long len = obj[1].offset - from;
-	unsigned char *src, *data;
+	unsigned pg_offset = from % getpagesize();
+	unsigned char *map, *data;
 	z_stream stream;
 	int st;
 
-	src = xmalloc(len);
-	if (pread(pack_fd, src, len, from) != len)
-		die("cannot pread pack file: %s", strerror(errno));
+	map = mmap(NULL, len + pg_offset, PROT_READ, MAP_PRIVATE,
+		   pack_fd, from - pg_offset);
+	if (map == MAP_FAILED)
+		die("cannot mmap pack file: %s", strerror(errno));
 	data = xmalloc(obj->size);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = data;
 	stream.avail_out = obj->size;
-	stream.next_in = src;
+	stream.next_in = map + pg_offset;
 	stream.avail_in = len;
 	inflateInit(&stream);
 	while ((st = inflate(&stream, Z_FINISH)) == Z_OK);
 	inflateEnd(&stream);
 	if (st != Z_STREAM_END || stream.total_out != obj->size)
 		die("serious inflate inconsistency");
-	free(src);
+	munmap(map, len + pg_offset);
 	return data;
 }
 
diff --git a/t/t5610-clone-fail.sh b/t/t5610-clone-fail.sh
new file mode 100755
index 0000000..d46c255
--- /dev/null
+++ b/t/t5610-clone-fail.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+
+test_description='test git-clone failure on cygwin using pread()
+'
+
+. ./test-lib.sh
+
+# Need a repo to clone
+test_create_repo foo2
+
+GIT_AUTHOR_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy
+GIT_COMMITTER_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy
+export GIT_AUTHOR_EMAIL
+export GIT_COMMITTER_EMAIL
+
+(cd foo2 && echo "Hello" > file && git add file && git commit -m 'add file' >/dev/null 2>&1)
+(cd foo2 && echo "Hello2" >> file && git commit -a -m 'test' >/dev/null 2>&1)
+
+test_expect_success \
+    'clone with resolving' \
+    'git-clone foo2 bar2'
+
+test_done
Juergen Ruehle· Jan 6, 2007, 21:40 UTC · re: Stefan-W. Hahn · lore

Re: problem with git clone on cygwin

Stefan-W. Hahn writes:
 > It seems to be a problem with cygwin.dll prior v1.5.22 and pread(), if
 > using an offset!=0. (I'm running cygwin.dll v1.5.21 build date
 > 2006-07-27 and I can't update because of other compatibility problems).

You should compile a cygwin.dll from 1.5.21 sources with the pread patch applied.

Could you enumerate the other compatability problems?
Shawn O. Pearce· Jan 7, 2007, 06:00 UTC · re: Stefan-W. Hahn · lore

Re: problem with git clone on cygwin

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> wrote:
Show 5 quoted lines
> running git on Cygwin I have a problem with git clone on local disk,
> while packing data. 
> 
> The problem comes with v1.5.0-rc0. I bisected the problem down to
> commit 6d2fa7 as the first bad commit.
That was a performance improvement for Mac OS X to reduce the
index-pack time on linux.git from almost an hour to about 1 minute.
 
> It seems to be a problem with cygwin.dll prior v1.5.22 and pread(), if
> using an offset!=0. (I'm running cygwin.dll v1.5.21 build date
> 2006-07-27 and I can't update because of other compatibility problems).
Why can't you upgrade Cygwin?
 
> So I tried:
> - not to set NO_MMAP to use real mmap
FYI: you can "unset" NO_MMAP at compile time:
  make NO_MMAP=
or by putting it into your personal config.mak:
  echo NO_MMAP= >config.mak
  make

this way you always build with the real mmap, but don't need to locally patch Makefile.

> - changing get_data_from_pack() from index-pack.c to used mmap() as
>   in 042aea8. (I did this because it directly uses pread().)
> This solved the problem for my testcase.

This isn't ideal because of the performance problems that some OSes have with small but frequent mmap() calls.

Perhaps we should add our own fake pread (aka git_pread) to git-compat-util.h and provide a NO_PREAD option in the Makefile? Though I don't think its really worth it, as pretty much every OS we run on should have a functioning pread call. Except some Cygwins apparently.

-- 
Shawn.
Stefan-W. Hahn· Jan 7, 2007, 11:17 UTC · re: Shawn O. Pearce · lore

[PATCH] Replacing the system call pread() with real mmap().

Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy. This patch introduces NO_PREAD. If NO_MMAP is not set and NO_PREAD is set git uses real mmap instead of pread.

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
I made a patch for this problem replacing pread().
 Makefile          |    9 +++++++++
 compat/pread.c    |   16 ++++++++++++++++
 git-compat-util.h |    5 +++++
 3 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 180e1e0..4f928fd 100644
--- a/Makefile
+++ b/Makefile
@@ -69,6 +69,10 @@ all:
 #
 # Define NO_MMAP if you want to avoid mmap.
 #
+# Define NO_PREAD if you have a problem with pread() system call (i.e.
+# cygwin.dll before v1.5.22). This needs NO_MMAP not to be set, because pread()
+# is emulated using mmap.
+#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -521,6 +525,11 @@ endif
 ifdef NO_MMAP
 	COMPAT_CFLAGS += -DNO_MMAP
 	COMPAT_OBJS += compat/mmap.o
+else
+ifdef NO_PREAD
+	COMPAT_CFLAGS += -DNO_PREAD
+	COMPAT_OBJS += compat/pread.o
+endif
 endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
diff --git a/compat/pread.c b/compat/pread.c
new file mode 100644
index 0000000..23e775d
--- /dev/null
+++ b/compat/pread.c
@@ -0,0 +1,16 @@
+#include "../git-compat-util.h"
+
+ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)
+{
+        unsigned pg_offset = offset % getpagesize();
+        unsigned char *map;
+
+        map = mmap(NULL, count + pg_offset, PROT_READ, MAP_PRIVATE,
+                   fd, offset - pg_offset);
+        if (map == MAP_FAILED)
+                return -1;
+
+        memcpy(buf, map + pg_offset, count);
+        munmap(map, count + pg_offset);
+        return count;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 5d9eb26..413e7e8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -96,6 +96,11 @@ extern int git_munmap(void *start, size_t length);
 
 #include <sys/mman.h>
 
+#ifdef NO_PREAD
+#define pread git_pread
+extern ssize_t git_pread(int fd,void *buf,size_t count,off_t offset);
+#endif
+
 #endif /* NO_MMAP */
 
 #ifdef NO_SETENV
-- 
1.5.0.rc0.g244a7
Shawn O. Pearce· Jan 7, 2007, 11:24 UTC · re: Stefan-W. Hahn · lore

Re: [PATCH] Replacing the system call pread() with real mmap().

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> wrote:
> +# Define NO_PREAD if you have a problem with pread() system call (i.e.
> +# cygwin.dll before v1.5.22). This needs NO_MMAP not to be set, because pread()
> +# is emulated using mmap.
Better to emulate pread using lseek/xread/lseek:
	off_t p = lseek(fd, offset, SEEK_SET);
	xread(fd, buf, count);
	lseek(fd, p, SEEK_SET);

If you look at the history for contrib/mmap.c you will see an implementation like that before it was called git_mmap().

This works OK in Git as we only ever have one thread, therefore there is no race condition during the seek/read/seek calls. Its also usually faster on platforms like Mac OS X where mmap() is horrible. If the platform is lacking a good pread() it may also be the case that it lacks a good mmap(); in this case it would be desirable to set both NO_MMAP and NO_PREAD.

-- 
Shawn.
Stefan-W. Hahn· Jan 7, 2007, 16:36 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Replacing the system call pread() with real mmap().

Also sprach Shawn O. Pearce am Sun, 07 Jan 2007 at 06:24:45 -0500:
Show 8 quoted lines
> Better to emulate pread using lseek/xread/lseek:
> 
> 	off_t p = lseek(fd, offset, SEEK_SET);
> 	xread(fd, buf, count);
> 	lseek(fd, p, SEEK_SET);
> 
> If you look at the history for contrib/mmap.c you will see an
> implementation like that before it was called git_mmap().
This doesn't work;I tried it before.
Stefan
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			
Johannes Schindelin· Jan 7, 2007, 17:10 UTC · re: Stefan-W. Hahn · lore

Re: [PATCH] Replacing the system call pread() with real mmap().

Hi,
On Sun, 7 Jan 2007, Stefan-W. Hahn wrote:
Show 11 quoted lines
> Also sprach Shawn O. Pearce am Sun, 07 Jan 2007 at 06:24:45 -0500:
> > Better to emulate pread using lseek/xread/lseek:
> > 
> > 	off_t p = lseek(fd, offset, SEEK_SET);
> > 	xread(fd, buf, count);
> > 	lseek(fd, p, SEEK_SET);
> > 
> > If you look at the history for contrib/mmap.c you will see an
> > implementation like that before it was called git_mmap().
> 
> This doesn't work;I tried it before.

Care to elaborate? It worked pretty well on _all_ cygwin setups I tested with.

Ciao, Dscho

Stefan-W. Hahn· Jan 7, 2007, 20:01 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Replacing the system call pread() with real mmap().

Also sprach Johannes Schindelin am Sun, 07 Jan 2007 at 18:10:55 +0100:
> Care to elaborate? It worked pretty well on _all_ cygwin setups I tested 
> with.
Yes, but not today.

But after a first look the received pack file is corrupt. It doesn't start with the pack header.

Stefan
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			
Stefan-W. Hahn· Jan 7, 2007, 11:18 UTC · re: Shawn O. Pearce · lore

[PATCH] Test for failing pread() on cygwin.

Test for failing pread() on cygwin.
Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
And the test which fails on cygwin with 1.5.21 and original pread().
 t/t5610-clone-fail.sh |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/t/t5610-clone-fail.sh b/t/t5610-clone-fail.sh
new file mode 100644
index 0000000..d30b25e
--- /dev/null
+++ b/t/t5610-clone-fail.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+#
+# Copyright (C) 2007 Stefan-W. Hahn <stefan.hahn@s-hahn.de>
+#
+
+test_description='test git-clone failure on cygwin using pread()
+
+This test covers the fact that git-clone fails, if pread() does not
+work properly, because the size of objects makes it neccessary to
+read data from a not zero offset.'
+
+. ./test-lib.sh
+
+# Need a repo to clone
+test_create_repo foo2
+
+GIT_AUTHOR_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy
+GIT_COMMITTER_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy
+export GIT_AUTHOR_EMAIL
+export GIT_COMMITTER_EMAIL
+
+(cd foo2 && echo "Hello" > file && git add file && git commit -m 'add file' >/dev/null 2>&1)
+(cd foo2 && echo "Hello2" >> file && git commit -a -m 'test' >/dev/null 2>&1)
+
+test_expect_success \
+    'clone with resolving' \
+    'git-clone foo2 bar2'
+
+test_done
-- 
1.5.0.rc0.g244a7
Stefan-W. Hahn· Jan 7, 2007, 09:30 UTC · lore

Re: problem with git clone on cygwin

Also sprach Stefan-W. Hahn am Sat, 06 Jan 2007 at 22:59:19 +0100:
Show 17 quoted lines
> Also sprach Juergen Ruehle am Sat, 06 Jan 2007 at 22:40:52 +0100:
> > You should compile a cygwin.dll from 1.5.21 sources with the pread
> > patch applied.
> 
> Thanks for the info.
> 
> > Could you enumerate the other compatability problems?
> 
> The problem that one cannot use one version of cygwin.dll and
> simultaniously another (incomatible) version. (Having a local
> installed cygwin window open and trying to build in a branch where
> another version is used.)
> 
> -- 
> Stefan-W. Hahn                          It is easy to make things.
> / mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			
> 
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			
Juergen Ruehle· Jan 7, 2007, 10:46 UTC · re: Stefan-W. Hahn · lore

Re: problem with git clone on cygwin

Stefan-W. Hahn writes:
 > Also sprach Juergen Ruehle am Sat, 06 Jan 2007 at 22:40:52 +0100:
 > > Could you enumerate the other compatability problems?
 > 
 > The problem that one cannot use one version of cygwin.dll and
 > simultaniously another (incomatible) version. (Having a local
 > installed cygwin window open and trying to build in a branch where
 > another version is used.)

Yes that is an unfortunate (though probably unavoidable) downside of cygwin. But my question was meant more along the lines of 'could you give some examples of applications that do not work with 1.5.22 or later and prevent you from upgrading your installed cygwin version'. If there are serious showstoppers it would probably be a good idea to go the route Shawn outlined to retain compatibility with earlier cygwin releases. Otherwise we'll just keep recommending an upgrade:-)

Sorry for being unclear.
Stefan-W. Hahn· Jan 7, 2007, 11:21 UTC · re: Juergen Ruehle · lore

Re: problem with git clone on cygwin

Also sprach Juergen Ruehle am Sun, 07 Jan 2007 at 11:46:45 +0100:
> Stefan-W. Hahn writes:
> cygwin. But my question was meant more along the lines of 'could you
> give some examples of applications that do not work with 1.5.22 or
> later and prevent you from upgrading your installed cygwin version'.

Non, its just the above metioned incompatibility between the cygwin versions.

> If there are serious showstoppers it would probably be a good idea to
> go the route Shawn outlined to retain compatibility with earlier
> cygwin releases. Otherwise we'll just keep recommending an upgrade:-)
I sent out a patch using mmap() instead of pread() if configured.
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

← back to recent threads