Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Max Kirillov" <max@max630.net>,
	git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Jelmer Vernooĳ" <jelmer@jelmer.uk>,
	"Florian Manschwetus" <manschwetus@cs-software-gmbh.de>
Subject: Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
Date: Tue, 11 Sep 2018 11:15:13 -0700	[thread overview]
Message-ID: <xmqqk1nrq4su.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180911040343.GC20518@aiede.svl.corp.google.com> (Jonathan Nieder's message of "Mon, 10 Sep 2018 21:03:43 -0700")

Jonathan Nieder <jrnieder@gmail.com> 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"))
>
> ?

Hmph, that's 

	"Transfer-Encoding" ":" 1#transfer-coding

where #rule is

   #rule
      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
         1#element

So

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

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.

Thanks.