[PATCH] xread(): Fix read error when filtering >= 2GB on Mac OS X

Previously, filtering more than 2GB through an external filter (see
test) failed on Mac OS X 10.8.4 (12E55) with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason is that read() immediately returns with EINVAL if len >= 2GB.
I haven't found any information under which specific conditions this
occurs.  My suspicion is that it happens when reading from a pipe, while
reading from a standard file should always be fine.  I haven't tested
any other version of Mac OS X, though I'd expect that other versions are
affected as well.

The problem is fixed by always reading less than 2GB in xread().
xread() doesn't guarantee to read all the requested data at once, and
callers are expected to gracefully handle partial reads.  Slicing large
reads into 2GB pieces should not hurt practical performance.

Signed-off-by: Steffen Prohaska <redacted>
---
 t/t0021-conversion.sh | 9 +++++++++
 wrapper.c             | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..aec1253 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,13 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test_expect_success 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	dd if=/dev/zero of=2GB count=2097152 bs=1024 &&
+	echo "/2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! grep -q "error" err
+'
+
 test_done

diff --git a/wrapper.c b/wrapper.c
index 6a015de..2a2f496 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -139,6 +139,14 @@ ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
 	while (1) {
+#ifdef __APPLE__
+		const size_t twoGB = (1l << 31);
+		/* len >= 2GB immediately fails on Mac OS X with EINVAL when
+		 * reading from pipe. */
+		if (len >= twoGB) {
+			len = twoGB - 1;
+		}
+#endif
 		nr = read(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
 			continue;

-- 
1.8.4.rc3.5.gfcb973a

On Sat, Aug 17, 2013 at 02:40:05PM +0200, Steffen Prohaska wrote:

Please don't use unnecessary curly braces here (see
Documentation/CodingGuidelines).

+#endif
 		nr = read(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
 			continue;
-- 
1.8.4.rc3.5.gfcb973a

On 2013-08-17 14.40, Steffen Prohaska wrote:

Thanks for the patch.
I think we can we can replace  __APPLE__ define with a more generic one.
We had a similar patch for write() some time ago:

config.mak.uname
	NEEDS_CLIPPED_WRITE = YesPlease


Makefile
ifdef NEEDS_CLIPPED_WRITE
	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
	COMPAT_OBJS += compat/clipped-write.o
endif

Am 17.08.2013 14:40, schrieb Steffen Prohaska:

We don't have /dev/zero on Windows. Even if we get a file slightly over 
2GB, we can't handle it on Windows, and other 32bit architectures will 
very likely also be handicapped.

Finally, this test (if it remains in some form) should probably be 
protected by EXPENSIVE.

+	echo "/2GB filter=largefile" >.gitattributes &&

Drop the slash, please; it may confuse our bash on Windows (it doesn't 
currently because echo is a builtin, but better safe than sorry).

+	git add 2GB 2>err &&
+	! grep -q "error" err

Executive summary: drop everything starting at "2>err".

Long story: Can it happen that (1) git add succeeds, but still produces 
something on stderr, and (2) we do not care what this something is as long 
as it does not contain "error"? I don't think this combination of 
conditions makes sense; it's sufficient to check that git add does not fail.

BTW, if you add

	... &&
	rm -f 2GB &&
	git checkout -- 2GB

you would also test the smudge filter code path with a huge file, no?

BTW2, to create a file with slightly over 2GB, you can use

	for i in $(test_seq 0 128); do printf "%16777216d" 1; done >2GB

-- Hannes

Hi,

Steffen Prohaska wrote:

See 6c642a87 (compat: large write(2) fails on Mac OS X/XNU,
2013-05-10) for a cleaner way to do this.

Hope that helps,
Jonathan

On Aug 17, 2013, at 05:40, Steffen Prohaska wrote:

Previously, filtering more than 2GB through an external filter (see
test) failed on Mac OS X 10.8.4 (12E55) with:

   error: read from external filter cat failed
   error: cannot feed the input to external filter cat
   error: cat died of signal 13
   error: external filter cat failed 141
   error: external filter cat failed

The reason is that read() immediately returns with EINVAL if len >=  
2GB.
I haven't found any information under which specific conditions this
occurs.

According to POSIX [1] for read:

If the value of nbyte is greater than {SSIZE_MAX}, the result is  
implementation-defined.


The write function also has the same restriction [2].

Since OS X still supports running 32-bit executables, and SSIZE_MAX is  
2GB - 1 when running 32-bit it would seem the same limit has been  
imposed on 64-bit executables.  In any case, we should avoid  
"implementation-defined" behavior for portability unless we know the  
OS we were compiled on has acceptable "implementation-defined"  
behavior and otherwise never attempt to read or write more than  
SSIZE_MAX bytes.

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

Kyle J. McKay wrote:

According to POSIX [1] for read:

If the value of nbyte is greater than {SSIZE_MAX}, the result is
implementation-defined.

Sure.

[...]

Since OS X still supports running 32-bit executables, and SSIZE_MAX is 2GB -
1 when running 32-bit it would seem the same limit has been imposed on
64-bit executables.  In any case, we should avoid "implementation-defined"
behavior

Wait --- that's a big leap.

In a 64-bit executable, SSIZE_MAX is 2^63 - 1, so the behavior is not
implementation-defined.  I'm not sure if Steffen's copy of git is
32-bit or 64-bit --- my guess would be 64-bit.  So at first glance
this does look like an XNU-specific bug, not a standards thing.

What about the related case where someone does try to "git add"
a file with a clean filter producing more than SSIZE_MAX and less
than SIZE_MAX bytes?

strbuf_grow() does not directly protect against a strbuf growing to >
SSIZE_MAX bytes, but in practice on most machines realloc() does.  So
in practice we could never read more than SSIZE_MAX characters in the
strbuf_read() codepath, but it might be worth a check for paranoia
anyway.

While we're here, it's easy to wonder: why is git reading into such a
large buffer anyway?  Normally git uses the streaming codepath for
files larger than big_file_threshold (typically 512 MiB).
Unfortunately there are cases where it doesn't.  For example:

  - convert_to_git() has not been taught to stream, so paths
    with a clean filter or requiring crlf conversion are read or
    mapped into memory.

  - deflate_to_pack() relies on seeking backward to retry when
    a pack would grow too large, so "git hash-object --stdin"
    cannot use that codepath.

  - a "clean" filter can make a file bigger.

  Perhaps git needs to learn to write to a temporary file
  when asked to keep track of a blob that is larger than fits
  reasonably in memory.  Or maybe not.

So there is room for related work but the codepaths that read()
indefinitely large files do seem to be needed, at least in the short
term.  Working around this Mac OS X-specific limitation at the read()
level like you've done still sounds like the right thing to do.

Thanks, both, for your work tracking this down.  Hopefully the next
version of the patch will be in good shape and then it can be applied
quickly.

Thanks and hope that helps,
Jonathan

Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed in a earlier
commit [6c642a].

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.
Unfortunately, '#undef read' is needed at a few places to avoid
expanding the compat macro in constructs like 'vtbl->read(...)'.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for their suggestions:

    Johannes Sixt [off-list ref]
    John Keeping [off-list ref]
    Jonathan Nieder [off-list ref]
    Kyle J. McKay [off-list ref]
    Torsten Bögershausen [off-list ref]

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] 6c642a878688adf46b226903858b53e2d31ac5c3
    compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska <redacted>
---
 Makefile              |  8 ++++++++
 builtin/var.c         |  1 +
 config.mak.uname      |  1 +
 git-compat-util.h     |  5 +++++
 streaming.c           |  1 +
 t/t0021-conversion.sh | 14 ++++++++++++++
 6 files changed, 30 insertions(+)

diff --git a/Makefile b/Makefile
index 3588ca1..0f69e24 100644
--- a/Makefile
+++ b/Makefile

@@ -69,6 +69,9 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
+# Define NEEDS_CLIPPED_READ if your read(2) cannot read more than
+# INT_MAX bytes at once (e.g. MacOS X).
+#
 # Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
 # INT_MAX bytes at once (e.g. MacOS X).
 #

@@ -1493,6 +1496,11 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check --statistics
 endif
 
+ifdef NEEDS_CLIPPED_READ
+	BASIC_CFLAGS += -DNEEDS_CLIPPED_READ
+	COMPAT_OBJS += compat/clipped-read.o
+endif
+
 ifdef NEEDS_CLIPPED_WRITE
 	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
 	COMPAT_OBJS += compat/clipped-write.o

diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..e59f5ba 100644
--- a/builtin/var.c
+++ b/builtin/var.c

@@ -38,6 +38,7 @@ static struct git_var git_vars[] = {
 	{ "", NULL },
 };
 
+#undef read
 static void list_vars(void)
 {
 	struct git_var *ptr;

diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..5c10726 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -95,6 +95,7 @@ ifeq ($(uname_S),Darwin)
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
+	NEEDS_CLIPPED_READ = YesPlease
 	NEEDS_CLIPPED_WRITE = YesPlease
 	COMPAT_OBJS += compat/precompose_utf8.o
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE

diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..a227127 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -185,6 +185,11 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
+#ifdef NEEDS_CLIPPED_READ
+ssize_t clipped_read(int fd, void *buf, size_t nbyte);
+#define read(x,y,z) clipped_read((x),(y),(z))
+#endif
+
 #ifdef NEEDS_CLIPPED_WRITE
 ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
 #define write(x,y,z) clipped_write((x),(y),(z))

diff --git a/streaming.c b/streaming.c
index debe904..c1fe34a 100644
--- a/streaming.c
+++ b/streaming.c

@@ -99,6 +99,7 @@ int close_istream(struct git_istream *st)
 	return r;
 }
 
+#undef read
 ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
 {
 	return st->vtbl->read(st, buf, sz);

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'
+
 test_done

-- 
1.8.4.rc3.5.ga3343dd

On Mon, Aug 19, 2013 at 08:38:20AM +0200, Steffen Prohaska wrote:

You've created compat/clipped-write.c, but...

 Makefile              |  8 ++++++++
 builtin/var.c         |  1 +
 config.mak.uname      |  1 +
 git-compat-util.h     |  5 +++++
 streaming.c           |  1 +
 t/t0021-conversion.sh | 14 ++++++++++++++
 6 files changed, 30 insertions(+)

... it's not included here.  Did you forget to "git add"?

Am 19.08.2013 08:38, schrieb Steffen Prohaska:

+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&

Shouldn't you count to 2049 to get a file that is over 2GB?

+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'

-- Hannes

On Aug 19, 2013, at 9:54 AM, John Keeping [off-list ref] wrote:

You've created compat/clipped-read.c, but...

quoted

Makefile              |  8 ++++++++
builtin/var.c         |  1 +
config.mak.uname      |  1 +
git-compat-util.h     |  5 +++++
streaming.c           |  1 +
t/t0021-conversion.sh | 14 ++++++++++++++
6 files changed, 30 insertions(+)

... it's not included here.  Did you forget to "git add"?

Indeed.  How embarrassing.  Thanks for spotting this.  I'll send v3 in a minute.

	Stefffen

Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed in a earlier
commit [6c642a].

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.
Unfortunately, '#undef read' is needed at a few places to avoid
expanding the compat macro in constructs like 'vtbl->read(...)'.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for their suggestions:

    Johannes Sixt [off-list ref]
    John Keeping [off-list ref]
    Jonathan Nieder [off-list ref]
    Kyle J. McKay [off-list ref]
    Torsten Bögershausen [off-list ref]

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] 6c642a878688adf46b226903858b53e2d31ac5c3
    compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska <redacted>
---
 Makefile              |  8 ++++++++
 builtin/var.c         |  1 +
 compat/clipped-read.c | 13 +++++++++++++
 config.mak.uname      |  1 +
 git-compat-util.h     |  5 +++++
 streaming.c           |  1 +
 t/t0021-conversion.sh | 14 ++++++++++++++
 7 files changed, 43 insertions(+)
 create mode 100644 compat/clipped-read.c

diff --git a/Makefile b/Makefile
index 3588ca1..0f69e24 100644
--- a/Makefile
+++ b/Makefile

@@ -69,6 +69,9 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
+# Define NEEDS_CLIPPED_READ if your read(2) cannot read more than
+# INT_MAX bytes at once (e.g. MacOS X).
+#
 # Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
 # INT_MAX bytes at once (e.g. MacOS X).
 #

@@ -1493,6 +1496,11 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check --statistics
 endif
 
+ifdef NEEDS_CLIPPED_READ
+	BASIC_CFLAGS += -DNEEDS_CLIPPED_READ
+	COMPAT_OBJS += compat/clipped-read.o
+endif
+
 ifdef NEEDS_CLIPPED_WRITE
 	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
 	COMPAT_OBJS += compat/clipped-write.o

diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..e59f5ba 100644
--- a/builtin/var.c
+++ b/builtin/var.c

@@ -38,6 +38,7 @@ static struct git_var git_vars[] = {
 	{ "", NULL },
 };
 
+#undef read
 static void list_vars(void)
 {
 	struct git_var *ptr;

diff --git a/compat/clipped-read.c b/compat/clipped-read.c
new file mode 100644
index 0000000..6962f67
--- /dev/null
+++ b/compat/clipped-read.c

@@ -0,0 +1,13 @@
+#include "../git-compat-util.h"
+#undef read
+
+/*
+ * Version of read that will write at most INT_MAX bytes.
+ * Workaround a xnu bug on Mac OS X
+ */
+ssize_t clipped_read(int fd, void *buf, size_t nbyte)
+{
+	if (nbyte > INT_MAX)
+		nbyte = INT_MAX;
+	return read(fd, buf, nbyte);
+}

diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..5c10726 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -95,6 +95,7 @@ ifeq ($(uname_S),Darwin)
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
+	NEEDS_CLIPPED_READ = YesPlease
 	NEEDS_CLIPPED_WRITE = YesPlease
 	COMPAT_OBJS += compat/precompose_utf8.o
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE

diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..a227127 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -185,6 +185,11 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
+#ifdef NEEDS_CLIPPED_READ
+ssize_t clipped_read(int fd, void *buf, size_t nbyte);
+#define read(x,y,z) clipped_read((x),(y),(z))
+#endif
+
 #ifdef NEEDS_CLIPPED_WRITE
 ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
 #define write(x,y,z) clipped_write((x),(y),(z))

diff --git a/streaming.c b/streaming.c
index debe904..c1fe34a 100644
--- a/streaming.c
+++ b/streaming.c

@@ -99,6 +99,7 @@ int close_istream(struct git_istream *st)
 	return r;
 }
 
+#undef read
 ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
 {
 	return st->vtbl->read(st, buf, sz);

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'
+
 test_done

-- 
1.8.4.rc3.5.g4f480ff

On 08/19/2013 10:20 AM, Johannes Sixt wrote:

Am 19.08.2013 08:38, schrieb Steffen Prohaska:

quoted

+test_expect_success EXPENSIVE 'filter large file' '
+    git config filter.largefile.smudge cat &&
+    git config filter.largefile.clean cat &&
+    for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&

Shouldn't you count to 2049 to get a file that is over 2GB?

Would it be possible to offload the looping from shell to a real
program? So for example
	truncate -s 2049M <filename>
should do the job. That would create a file reading all bytes as zeros	
being larger as 2G. If truncate is not available, what about dd?

Stefan

Am 19.08.2013 08:38, schrieb Steffen Prohaska:

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks for this hint. I was not aware of this behavior.

Of course, we do *not* want to use test_must_fail because git add 
generally must not fail for files with more than 2GB. (Architectures with 
a 32bit size_t are a different matter, of course.)

-- Hannes

On Aug 19, 2013, at 10:20 AM, Johannes Sixt [off-list ref] wrote:

Am 19.08.2013 08:38, schrieb Steffen Prohaska:

quoted

+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&

Shouldn't you count to 2049 to get a file that is over 2GB?

No.  INT_MAX = 2GB - 1 works.  INT_MAX + 1 = 2GB fails.  It tests exactly at the boundary.

	Steffen

Am 19.08.2013 10:25, schrieb Stefan Beller:

On 08/19/2013 10:20 AM, Johannes Sixt wrote:

quoted

Am 19.08.2013 08:38, schrieb Steffen Prohaska:

quoted

+test_expect_success EXPENSIVE 'filter large file' '
+    git config filter.largefile.smudge cat &&
+    git config filter.largefile.clean cat &&
+    for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&

Shouldn't you count to 2049 to get a file that is over 2GB?

Would it be possible to offload the looping from shell to a real
program? So for example
	truncate -s 2049M <filename>
should do the job. That would create a file reading all bytes as zeros	
being larger as 2G. If truncate is not available, what about dd?

The point is exactly to avoid external dependencies. Our dd on Windows 
doesn't do the right thing with seek=2GB (it makes the file twice as large 
as expected).

-- Hannes

On Mon, Aug 19, 2013 at 4:21 AM, Steffen Prohaska [off-list ref] wrote:

Is it likely that we would see a platform requiring only one or the
other CLIPPED? Would it make sense to combine these into a single
NEEDS_CLIPPED_IO?

On 2013-08-19 08.38, Steffen Prohaska wrote:
[snip]

This is techically right for this very version of the  code,
but not really future proof, if someone uses read() further down in the code
(in a later version)

I think the problem comes from further up:
------------------
struct git_var {
	const char *name;
	const char *(*read)(int);
};
-----------------
could the read be replaced by readfn ?

===================

Same possible future problem as above.
When later someone uses read, the original (buggy) read() will be
used, and not the re-defined clipped_read() from git-compat-util.h

Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed in a earlier
commit [6c642a].

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.  It is
very likely that the read() and write() wrappers are always used
together.  To avoid introducing another option, NEEDS_CLIPPED_WRITE is
changed to NEEDS_CLIPPED_IO and used to activate both wrappers.

To avoid expanding the read compat macro in constructs like
'vtbl->read(...)', 'read' is renamed to 'readfn' in two cases.  The
solution seems more robust than using '#undef read'.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for their suggestions:

    Johannes Sixt [off-list ref]
    John Keeping [off-list ref]
    Jonathan Nieder [off-list ref]
    Kyle J. McKay [off-list ref]
    Torsten Bögershausen [off-list ref]
    Eric Sunshine [off-list ref]

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] 6c642a878688adf46b226903858b53e2d31ac5c3
    compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska <redacted>
