Re: [PATCH] http-backend: allow empty CONTENT_LENGTH

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 6, 2018, 22:45 UTC
Message-ID: <20180906224505.GA81412@aiede.svl.corp.google.com>
In-Reply-To: <20180906193516.28909-1-max@max630.net>

Hi,

Max Kirillov wrote:

Show 12 quoted lines

> According to RFC3875, empty environment variable is equivalent to unset,
> and for CONTENT_LENGTH it should mean zero body to read.
>
> However, as discussed in [1], unset CONTENT_LENGTH is also used for
> chunked encoding to indicate reading until EOF, so keep this behavior also
> for empty CONTENT_LENGTH.
>
> Add a test for the case.
>
> [1] https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/
>
> Signed-off-by: Max Kirillov <max@max630.net>

Reported-by: Jelmer Vernooĳ <jelmer@jelmer.uk>

Thanks for fixing it.

Can you include a summary of [1] instead of relying on the mailing list archive? Perhaps just omiting "as discussed in [1]" would do the trick. Alternatively, if there's a point from that discussion that's relevant to the change, please include it here. That way, people finding this change later can save some time by avoiding having to dig through that mailing list thread.

For example, it's probably worth mentioning that this was discovered using dulwich's test suite.

[...]

> This should fix it. I'm not sure should it treat it as 0 or "-1"
> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as "-1"
> So keep the existing behavior as much as possible

That sounds worth figuring out so we can understand and possibly document it better. What are the ramifications of this choice --- what would work / not work with each choice?

[...]

> --- a/t/t5562-http-backend-content-length.sh
> +++ b/t/t5562-http-backend-content-length.sh
> @@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '

Yay, thanks for this as well.

Sincerely, Jonathan