Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
On 2013-08-19 08.38, Steffen Prohaska wrote:
[snip]
> 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
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 ?
===
> 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
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
Am 19.08.2013 10:25, schrieb Stefan Beller: On 08/19/2013 10:20 AM, Johannes Sixt wrote: 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? Would it be possible to offload the looping from shell to a real program? So for example truncate -s 2049M 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
On Aug 19, 2013, at 10:20 AM, Johannes Sixt wrote: > 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? No. INT_MAX = 2GB - 1 works. INT_MAX + 1 = 2GB fails. It tests exactly at the boundary. Steffen -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
On 08/19/2013 10:20 AM, Johannes Sixt wrote: > 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? Would it be possible to offload the looping from shell to a real program? So for example truncate -s 2049M 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 signature.asc Description: OpenPGP digital signature
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
On Aug 19, 2013, at 9:54 AM, John Keeping wrote: > You've created compat/clipped-read.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"? Indeed. How embarrassing. Thanks for spotting this. I'll send v3 in a minute. Stefffen-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] compat: Fix read() of 2GB and more on Mac OS X
On Mon, Aug 19, 2013 at 08:38:20AM +0200, Steffen Prohaska wrote: > 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 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"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