---
 Makefile                                 | 10 +++++-----
 builtin/var.c                            | 10 +++++-----
 compat/{clipped-write.c => clipped-io.c} | 11 ++++++++++-
 config.mak.uname                         |  2 +-
 git-compat-util.h                        |  5 ++++-
 streaming.c                              |  4 ++--
 t/t0021-conversion.sh                    | 14 ++++++++++++++
 7 files changed, 41 insertions(+), 15 deletions(-)
 rename compat/{clipped-write.c => clipped-io.c} (53%)

diff --git a/Makefile b/Makefile
index 3588ca1..f54134d 100644
--- a/Makefile
+++ b/Makefile

@@ -69,8 +69,8 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
-# Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
-# INT_MAX bytes at once (e.g. MacOS X).
+# Define NEEDS_CLIPPED_IO if your read(2) and/or write(2) cannot handle more
+# than INT_MAX bytes at once (e.g. Mac OS X).
 #
 # Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
 # it specifies.

@@ -1493,9 +1493,9 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check --statistics
 endif
 
-ifdef NEEDS_CLIPPED_WRITE
-	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
-	COMPAT_OBJS += compat/clipped-write.o
+ifdef NEEDS_CLIPPED_IO
+	BASIC_CFLAGS += -DNEEDS_CLIPPED_IO
+	COMPAT_OBJS += compat/clipped-io.o
 endif
 
 ifneq (,$(XDL_FAST_HASH))

diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..06f8459 100644
--- a/builtin/var.c
+++ b/builtin/var.c

@@ -28,7 +28,7 @@ static const char *pager(int flag)
 
 struct git_var {
 	const char *name;
-	const char *(*read)(int);
+	const char *(*readfn)(int);
 };
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },

@@ -43,8 +43,8 @@ static void list_vars(void)
 	struct git_var *ptr;
 	const char *val;
 
-	for (ptr = git_vars; ptr->read; ptr++)
-		if ((val = ptr->read(0)))
+	for (ptr = git_vars; ptr->readfn; ptr++)
+		if ((val = ptr->readfn(0)))
 			printf("%s=%s\n", ptr->name, val);
 }
 

@@ -53,9 +53,9 @@ static const char *read_var(const char *var)
 	struct git_var *ptr;
 	const char *val;
 	val = NULL;
-	for (ptr = git_vars; ptr->read; ptr++) {
+	for (ptr = git_vars; ptr->readfn; ptr++) {
 		if (strcmp(var, ptr->name) == 0) {
-			val = ptr->read(IDENT_STRICT);
+			val = ptr->readfn(IDENT_STRICT);
 			break;
 		}
 	}

diff --git a/compat/clipped-write.c b/compat/clipped-io.c
similarity index 53%
rename from compat/clipped-write.c
rename to compat/clipped-io.c
index b8f98ff..ec3232a 100644
--- a/compat/clipped-write.c
+++ b/compat/clipped-io.c

@@ -1,10 +1,19 @@
 #include "../git-compat-util.h"
+#undef read
 #undef write
 
 /*
- * Version of write that will write at most INT_MAX bytes.
+ * Versions of read() and write() that limit nbyte to INT_MAX.
  * Workaround a xnu bug on Mac OS X
  */
+
+ssize_t clipped_read(int fd, void *buf, size_t nbyte)
+{
+	if (nbyte > INT_MAX)
+		nbyte = INT_MAX;
+	return read(fd, buf, nbyte);
+}
+
 ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
 {
 	if (nbyte > INT_MAX)

diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..fb39726 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -95,7 +95,7 @@ ifeq ($(uname_S),Darwin)
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
-	NEEDS_CLIPPED_WRITE = YesPlease
+	NEEDS_CLIPPED_IO = YesPlease
 	COMPAT_OBJS += compat/precompose_utf8.o
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
 endif

diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..4a875cb 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -185,7 +185,10 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
-#ifdef NEEDS_CLIPPED_WRITE
+#ifdef NEEDS_CLIPPED_IO
+ssize_t clipped_read(int fd, void *buf, size_t nbyte);
+#define read(x,y,z) clipped_read((x),(y),(z))
+
 ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
 #define write(x,y,z) clipped_write((x),(y),(z))
 #endif

diff --git a/streaming.c b/streaming.c
index debe904..2ac3047 100644
--- a/streaming.c
+++ b/streaming.c

@@ -20,7 +20,7 @@ typedef ssize_t (*read_istream_fn)(struct git_istream *, char *, size_t);
 
 struct stream_vtbl {
 	close_istream_fn close;
-	read_istream_fn read;
+	read_istream_fn readfn;
 };
 
 #define open_method_decl(name) \

@@ -101,7 +101,7 @@ int close_istream(struct git_istream *st)
 
 ssize_t read_istream(struct git_istream *st, void *buf, size_t sz)
 {
-	return st->vtbl->read(st, buf, sz);
+	return st->vtbl->readfn(st, buf, sz);
 }
 
 static enum input_source istream_source(const unsigned char *sha1,

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'
+
 test_done

-- 
1.8.4.rc3.5.g4f480ff

On Mon, Aug 19, 2013 at 8:41 AM, Steffen Prohaska [off-list ref] wrote:

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.

Yeah, the OS X filesystem layer is an incredible piece of shit. Not
only doesn't it follow POSIX, it fails *badly*. Because OS X kernel
engineers apparently have the mental capacity of a retarded rodent on
crack.

Linux also refuses to actually read more than a maximum value in one
go (because quite frankly, doing more than 2GB at a time is just not
reasonable, especially in unkillable disk wait), but at least Linux
gives you the partial read, so that the usual "read until you're
happy" works (which you have to do anyway with sockets, pipes, NFS
intr mounts, etc etc). Returning EINVAL is a sign of a diseased mind.

I hate your patch for other reasons, though:

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.

Why do you do that? We already _have_ wrapper functions for read(),
namely xread().  Exactly because you basically have to, in order to
handle signals on interruptible filesystems (which aren't POSIX
either, but at least sanely so) or from other random sources. And to
handle the "you can't do reads that big" issue.

So why isn't the patch much more straightforward? Like the attached
totally untested one that just limits the read/write size to 8MB
(which is totally arbitrary, but small enough to not have any latency
issues even on slow disks, and big enough that any reasonable IO
subsystem will still get good throughput).

And by "totally untested" I mean that it actually passes the git test
suite, but since I didn't apply your patch nor do I have OS X
anywhere, I can't actually test that it fixes *your* problem. But it
should.


                   Linus

On Aug 19, 2013, at 6:04 PM, Linus Torvalds [off-list ref] wrote:

I hate your patch for other reasons, though:

quoted

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.

Why do you do that? We already _have_ wrapper functions for read(),
namely xread().  Exactly because you basically have to, in order to
handle signals on interruptible filesystems (which aren't POSIX
either, but at least sanely so) or from other random sources. And to
handle the "you can't do reads that big" issue.

So why isn't the patch much more straightforward? 

The first version was more straightforward [1].  But reviewers suggested
that the compat wrappers would be the right way to do it and showed me
that it has been done like this before [2].

I haven't submitted anything in a while, so I tried to be a kind person
and followed the suggestions.  I started to hate the patch a bit (maybe less
than you), but I wasn't brave enough to reject the suggestions.  This is
why the patch became what it is.

I'm happy to rework it again towards your suggestion.  I would also remove
the compat wrapper for write().  But I got a bit tired.  I'd appreciate if
I received more indication whether a version without compat wrappers would
be accepted.

	Steffen

[1] http://article.gmane.org/gmane.comp.version-control.git/232455
[2] 6c642a8 compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed earlier [6c642a].

This commit addresses the problem for read() and write() by limiting
size of IO chunks unconditionally on all platforms in xread() and
xwrite().  Large chunks only cause problems, like triggering the OS
X bug or causing latencies when killing the process.  Reasonably sized
smaller chunks have no negative impact on performance.

The compat wrapper clipped_write() introduced earlier [6c642a] is not
needed anymore.  It will be reverted in a separate commit.  The new test
catches read and write problems.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for suggestions and testing:

    Johannes Sixt [off-list ref]
    John Keeping [off-list ref]
    Jonathan Nieder [off-list ref]
    Kyle J. McKay [off-list ref]
    Linus Torvalds [off-list ref]
    Torsten Bögershausen [off-list ref]

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] commit 6c642a878688adf46b226903858b53e2d31ac5c3
    compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska <redacted>
---
 t/t0021-conversion.sh | 14 ++++++++++++++
 wrapper.c             | 12 ++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'
+
 test_done

diff --git a/wrapper.c b/wrapper.c
index 6a015de..97e3cf7 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -131,6 +131,14 @@ void *xcalloc(size_t nmemb, size_t size)
 }
 
 /*
+ * Limit size of IO chunks, because huge chunks only cause pain.  OS X 64-bit
+ * buggy, returning EINVAL if len >= INT_MAX; and even in the absense of bugs,
+ * large chunks can result in bad latencies when you decide to kill the
+ * process.
+ */
+#define MAX_IO_SIZE (8*1024*1024)
+
+/*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
  * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.

@@ -138,6 +146,8 @@ void *xcalloc(size_t nmemb, size_t size)
 ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > MAX_IO_SIZE)
+	    len = MAX_IO_SIZE;
 	while (1) {
 		nr = read(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))

@@ -154,6 +164,8 @@ ssize_t xread(int fd, void *buf, size_t len)
 ssize_t xwrite(int fd, const void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > MAX_IO_SIZE)
+	    len = MAX_IO_SIZE;
 	while (1) {
 		nr = write(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))

-- 
1.8.4.rc3.5.g4f480ff

The previous commit introduced a size limit on IO chunks on all
platforms.  The compat clipped_write() is not needed anymore.

This reverts commit 6c642a878688adf46b226903858b53e2d31ac5c3.

Signed-off-by: Steffen Prohaska <redacted>
---
 Makefile               |  8 --------
 compat/clipped-write.c | 13 -------------
 config.mak.uname       |  1 -
 git-compat-util.h      |  5 -----
 4 files changed, 27 deletions(-)
 delete mode 100644 compat/clipped-write.c

diff --git a/Makefile b/Makefile
index 3588ca1..4026211 100644
--- a/Makefile
+++ b/Makefile

@@ -69,9 +69,6 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
-# Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
-# INT_MAX bytes at once (e.g. MacOS X).
-#
 # Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
 # it specifies.
 #

@@ -1493,11 +1490,6 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check --statistics
 endif
 
-ifdef NEEDS_CLIPPED_WRITE
-	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
-	COMPAT_OBJS += compat/clipped-write.o
-endif
-
 ifneq (,$(XDL_FAST_HASH))
 	BASIC_CFLAGS += -DXDL_FAST_HASH
 endif

diff --git a/compat/clipped-write.c b/compat/clipped-write.c
deleted file mode 100644
index b8f98ff..0000000
--- a/compat/clipped-write.c
+++ /dev/null

@@ -1,13 +0,0 @@
-#include "../git-compat-util.h"
-#undef write
-
-/*
- * Version of write that will write at most INT_MAX bytes.
- * Workaround a xnu bug on Mac OS X
- */
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
-{
-	if (nbyte > INT_MAX)
-		nbyte = INT_MAX;
-	return write(fildes, buf, nbyte);
-}

diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..7d61531 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -95,7 +95,6 @@ ifeq ($(uname_S),Darwin)
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
-	NEEDS_CLIPPED_WRITE = YesPlease
 	COMPAT_OBJS += compat/precompose_utf8.o
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
 endif

diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..96d8881 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -185,11 +185,6 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
-#ifdef NEEDS_CLIPPED_WRITE
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
-#define write(x,y,z) clipped_write((x),(y),(z))
-#endif
-
 #ifdef MKDIR_WO_TRAILING_SLASH
 #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
 extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);

-- 
1.8.4.rc3.5.g4f480ff

This is the revised patch taking the considerations about IO chunk size into
account.  The series deletes more than it adds and fixes a bug.  Nice.

Steffen Prohaska (2):
  xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X
  Revert "compate/clipped-write.c: large write(2) fails on Mac OS X/XNU"

 Makefile               |  8 --------
 compat/clipped-write.c | 13 -------------
 config.mak.uname       |  1 -
 git-compat-util.h      |  5 -----
 t/t0021-conversion.sh  | 14 ++++++++++++++
 wrapper.c              | 12 ++++++++++++
 6 files changed, 26 insertions(+), 27 deletions(-)
 delete mode 100644 compat/clipped-write.c

-- 
1.8.4.rc3.5.g4f480ff

