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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 10, 2018, 16:37 UTC
Message-ID: <xmqqa7opux4v.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180910131724.GA5233@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> But that couldn't have been what older versions were doing, since they
> never looked at CONTENT_LENGTH at all, and instead always read to EOF.
> So presumably the original problem wasn't that we tried to read a body,
> but that the empty string caused git_parse_ssize_t to report failure,
> and we called die(). Which probably should be explained by 574c513e8d
> (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too
> late for that.
>
> So after that patch, we really do have the original behavior, and that's
> enough for v2.19.

To recap to make sure I am following it correctly:

 - pay attention to content-length when it is clearly given with a
   byte count, which is an improvement over v2.18

 - mimick what we have been doing until now when content-length is
   missing or set to an empty string, so we are regression free and
   bug-to-bug compatible relative to v2.18 in these two cases.

Show 8 quoted lines

> But the remaining question then is: what should clients expect on an
> empty variable? We know what the RFC says, and we know what dulwich
> expected, but I'm not sure we have real world cases beyond that. So it
> might actually make sense to punt until we see one, though I don't mind
> doing what the rfc says in the meantime. And then the explanation in the
> commit message would be "do what the rfc says", and any test probably
> ought to be feeding a non-empty empty and confirming that we don't read
> it.

The RFC is pretty clear that no data is signaled by "NULL (or unset)", meaning an empty string value and missing variable both mean the same "no message body", but it further says that the servers MUST set CONTENT_LENGTH if and only if there is a message-body, which contradicts with itself (if you adhered to 'if and only if', in no case you would set it to NULL).

Googling "cgi chunked encoding" seems to give us tons of hits to show that people are puzzled, just like us, that the scripts would not get to see Chunked (as the server is supposed to deChunk to count content-length before calling the backend). So I agree "do what the rfc says" is a good thing to try early in the next cycle.