Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Max Kirillov <max@max630.net>
Cc: "Jelmer Vernooij" <jelmer@jelmer.uk>,
	git@vger.kernel.org, "Jeff King" <peff@peff.net>
Subject: Re: [PATCH] http-backend: allow empty CONTENT_LENGTH
Date: Thu, 6 Sep 2018 15:45:05 -0700
Message-ID: <20180906224505.GA81412@aiede.svl.corp.google.com> (raw)
In-Reply-To: <20180906193516.28909-1-max@max630.net>

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

  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 [this message]
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
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 \
    --in-reply-to=20180906224505.GA81412@aiede.svl.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jelmer@jelmer.uk \
    --cc=max@max630.net \
    --cc=peff@peff.net \
    /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 \
		git@vger.kernel.org
	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