Fixed typo in comment.

Steffen Prohaska (2):
  xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X
  Revert "compate/clipped-write.c: large write(2) fails on Mac OS X/XNU"

 Makefile               |  8 --------
 compat/clipped-write.c | 13 -------------
 config.mak.uname       |  1 -
 git-compat-util.h      |  5 -----
 t/t0021-conversion.sh  | 14 ++++++++++++++
 wrapper.c              | 12 ++++++++++++
 6 files changed, 26 insertions(+), 27 deletions(-)
 delete mode 100644 compat/clipped-write.c

-- 
1.8.4.rc3.5.g4f480ff

Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

    error: read from external filter cat failed
    error: cannot feed the input to external filter cat
    error: cat died of signal 13
    error: external filter cat failed 141
    error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte >=
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed earlier [6c642a].

This commit addresses the problem for read() and write() by limiting
size of IO chunks unconditionally on all platforms in xread() and
xwrite().  Large chunks only cause problems, like triggering the OS
X bug or causing latencies when killing the process.  Reasonably sized
smaller chunks have no negative impact on performance.

The compat wrapper clipped_write() introduced earlier [6c642a] is not
needed anymore.  It will be reverted in a separate commit.  The new test
catches read and write problems.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for suggestions and testing:

    Johannes Sixt [off-list ref]
    John Keeping [off-list ref]
    Jonathan Nieder [off-list ref]
    Kyle J. McKay [off-list ref]
    Linus Torvalds [off-list ref]
    Torsten Bögershausen [off-list ref]

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] commit 6c642a878688adf46b226903858b53e2d31ac5c3
    compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska <redacted>
