problem with git clone on cygwin

problem with git clone on cygwin

[Stefan-W. Hahn]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

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.			


[-- Attachment #2: ppp.diff --]
[-- Type: text/plain, Size: 2341 bytes --]

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

Re: problem with git clone on cygwin

[Juergen Ruehle]

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?

Re: problem with git clone on cygwin

[Shawn O. Pearce]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> wrote:
> 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.

Re: problem with git clone on cygwin

[Stefan-W. Hahn]

Also sprach Stefan-W. Hahn am Sat, 06 Jan 2007 at 22:59:19 +0100:
> 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.			

Re: problem with git clone on cygwin

[Juergen Ruehle]

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.

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

[Stefan-W. Hahn]

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

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

[Stefan-W. Hahn]

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

Re: problem with git clone on cygwin

[Stefan-W. Hahn]

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.			

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

[Shawn O. Pearce]

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

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

[Stefan-W. Hahn]

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.

Stefan
-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

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

[Johannes Schindelin]

Hi,

On Sun, 7 Jan 2007, Stefan-W. Hahn wrote:

> 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

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

[Stefan-W. Hahn]

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.			

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

[Horst H. von Brand]

Stefan-W. Hahn <stefan.hahn@s-hahn.de> wrote:

> Test for failing pread() on cygwin.
> 
> Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
> ---
> 

[...]

> +GIT_AUTHOR_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy

Why not the standard "Random J. User" <rju@example.com>? ;-)
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239
Casilla 110-V, Valparaiso, Chile               Fax:  +56 32 2797513

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

[Stefan-W. Hahn]

Also sprach Horst H. von Brand am Sun, 07 Jan 2007 at 21:19:32 -0300:

> > +GIT_AUTHOR_EMAIL=xxxxxxxx@yyyyyyyy.yyyyy.yyyyyyy.yyy
> 
> Why not the standard "Random J. User" <rju@example.com>? ;-)

Just, because the length of the strings plays a role. With the strings
defined in test_lib there were no errors.

Stefan

-- 
Stefan-W. Hahn                          It is easy to make things.
/ mailto:stefan.hahn@s-hahn.de /        It is hard to make things simple.			

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

[Stefan-W. Hahn]

From: Stefan-W. Hahn <stefan.hahn@s-hahn.de>


Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 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 100755
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.4.4.4.g46aa

[PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.

[Stefan-W. Hahn]

From: Stefan-W. Hahn <stefan.hahn@s-hahn.de>

Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.
This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of
lseek()/xread()/lseek() to emulate pread.

Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
---
 Makefile          |    7 +++++++
 compat/pread.c    |   15 +++++++++++++++
 git-compat-util.h |    5 +++++
 3 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 6c12bc6..47af0de 100644
--- a/Makefile
+++ b/Makefile
@@ -69,6 +69,9 @@ 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).
+#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -523,6 +526,10 @@ ifdef NO_MMAP
 	COMPAT_CFLAGS += -DNO_MMAP
 	COMPAT_OBJS += compat/mmap.o
 endif
+ifdef NO_PREAD
+	COMPAT_CFLAGS += -DNO_PREAD
+	COMPAT_OBJS += compat/pread.o
+endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
 endif
diff --git a/compat/pread.c b/compat/pread.c
new file mode 100644
index 0000000..cd1da87
--- /dev/null
+++ b/compat/pread.c
@@ -0,0 +1,15 @@
+#include "../git-compat-util.h"
+
+ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)
+{
+        off_t current_offset = lseek(fd, 0, SEEK_CUR);
+
+        if (lseek(fd, offset, SEEK_SET) < 0)
+                return EINVAL;
+
+        ssize_t rc=xread(fd, buf, count);
+
+        if (current_offset != lseek(fd, current_offset, SEEK_SET))
+                return EINVAL;
+        return rc;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index e023bf1..00b14df 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -107,6 +107,11 @@ extern int git_munmap(void *start, size_t length);
 #define DEFAULT_PACKED_GIT_LIMIT \
 	((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))
 
+#ifdef NO_PREAD
+#define pread git_pread
+extern ssize_t git_pread(int fd,void *buf,size_t count,off_t offset);
+#endif
+
 #ifdef NO_SETENV
 #define setenv gitsetenv
 extern int gitsetenv(const char *, const char *, int);
-- 
1.4.4.4.g46aa

Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.
> This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of
> lseek()/xread()/lseek() to emulate pread.
>
> Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>

Thanks.

> +# Define NO_PREAD if you have a problem with pread() system call (i.e.
> +# cygwin.dll before v1.5.22).
> +#

I would have preferred e.g. not i.e..

> diff --git a/compat/pread.c b/compat/pread.c
> new file mode 100644
> index 0000000..cd1da87
> --- /dev/null
> +++ b/compat/pread.c
> @@ -0,0 +1,15 @@
> +#include "../git-compat-util.h"
> +
> +ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)

Style: s/,/, /; applies to git-compat-util.h as well.

> +{
> +        off_t current_offset = lseek(fd, 0, SEEK_CUR);
> +
> +        if (lseek(fd, offset, SEEK_SET) < 0)
> +                return EINVAL;
> +
> +        ssize_t rc=xread(fd, buf, count);

Style: please don't have declaration after statement.  Yes, it
is in the more recent ANSI, but the rest of git has avoided it
so far and having the decls upfront makes it easier to read.

I do not think you would want to return EINVAL.

Andy's read_in_full() is now on 'master' so please use it
instead to avoid short read.