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

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Jonathan Nieder" <jrnieder@gmail.com>,
	"Max Kirillov" <max@max630.net>,
	git@vger.kernel.org, "Jelmer Vernooĳ" <jelmer@jelmer.uk>,
	"Florian Manschwetus" <manschwetus@cs-software-gmbh.de>
Subject: Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH
Date: Mon, 10 Sep 2018 09:37:20 -0700	[thread overview]
Message-ID: <xmqqa7opux4v.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180910131724.GA5233@sigill.intra.peff.net> (Jeff King's message of "Mon, 10 Sep 2018 09:17:25 -0400")

Jeff King <peff@peff.net> writes:

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

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