[v2] http-backend: allow empty CONTENT_LENGTH
diff mbox series

Message ID 20180907033607.24604-1-max@max630.net
State New
Headers show
Series
  • [v2] http-backend: allow empty CONTENT_LENGTH
Related show

Commit Message

Max Kirillov Sept. 7, 2018, 3:36 a.m. UTC
According to RFC3875, empty environment variable is equivalent to unset,
and for CONTENT_LENGTH it should mean zero body to read.

However, unset CONTENT_LENGTH is also used for chunked encoding to indicate
reading until EOF. At least, the test "large fetch-pack requests can be split
across POSTs" from t5551 starts faliing, if unset or empty CONTENT_LENGTH is
treated as zero length body. So keep the existing behavior as much as possible.

Add a test for the case.

Reported-By: Jelmer Vernooń≥ <jelmer@jelmer.uk>
Signed-off-by: Max Kirillov <max@max630.net>
---
Added the "reported-by" and explained inline the reason to keep existing behavior
 http-backend.c                         |  2 +-
 t/t5562-http-backend-content-length.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Jonathan Nieder Sept. 8, 2018, 12:19 a.m. UTC | #1
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, unset CONTENT_LENGTH is also used for chunked encoding to indicate
> reading until EOF. At least, the test "large fetch-pack requests can be split
> across POSTs" from t5551 starts faliing, if unset or empty CONTENT_LENGTH is
> treated as zero length body. So keep the existing behavior as much as possible.
>
> Add a test for the case.
>
> Reported-By: Jelmer Vernooń≥ <jelmer@jelmer.uk>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> Added the "reported-by" and explained inline the reason to keep existing behavior

Lovely, thanks.

To me, "keep the existing behavior as much as possible" isn't comforting
because it doesn't tell me *which* existing behavior.  Fortunately the patch
itself is comforting: it makes us treat "" the same way as unset, which is
exactly what the RFC requires.

So I'm happy with this version.  Thanks for your patient work.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Patch
diff mbox series

diff --git a/http-backend.c b/http-backend.c
index e88d29f62b..a1230d7ead 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -353,7 +353,7 @@  static ssize_t get_content_length(void)
 	ssize_t val = -1;
 	const char *str = getenv("CONTENT_LENGTH");
 
-	if (str && !git_parse_ssize_t(str, &val))
+	if (str && *str && !git_parse_ssize_t(str, &val))
 		die("failed to parse CONTENT_LENGTH: %s", str);
 	return val;
 }
diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh
index 057dcb85d6..ca34c2f054 100755
--- 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' '
 	grep "fatal:.*CONTENT_LENGTH" err
 '
 
+test_expect_success 'empty CONTENT_LENGTH' '
+	env \
+		QUERY_STRING=/repo.git/HEAD \
+		PATH_TRANSLATED="$PWD"/.git/HEAD \
+		GIT_HTTP_EXPORT_ALL=TRUE \
+		REQUEST_METHOD=GET \
+		CONTENT_LENGTH="" \
+		git http-backend <empty_body >act.out 2>act.err &&
+	verify_http_result "200 OK"
+'
+
 test_done