Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
- Date: Tue, 11 Sep 2018 11:15:13 -0700
- From: Junio C Hamano <gitster@xxxxxxxxx>
- Subject: Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> Kicking off the reviews: ;-)
> Jonathan Nieder wrote:
>> --- a/http-backend.c
>> +++ b/http-backend.c
>> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o
>> static ssize_t get_content_length(void)
>> + /*
>> + * According to RFC 3875, an empty or missing
>> + * CONTENT_LENGTH means "no body", but RFC 3875
>> + * precedes HTTP/1.1 and chunked encoding. Apache and
>> + * its imitators leave CONTENT_LENGTH unset for
> Which imitators? Maybe this should just say "Apache leaves [...]".
I tend to agree; I do not mind amending the text while queuing.
>> + * chunked requests, for which we should use EOF to
>> + * detect the end of the request.
>> + */
>> + str = getenv("HTTP_TRANSFER_ENCODING");
>> + if (str && !strcmp(str, "chunked"))
> RFC 2616 says Transfer-Encoding is a list of transfer-codings applied,
> in the order that they were applied, and that "chunked" is always
> applied last. That means a transfer-encoding like
> Transfer-Encoding: identity chunked
> would be permitted, or e.g.
> Transfer-Encoding: gzip chunked
> Does that means we should be using a check like
> str && (!strcmp(str, "chunked") || ends_with(str, " chunked"))
"Transfer-Encoding" ":" 1#transfer-coding
where #rule is
A construct "#" is defined, similar to "*", for defining lists of
elements. The full form is "<n>#<m>element" indicating at least
<n> and at most <m> elements, each separated by one or more commas
(",") and OPTIONAL linear white space (LWS). This makes the usual
form of lists very easy; a rule such as
( *LWS element *( *LWS "," *LWS element ))
can be shown as
- you need to account for comma
- your LWS may not be a SP
if you want to handle gzipped stream coming in a chunked form, I
Unless I am missing the rule in CGI spec that is used to transform
the value on the Transfer-Encoding header to HTTP_TRANSFER_ENCODING
environment variable, that is.
> That said, a quick search of codesearch.debian.net mostly finds
> examples using straight comparison, so maybe the patch is fine as-is.
I do not think we would mind terribly if we do not support
combinations like gzipped-and-then-chunked from day one. An in-code
NEEDSWORK comment that refers to the production in RFC 2616 Page 143
may not hurt, though.