From: Junio C Hamano <firstname.lastname@example.org> To: Jeff King <email@example.com> Cc: "Jonathan Nieder" <firstname.lastname@example.org>, "Max Kirillov" <email@example.com>, firstname.lastname@example.org, "Jelmer Vernooĳ" <email@example.com>, "Florian Manschwetus" <firstname.lastname@example.org> Subject: Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH Date: Mon, 10 Sep 2018 09:37:20 -0700 Message-ID: <email@example.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 <firstname.lastname@example.org> 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.
next prev parent reply index Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-06 6:10 CONTENT_LENGTH can no longer be empty Jonathan Nieder 2018-09-06 19:35 ` [PATCH] http-backend: allow empty CONTENT_LENGTH Max Kirillov 2018-09-06 21:54 ` Junio C Hamano 2018-09-07 3:27 ` Max Kirillov 2018-09-07 3:38 ` Jeff King 2018-09-07 4:20 ` Max Kirillov 2018-09-07 4:59 ` Max Kirillov 2018-09-07 9:49 ` Junio C Hamano 2018-09-08 5:41 ` Max Kirillov 2018-09-09 4:40 ` Max Kirillov 2018-09-06 22:45 ` Jonathan Nieder 2018-09-07 3:36 ` [PATCH v2] " Max Kirillov 2018-09-09 4:10 ` [PATCH v4] " Max Kirillov 2018-09-10 5:25 ` Jonathan Nieder 2018-09-10 13:17 ` Jeff King 2018-09-10 16:37 ` Junio C Hamano [this message] 2018-09-10 18:46 ` Jeff King 2018-09-10 20:53 ` [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero Max Kirillov 2018-09-10 21:22 ` Jonathan Nieder 2018-09-11 1:55 ` Jeff King 2018-09-11 2:20 ` Jonathan Nieder 2018-09-11 2:30 ` Jeff King 2018-09-11 1:58 ` Jeff King 2018-09-11 3:42 ` [PATCH] http-backend: treat " Jonathan Nieder 2018-09-11 4:03 ` Jonathan Nieder 2018-09-11 18:15 ` Junio C Hamano 2018-09-11 18:27 ` Junio C Hamano 2018-09-12 5:56 ` Jeff King 2018-09-12 6:26 ` Jonathan Nieder 2018-09-12 16:10 ` Junio C Hamano 2018-09-11 4:18 ` Junio C Hamano 2018-09-11 4:29 ` Jonathan Nieder 2018-09-08 0:19 [PATCH v2] http-backend: allow empty CONTENT_LENGTH Jonathan Nieder 2018-09-08 5:35 ` Max Kirillov 2018-09-08 5:42 ` [PATCH v3] " Max Kirillov 2018-09-10 5:17 ` Jonathan Nieder 2018-09-10 20:36 ` Max Kirillov 2018-09-11 4:06 Jonathan Nieder 2018-09-11 20:33 ` [PATCH v2] http-backend test: make empty CONTENT_LENGTH test more realistic Max Kirillov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Git Mailing List Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/git/0 git/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 git git/ https://lore.kernel.org/git \ email@example.com public-inbox-index git Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git