# [PATCH] http.postbuffer: make a size_t 6 messages from 2017-03-30 to 2017-03-31. Participants: David Turner, Shawn Pearce, Junio C Hamano, Torsten Bögershausen. Thread: https://gitlist.dev/t/45553 ## David Turner, 2017-03-30 20:29 Subject: [PATCH] http.postbuffer: make a size_t Message-ID: <20170330202917.24281-1-dturner@twosigma.com> URL: https://gitlist.dev/e/20170330202917.24281-1-dturner%40twosigma.com ``` Unfortunately, in order to push some large repos, the http postbuffer must sometimes exceed two gigabytes. On a 64-bit system, this is OK: we just malloc a larger buffer. Signed-off-by: David Turner --- cache.h | 1 + config.c | 17 +++++++++++++++++ http.c | 2 +- 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/cache.h b/cache.h index fbdf7a815a..a8c1b65db0 100644 --- a/cache.h +++ b/cache.h @@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *); extern int git_config_int(const char *, const char *); extern int64_t git_config_int64(const char *, const char *); extern unsigned long git_config_ulong(const char *, const char *); +extern size_t git_config_size_t(const char *, const char *); extern int git_config_bool_or_int(const char *, const char *, int *); extern int git_config_bool(const char *, const char *); extern int git_config_maybe_bool(const char *, const char *); diff --git a/config.c b/config.c index 1a4d85537b..7b706cf27a 100644 --- a/config.c +++ b/config.c @@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned long *ret) return 1; } +static size_t git_parse_size_t(const char *value, unsigned long *ret) +{ + size_t tmp; + if (!git_parse_signed(value, &tmp, maximum_unsigned_value_of_type(size_t))) + return 0; + *ret = tmp; + return 1; +} + NORETURN static void die_bad_number(const char *name, const char *value) { @@ -892,6 +901,14 @@ unsigned long git_config_ulong(const char *name, const char *value) return ret; } +size_t git_config_size_t(const char *name, const char *value) +{ + unsigned long ret; + if (!git_parse_size_t(value, &ret)) + die_bad_number(name, value); + return ret; +} + int git_parse_maybe_bool(const char *value) { if (!value) diff --git a/http.c b/http.c index 96d84bbed3..ab6080835f 100644 --- a/http.c +++ b/http.c @@ -331,7 +331,7 @@ static int http_options(const char *var, const char *value, void *cb) } if (!strcmp("http.postbuffer", var)) { - http_post_buffer = git_config_int(var, value); + http_post_buffer = git_config_size_t(var, value); if (http_post_buffer < LARGE_PACKET_MAX) http_post_buffer = LARGE_PACKET_MAX; return 0; -- 2.11.GIT ``` ## Shawn Pearce, 2017-03-30 20:42 Subject: Re: [PATCH] http.postbuffer: make a size_t Message-ID: URL: https://gitlist.dev/e/CAJo%3DhJvs0UKO_NLbWAi8Y8XRJJ0v5kdjf9BN0V%3DpN_9yYrDp7Q%40mail.gmail.com In-Reply-To: <20170330202917.24281-1-dturner@twosigma.com> ``` On Thu, Mar 30, 2017 at 1:29 PM, David Turner wrote: > Unfortunately, in order to push some large repos, the http postbuffer > must sometimes exceed two gigabytes. On a 64-bit system, this is OK: > we just malloc a larger buffer. I'm slightly curious what server you are pushing to that needs the entire thing buffered to compute a Content-Length, rather than using Transfer-Encoding: chunked. Most Git-over-HTTP should be accepting Transfer-Encoding: chunked when the stream exceeds postBuffer. ``` ## David Turner, 2017-03-30 20:59 Subject: RE: [PATCH] http.postbuffer: make a size_t Message-ID: <82ec49a3ae4d402daf6113cf19c47fb0@exmbdft7.ad.twosigma.com> URL: https://gitlist.dev/e/82ec49a3ae4d402daf6113cf19c47fb0%40exmbdft7.ad.twosigma.com In-Reply-To: ``` GitLab. I can't speak to our particular configuration of it -- but if you have a specific question about what the config is, I can ask our gitlab admins. > -----Original Message----- > From: Shawn Pearce [mailto:spearce@spearce.org] > Sent: Thursday, March 30, 2017 4:42 PM > To: David Turner > Cc: git > Subject: Re: [PATCH] http.postbuffer: make a size_t > > On Thu, Mar 30, 2017 at 1:29 PM, David Turner > wrote: > > Unfortunately, in order to push some large repos, the http postbuffer > > must sometimes exceed two gigabytes. On a 64-bit system, this is OK: > > we just malloc a larger buffer. > > I'm slightly curious what server you are pushing to that needs the entire thing > buffered to compute a Content-Length, rather than using > Transfer-Encoding: chunked. Most Git-over-HTTP should be accepting > Transfer-Encoding: chunked when the stream exceeds postBuffer. ``` ## Junio C Hamano, 2017-03-30 22:07 Subject: Re: [PATCH] http.postbuffer: make a size_t Message-ID: URL: https://gitlist.dev/e/xmqqwpb61kre.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170330202917.24281-1-dturner@twosigma.com> ``` David Turner writes: > Unfortunately, in order to push some large repos, the http postbuffer > must sometimes exceed two gigabytes. On a 64-bit system, this is OK: > we just malloc a larger buffer. > > Signed-off-by: David Turner > --- > cache.h | 1 + > config.c | 17 +++++++++++++++++ > http.c | 2 +- > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/cache.h b/cache.h > index fbdf7a815a..a8c1b65db0 100644 > --- a/cache.h > +++ b/cache.h > @@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *); > extern int git_config_int(const char *, const char *); > extern int64_t git_config_int64(const char *, const char *); > extern unsigned long git_config_ulong(const char *, const char *); > +extern size_t git_config_size_t(const char *, const char *); > extern int git_config_bool_or_int(const char *, const char *, int *); > extern int git_config_bool(const char *, const char *); > extern int git_config_maybe_bool(const char *, const char *); > diff --git a/config.c b/config.c > index 1a4d85537b..7b706cf27a 100644 > --- a/config.c > +++ b/config.c > @@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned long *ret) > return 1; > } > > +static size_t git_parse_size_t(const char *value, unsigned long *ret) > +{ > + size_t tmp; > + if (!git_parse_signed(value, &tmp, maximum_unsigned_value_of_type(size_t))) I am getting these: config.c:840:2: error: pointer targets in passing argument 2 of 'git_parse_signed' differ in signedness [-Werror=pointer-sign] if (!git_parse_signed(value, &tmp, maximum_unsigned_value_of_type(size_t))) ^ config.c:753:12: note: expected 'intmax_t *' but argument is of type 'size_t *' static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max) ^ Changing "size_t tmp" to "intmax_t tmp" squelches it but the maximum unsigned value of size_t type would probably overflow "intmax_t max" which is signed, so... ``` ## Torsten Bögershausen, 2017-03-31 04:22 Subject: Re: [PATCH] http.postbuffer: make a size_t Message-ID: URL: https://gitlist.dev/e/d312abfe-f87d-3940-ecfa-348fed7ad3a1%40web.de In-Reply-To: <20170330202917.24281-1-dturner@twosigma.com> ``` On 30/03/17 22:29, David Turner wrote: > Unfortunately, in order to push some large repos, the http postbuffer > must sometimes exceed two gigabytes. On a 64-bit system, this is OK: > we just malloc a larger buffer. > > Signed-off-by: David Turner > --- > cache.h | 1 + > config.c | 17 +++++++++++++++++ > http.c | 2 +- > 3 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/cache.h b/cache.h > index fbdf7a815a..a8c1b65db0 100644 > --- a/cache.h > +++ b/cache.h > @@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *); > extern int git_config_int(const char *, const char *); > extern int64_t git_config_int64(const char *, const char *); > extern unsigned long git_config_ulong(const char *, const char *); > +extern size_t git_config_size_t(const char *, const char *); > extern int git_config_bool_or_int(const char *, const char *, int *); > extern int git_config_bool(const char *, const char *); > extern int git_config_maybe_bool(const char *, const char *); > diff --git a/config.c b/config.c > index 1a4d85537b..7b706cf27a 100644 > --- a/config.c > +++ b/config.c > @@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned long *ret) > return 1; > } > > +static size_t git_parse_size_t(const char *value, unsigned long *ret) > +{ > + size_t tmp; > + if (!git_parse_signed(value, &tmp, maximum_unsigned_value_of_type(size_t))) > + return 0; > + *ret = tmp; > + return 1; > +} What is the return value here ? Isn't it a size_t we want ? (There was a recent discussion about "unsigned long" vs "size_t", which are the same on many systems, but not under Win64) Would the following work ? static int git_parse_size_t(const char *value, size_t *ret) { if (!git_parse_signed(value, ret, maximum_unsigned_value_of_type(size_t))) return 0; return 1; } [] > +size_t git_config_size_t(const char *name, const char *value) > +{ > + unsigned long ret; > + if (!git_parse_size_t(value, &ret)) > + die_bad_number(name, value); > + return ret; > +} > + Same here: size_t git_config_size_t(const char *name, const char *value) { size_t ret; if (!git_parse_size_t(value, &ret)) die_bad_number(name, value); return ret; } ``` ## David Turner, 2017-03-31 15:52 Subject: RE: [PATCH] http.postbuffer: make a size_t Message-ID: <8d9af9884ccc4f99a01da863cb7ba600@exmbdft7.ad.twosigma.com> URL: https://gitlist.dev/e/8d9af9884ccc4f99a01da863cb7ba600%40exmbdft7.ad.twosigma.com In-Reply-To: ``` > -----Original Message----- > From: Torsten Bögershausen [mailto:tboegi@web.de] > Sent: Friday, March 31, 2017 12:22 AM > To: David Turner ; git@vger.kernel.org > Subject: Re: [PATCH] http.postbuffer: make a size_t > > > > On 30/03/17 22:29, David Turner wrote: > > Unfortunately, in order to push some large repos, the http postbuffer > > must sometimes exceed two gigabytes. On a 64-bit system, this is OK: > > we just malloc a larger buffer. > > > > Signed-off-by: David Turner > > --- > > cache.h | 1 + > > config.c | 17 +++++++++++++++++ > > http.c | 2 +- > > 3 files changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/cache.h b/cache.h > > index fbdf7a815a..a8c1b65db0 100644 > > --- a/cache.h > > +++ b/cache.h > > @@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *); > > extern int git_config_int(const char *, const char *); extern int64_t > > git_config_int64(const char *, const char *); extern unsigned long > > git_config_ulong(const char *, const char *); > > +extern size_t git_config_size_t(const char *, const char *); > > extern int git_config_bool_or_int(const char *, const char *, int *); > > extern int git_config_bool(const char *, const char *); extern int > > git_config_maybe_bool(const char *, const char *); diff --git > > a/config.c b/config.c index 1a4d85537b..7b706cf27a 100644 > > --- a/config.c > > +++ b/config.c > > @@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned > long *ret) > > return 1; > > } > > > > +static size_t git_parse_size_t(const char *value, unsigned long *ret) > > +{ > > + size_t tmp; > > + if (!git_parse_signed(value, &tmp, > maximum_unsigned_value_of_type(size_t))) > > + return 0; > > + *ret = tmp; > > + return 1; > > +} > What is the return value here ? > Isn't it a size_t we want ? Yeah, that should return an int, since it's just "parsed" vs "unparsed". > (There was a recent discussion about "unsigned long" vs "size_t", which > are the same on many systems, but not under Win64) Would the following > work ? > > static int git_parse_size_t(const char *value, size_t *ret) { > if (!git_parse_signed(value, ret, > maximum_unsigned_value_of_type(size_t))) > return 0; > return 1; > } > > [] > > +size_t git_config_size_t(const char *name, const char *value) { > > + unsigned long ret; > > + if (!git_parse_size_t(value, &ret)) > > + die_bad_number(name, value); > > + return ret; > > +} > > + > Same here: > size_t git_config_size_t(const char *name, const char *value) { > size_t ret; > if (!git_parse_size_t(value, &ret)) > die_bad_number(name, value); > return ret; > } In v2 I decided to make this a ssize_t, since it was previously a signed int. I know we're not using negative values right now, but since nobody has 2**63 bytes of ram anyway, it's probably safe to keep it signed to keep our options open. ```