---
 t/t0021-conversion.sh | 14 ++++++++++++++
 wrapper.c             | 12 ++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh

@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
 	test_must_fail git add test.fc
 '
 
+test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+	git config filter.largefile.smudge cat &&
+	git config filter.largefile.clean cat &&
+	for i in $(test_seq 1 2048); do printf "%1048576d" 1; done >2GB &&
+	echo "2GB filter=largefile" >.gitattributes &&
+	git add 2GB 2>err &&
+	! test -s err &&
+	rm -f 2GB &&
+	git checkout -- 2GB 2>err &&
+	! test -s err
+'
+
 test_done

diff --git a/wrapper.c b/wrapper.c
index 6a015de..66cc727 100644
--- a/wrapper.c
+++ b/wrapper.c

@@ -131,6 +131,14 @@ void *xcalloc(size_t nmemb, size_t size)
 }
 
 /*
+ * Limit size of IO chunks, because huge chunks only cause pain.  OS X 64-bit
+ * is buggy, returning EINVAL if len >= INT_MAX; and even in the absense of
+ * bugs, large chunks can result in bad latencies when you decide to kill the
+ * process.
+ */
+#define MAX_IO_SIZE (8*1024*1024)
+
+/*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
  * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.

@@ -138,6 +146,8 @@ void *xcalloc(size_t nmemb, size_t size)
 ssize_t xread(int fd, void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > MAX_IO_SIZE)
+	    len = MAX_IO_SIZE;
 	while (1) {
 		nr = read(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))

@@ -154,6 +164,8 @@ ssize_t xread(int fd, void *buf, size_t len)
 ssize_t xwrite(int fd, const void *buf, size_t len)
 {
 	ssize_t nr;
+	if (len > MAX_IO_SIZE)
+	    len = MAX_IO_SIZE;
 	while (1) {
 		nr = write(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))

-- 
1.8.4.rc3.5.g4f480ff

The previous commit introduced a size limit on IO chunks on all
platforms.  The compat clipped_write() is not needed anymore.

This reverts commit 6c642a878688adf46b226903858b53e2d31ac5c3.

Signed-off-by: Steffen Prohaska <redacted>
---
 Makefile               |  8 --------
 compat/clipped-write.c | 13 -------------
 config.mak.uname       |  1 -
 git-compat-util.h      |  5 -----
 4 files changed, 27 deletions(-)
 delete mode 100644 compat/clipped-write.c

diff --git a/Makefile b/Makefile
index 3588ca1..4026211 100644
--- a/Makefile
+++ b/Makefile

@@ -69,9 +69,6 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
-# Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
-# INT_MAX bytes at once (e.g. MacOS X).
-#
 # Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
 # it specifies.
 #

@@ -1493,11 +1490,6 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 	MSGFMT += --check --statistics
 endif
 
-ifdef NEEDS_CLIPPED_WRITE
-	BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
-	COMPAT_OBJS += compat/clipped-write.o
-endif
-
 ifneq (,$(XDL_FAST_HASH))
 	BASIC_CFLAGS += -DXDL_FAST_HASH
 endif

diff --git a/compat/clipped-write.c b/compat/clipped-write.c
deleted file mode 100644
index b8f98ff..0000000
--- a/compat/clipped-write.c
+++ /dev/null

@@ -1,13 +0,0 @@
-#include "../git-compat-util.h"
-#undef write
-
-/*
- * Version of write that will write at most INT_MAX bytes.
- * Workaround a xnu bug on Mac OS X
- */
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
-{
-	if (nbyte > INT_MAX)
-		nbyte = INT_MAX;
-	return write(fildes, buf, nbyte);
-}

diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..7d61531 100644
--- a/config.mak.uname
+++ b/config.mak.uname

@@ -95,7 +95,6 @@ ifeq ($(uname_S),Darwin)
 	NO_MEMMEM = YesPlease
 	USE_ST_TIMESPEC = YesPlease
 	HAVE_DEV_TTY = YesPlease
-	NEEDS_CLIPPED_WRITE = YesPlease
 	COMPAT_OBJS += compat/precompose_utf8.o
 	BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
 endif

diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..96d8881 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -185,11 +185,6 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
-#ifdef NEEDS_CLIPPED_WRITE
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
-#define write(x,y,z) clipped_write((x),(y),(z))
-#endif
-
 #ifdef MKDIR_WO_TRAILING_SLASH
 #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
 extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);

-- 
1.8.4.rc3.5.g4f480ff

On 2013-08-20 08.43, Steffen Prohaska wrote:
[]
Thanks for V5. It was tested OK on my system here.
(And apologies for recommending a wrapper on top of a wrapper).

One question is left: 
As xread() is tolerant against EAGAIN and especially EINTR,
could it make sense to replace read() with xread() everywhere?

(The risk for getting EINTR is smaller when we only read a small amount
of data, but it is more on the safe side)

And s/write/xwrite/

/Torsten