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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Max Kirillov wrote:

> 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 Vernooij <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