git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 01/20] t5541: run "used receive-pack service" test earlier
@ 2023-02-28 23:38 Junio C Hamano
  2023-02-28 23:38 ` [PATCH 02/20] t5541: stop marking "used receive-pack service" test as v0 only Junio C Hamano
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

There's a test in t5541 that confirms that "git push" makes two requests
(a GET to /info/refs, and a POST to /git-receive-pack). However, it's a
noop unless GIT_TEST_PROTOCOL_VERSION is set to "0", due to 8a1b0978ab
(test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate,
2019-12-23).

This means that almost nobody runs it. And indeed, it has been broken
since b0c4adcdd7 (remote-curl: send Accept-Language header to server,
2022-07-11). But the fault is not in that commit, but in how brittle the
test is. It runs after several operations have been performed, which
means that it expects to see the complete set of requests made so far in
the script. Commit b0c4adcdd7 added a new test, which means that the
"used receive-pack service" test must be updated, too.

Let's fix this by making the test less brittle. We'll move it higher in
the script, right after the first push has completed. And we'll clear
the access log right before doing the push, so we'll see only the
requests from that command.

This is technically testing less, in that we won't check that all of
those other requests also correctly used smart http. But there's no
particular reason think that if the first one did, the others wouldn't.

After this patch, running:

  GIT_TEST_PROTOCOL_VERSION=0 ./t5541-http-push-smart.sh

passes, whereas it did not before.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5541-http-push-smart.sh | 44 ++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index fbad2d5ff5..ef39d14ed2 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -41,10 +41,6 @@ GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
 POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
 EOF
 test_expect_success 'no empty path components' '
-	# Clear the log, so that it does not affect the "used receive-pack
-	# service" test which reads the log too.
-	test_when_finished ">\"\$HTTPD_ROOT_PATH\"/access.log" &&
-
 	# In the URL, add a trailing slash, and see if git appends yet another
 	# slash.
 	cd "$ROOT_PATH" &&
@@ -67,6 +63,10 @@ test_expect_success 'clone remote repository' '
 '
 
 test_expect_success 'push to remote repository (standard)' '
+	# Clear the log, so that the "used receive-pack service" test below
+	# sees just what we did here.
+	>"$HTTPD_ROOT_PATH"/access.log &&
+
 	cd "$ROOT_PATH"/test_repo_clone &&
 	: >path2 &&
 	git add path2 &&
@@ -80,6 +80,20 @@ test_expect_success 'push to remote repository (standard)' '
 	 test $HEAD = $(git rev-parse --verify HEAD))
 '
 
+test_expect_success 'used receive-pack service' '
+	cat >exp <<-\EOF &&
+	GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
+	POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
+	EOF
+
+	# NEEDSWORK: If the overspecification of the expected result is reduced, we
+	# might be able to run this test in all protocol versions.
+	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
+	then
+		check_access_log exp
+	fi
+'
+
 test_expect_success 'push to remote repository (standard) with sending Accept-Language' '
 	cat >exp <<-\EOF &&
 	=> Send header: Accept-Language: ko-KR, *;q=0.9
@@ -141,28 +155,6 @@ test_expect_success 'rejected update prints status' '
 '
 rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update"
 
-cat >exp <<EOF
-GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
-GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
-GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
-GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
-GET  /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
-EOF
-test_expect_success 'used receive-pack service' '
-	# NEEDSWORK: If the overspecification of the expected result is reduced, we
-	# might be able to run this test in all protocol versions.
-	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
-	then
-		check_access_log exp
-	fi
-'
-
 test_http_push_nonff "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
 	"$ROOT_PATH"/test_repo_clone main 		success
 
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 02/20] t5541: stop marking "used receive-pack service" test as v0 only
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
@ 2023-02-28 23:38 ` Junio C Hamano
  2023-02-28 23:38 ` [PATCH 03/20] t5541: simplify and move "no empty path components" test Junio C Hamano
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

We have a test which checks to see if a request to git-receive-pack was
made. Originally, it was checking the entire set of requests made in the
script so far, including clones, and thus it would break when run with
the v2 protocol (since that implies an extra request for fetches).

Since the previous commit, though, we are only checking the requests
made by a single push. And since there is no v2 push protocol, the test
now passes no matter what's in GIT_TEST_PROTOCOL_VERSION. We can just
run it all the time.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5541-http-push-smart.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index ef39d14ed2..f8bf533c33 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -86,12 +86,7 @@ test_expect_success 'used receive-pack service' '
 	POST /smart/test_repo.git/git-receive-pack HTTP/1.1 200
 	EOF
 
-	# NEEDSWORK: If the overspecification of the expected result is reduced, we
-	# might be able to run this test in all protocol versions.
-	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
-	then
-		check_access_log exp
-	fi
+	check_access_log exp
 '
 
 test_expect_success 'push to remote repository (standard) with sending Accept-Language' '
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 03/20] t5541: simplify and move "no empty path components" test
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
  2023-02-28 23:38 ` [PATCH 02/20] t5541: stop marking "used receive-pack service" test as v0 only Junio C Hamano
@ 2023-02-28 23:38 ` Junio C Hamano
  2023-02-28 23:38 ` [PATCH 04/20] t5551: drop redundant grep for Accept-Language Junio C Hamano
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

Commit 9ee6bcd398 (t5541-http-push: add test for URLs with trailing
slash, 2010-04-08) added a test that clones a URL with a trailing slash,
and confirms that we don't send a doubled slash (like "$url//info/refs")
to the server.

But this test makes no sense in t5541, which is about pushing. It should
have been added in t5551. Let's move it there.

But putting it at the end is tricky, since it checks the entire contents
of the Apache access log. We could get around this by clearing the log
before our test. But there's an even simpler solution: just make sure no
doubled slashes appear in the log (fortunately, "http://" does not
appear in the log itself).

As a bonus, this also lets us drop the check for the v0 protocol (which
is otherwise necessary since v2 makes multiple requests, and
check_access_log insists on exactly matching the number of requests,
even though we don't care about that here).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5541-http-push-smart.sh  | 18 ------------------
 t/t5551-http-fetch-smart.sh |  9 +++++++++
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index f8bf533c33..d0211cd8be 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -36,24 +36,6 @@ test_expect_success 'setup remote repository' '
 
 setup_askpass_helper
 
-cat >exp <<EOF
-GET  /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
-POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200
-EOF
-test_expect_success 'no empty path components' '
-	# In the URL, add a trailing slash, and see if git appends yet another
-	# slash.
-	cd "$ROOT_PATH" &&
-	git clone $HTTPD_URL/smart/test_repo.git/ test_repo_clone &&
-
-	# NEEDSWORK: If the overspecification of the expected result is reduced, we
-	# might be able to run this test in all protocol versions.
-	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
-	then
-		check_access_log exp
-	fi
-'
-
 test_expect_success 'clone remote repository' '
 	rm -rf test_repo_clone &&
 	git clone $HTTPD_URL/smart/test_repo.git test_repo_clone &&
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index bc0719a4fc..10b7e7cda2 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -666,4 +666,13 @@ test_expect_success 'push warns or fails when using username:password' '
 	test_line_count -ge 1 warnings
 '
 
+test_expect_success 'no empty path components' '
+	# In the URL, add a trailing slash, and see if git appends yet another
+	# slash.
+	git clone $HTTPD_URL/smart/repo.git/ clone-with-slash &&
+
+	strip_access_log >log &&
+	! grep "//" log
+'
+
 test_done
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 04/20] t5551: drop redundant grep for Accept-Language
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
  2023-02-28 23:38 ` [PATCH 02/20] t5541: stop marking "used receive-pack service" test as v0 only Junio C Hamano
  2023-02-28 23:38 ` [PATCH 03/20] t5541: simplify and move "no empty path components" test Junio C Hamano
@ 2023-02-28 23:38 ` Junio C Hamano
  2023-02-28 23:38 ` [PATCH 05/20] t5551: lower-case headers in expected curl trace Junio C Hamano
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

Commit b0c4adcdd7 (remote-curl: send Accept-Language header to server,
2022-07-11) added tests to make sure the header is sent via HTTP.
However, it checks in two places:

  1. In the expected trace output, we check verbatim for the header and
     its value.

  2. Afterwards, we grep for the header again in the trace file.

This (2) is probably cargo-culted from the earlier grep for
Accept-Encoding. It is needed for the encoding because we smudge the
value of that header when doing the verbatim check; see 1a53e692af
(remote-curl: accept all encodings supported by curl, 2018-05-22).

But we don't do so for the language header, so any problem that the
"grep" would catch in (2) would already have been caught by (1).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 10b7e7cda2..29d489768e 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -103,10 +103,7 @@ test_expect_success 'clone http repository' '
 		test_cmp exp actual.smudged &&
 
 		grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
-		test_line_count = 2 actual.gzip &&
-
-		grep "Accept-Language: ko-KR, *" actual >actual.language &&
-		test_line_count = 2 actual.language
+		test_line_count = 2 actual.gzip
 	fi
 '
 
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 05/20] t5551: lower-case headers in expected curl trace
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (2 preceding siblings ...)
  2023-02-28 23:38 ` [PATCH 04/20] t5551: drop redundant grep for Accept-Language Junio C Hamano
@ 2023-02-28 23:38 ` Junio C Hamano
  2023-02-28 23:38 ` [PATCH 06/20] t5551: handle HTTP/2 when checking " Junio C Hamano
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

There's a test in t5551 which checks the curl trace (after simplifying
it a bit). It doesn't work with HTTP/2, because in that case curl
outputs all of the headers in lower-case. Even though this test is run
with HTTP/2 by t5559, nobody has noticed because checking the trace only
happens if GIT_TEST_PROTOCOL_VERSION is manually set to "0".

Let's fix this by lower-casing all of the header names in the trace, and
then checking for those in our expected code (this is easier than making
HTTP/2 traces look like HTTP/1.1, since HTTP/1.1 uses title-casing).

Sadly, we can't quite do this in our existing sed script. This works if
you have GNU sed:

  s/^\\([><]\\) \\([A-Za-z0-9-]*:\\)/\1 \L\2\E/

but \L is a GNU-ism, and I don't think there's a portable solution. We
could just "tr A-Z a-z" on the way in, of course, but that makes the
non-header parts harder to read (e.g., lowercase "post" requests). But
to paraphrase Baron Munchausen, I have learned from experience that a
modicum of Perl can be most efficacious.

Note that this doesn't quite get the test passing with t5559; there are
more fixes needed on top.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 55 ++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 29d489768e..a81f852cbf 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -35,30 +35,35 @@ setup_askpass_helper
 test_expect_success 'clone http repository' '
 	cat >exp <<-\EOF &&
 	> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
-	> Accept: */*
-	> Accept-Encoding: ENCODINGS
-	> Accept-Language: ko-KR, *;q=0.9
-	> Pragma: no-cache
+	> accept: */*
+	> accept-encoding: ENCODINGS
+	> accept-language: ko-KR, *;q=0.9
+	> pragma: no-cache
 	< HTTP/1.1 200 OK
-	< Pragma: no-cache
-	< Cache-Control: no-cache, max-age=0, must-revalidate
-	< Content-Type: application/x-git-upload-pack-advertisement
+	< pragma: no-cache
+	< cache-control: no-cache, max-age=0, must-revalidate
+	< content-type: application/x-git-upload-pack-advertisement
 	> POST /smart/repo.git/git-upload-pack HTTP/1.1
-	> Accept-Encoding: ENCODINGS
-	> Content-Type: application/x-git-upload-pack-request
-	> Accept: application/x-git-upload-pack-result
-	> Accept-Language: ko-KR, *;q=0.9
-	> Content-Length: xxx
+	> accept-encoding: ENCODINGS
+	> content-type: application/x-git-upload-pack-request
+	> accept: application/x-git-upload-pack-result
+	> accept-language: ko-KR, *;q=0.9
+	> content-length: xxx
 	< HTTP/1.1 200 OK
-	< Pragma: no-cache
-	< Cache-Control: no-cache, max-age=0, must-revalidate
-	< Content-Type: application/x-git-upload-pack-result
+	< pragma: no-cache
+	< cache-control: no-cache, max-age=0, must-revalidate
+	< content-type: application/x-git-upload-pack-result
 	EOF
 
 	GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 LANGUAGE="ko_KR.UTF-8" \
 		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
 	tr '\''\015'\'' Q <err |
+	perl -pe '\''
+		s/(Send|Recv) header: ([A-Za-z0-9-]+):/
+		"$1 header: " . lc($2) . ":"
+		/e;
+	'\'' |
 	sed -e "
 		s/Q\$//
 		/^[*] /d
@@ -78,31 +83,31 @@ test_expect_success 'clone http repository' '
 			s/^/> /
 		}
 
-		/^> User-Agent: /d
-		/^> Host: /d
+		/^> user-agent: /d
+		/^> host: /d
 		/^> POST /,$ {
 			/^> Accept: [*]\\/[*]/d
 		}
-		s/^> Content-Length: .*/> Content-Length: xxx/
+		s/^> content-length: .*/> content-length: xxx/
 		/^> 00..want /d
 		/^> 00.*done/d
 
-		/^< Server: /d
-		/^< Expires: /d
-		/^< Date: /d
-		/^< Content-Length: /d
-		/^< Transfer-Encoding: /d
+		/^< server: /d
+		/^< expires: /d
+		/^< date: /d
+		/^< content-length: /d
+		/^< transfer-encoding: /d
 	" >actual &&
 
 	# NEEDSWORK: If the overspecification of the expected result is reduced, we
 	# might be able to run this test in all protocol versions.
 	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
 	then
-		sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+		sed -e "s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/" \
 				actual >actual.smudged &&
 		test_cmp exp actual.smudged &&
 
-		grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+		grep "accept-encoding:.*gzip" actual >actual.gzip &&
 		test_line_count = 2 actual.gzip
 	fi
 '
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 06/20] t5551: handle HTTP/2 when checking curl trace
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (3 preceding siblings ...)
  2023-02-28 23:38 ` [PATCH 05/20] t5551: lower-case headers in expected curl trace Junio C Hamano
@ 2023-02-28 23:38 ` Junio C Hamano
  2023-02-28 23:39 ` [PATCH 07/20] t5551: stop forcing clone to run with v0 protocol Junio C Hamano
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

We check that the curl trace of a clone has the lines we expect, but
this won't work when we run the test under t5559, because a few details
are different under HTTP/2 (but nobody noticed because it only happens
when you manually set GIT_TEST_PROTOCOL_VERSION to "0").

We can handle both HTTP protocols with a few tweaks:

  - we'll drop the HTTP "101 Switching Protocols" response, as well as
    various protocol upgrade headers. These details aren't interesting
    to us. We just want to make sure the correct protocol was used (and
    we do in the main request/response lines).

  - successful HTTP/2 responses just say "200" and not "200 OK"; we can
    normalize these

  - replace HTTP/1.1 with a variable in the request/response lines. We
    can use the existing $HTTP_PROTO for this, as it's already set to
    "HTTP/2" when appropriate. We do need to tweak the fallback value to
    "HTTP/1.1" to match what curl will write (prior to this patch, the
    fallback value didn't matter at all; we only checked if it was the
    literal string "HTTP/2").

Note that several lines still expect HTTP/1.1 unconditionally. The first
request does so because the client requests an upgrade during the
request. The POST request and response do so because you can't do an
upgrade if there is a request body. (This will all be different if we
trigger HTTP/2 via ALPN, but the tests aren't yet capable of that).

This is enough to let:

  GIT_TEST_PROTOCOL_VERSION=0 ./t5559-http-fetch-smart-http2.sh

pass the "clone http repository" test (but there are some other failures
later on).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index a81f852cbf..716c9dbb69 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-: ${HTTP_PROTO:=HTTP}
+: ${HTTP_PROTO:=HTTP/1.1}
 test_description="test smart fetching over http via http-backend ($HTTP_PROTO)"
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
@@ -33,13 +33,13 @@ test_expect_success 'create http-accessible bare repository' '
 setup_askpass_helper
 
 test_expect_success 'clone http repository' '
-	cat >exp <<-\EOF &&
+	cat >exp <<-EOF &&
 	> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 	> accept: */*
 	> accept-encoding: ENCODINGS
 	> accept-language: ko-KR, *;q=0.9
 	> pragma: no-cache
-	< HTTP/1.1 200 OK
+	< $HTTP_PROTO 200 OK
 	< pragma: no-cache
 	< cache-control: no-cache, max-age=0, must-revalidate
 	< content-type: application/x-git-upload-pack-advertisement
@@ -83,6 +83,14 @@ test_expect_success 'clone http repository' '
 			s/^/> /
 		}
 
+		/^< HTTP/ {
+			s/200$/200 OK/
+		}
+		/^< HTTP\\/1.1 101/d
+		/^[><] connection: /d
+		/^[><] upgrade: /d
+		/^> http2-settings: /d
+
 		/^> user-agent: /d
 		/^> host: /d
 		/^> POST /,$ {
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 07/20] t5551: stop forcing clone to run with v0 protocol
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (4 preceding siblings ...)
  2023-02-28 23:38 ` [PATCH 06/20] t5551: handle HTTP/2 when checking " Junio C Hamano
@ 2023-02-28 23:39 ` Junio C Hamano
  2023-02-28 23:39 ` [PATCH 08/20] t5551: handle v2 protocol when checking curl trace Junio C Hamano
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:39 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

In the "clone http repository" test, we check the curl trace to make
sure the expected requests were made. This whole script was marked to
handle only the v0 protocol in d790ee1707 (tests: fix protocol version
for overspecifications, 2019-02-25). That makes sense, since v2 requires
an extra request, so tests as specific as this would fail unless
modified.

Later, in preparation for v2 becoming the default, this was tweaked by
8a1b0978ab (test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate,
2019-12-23). There we run the trace check only if the user has
explicitly asked to test protocol version 0. But it also forced the
clone itself to run with the v0 protocol.

This makes the check for "can we expect a v0 trace" silly; it will
always be v0. But much worse, it means that the clone we are testing is
not like the one that normal users would run. They would use the
defaults, which are now v2.  And since this is supposed to be a basic
check of clone-over-http, we should do the same.

Let's fix this by dropping the extra v0 override. The test still passes
because the trace checking only kicks in if we asked to use v0
explicitly (this is the same as before; even though we were running a v0
clone, unless you specifically set GIT_TEST_PROTOCOL_VERSION=0, the
trace check was always skipped).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 716c9dbb69..4191174584 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -55,7 +55,7 @@ test_expect_success 'clone http repository' '
 	< content-type: application/x-git-upload-pack-result
 	EOF
 
-	GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 LANGUAGE="ko_KR.UTF-8" \
+	GIT_TRACE_CURL=true LANGUAGE="ko_KR.UTF-8" \
 		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
 	tr '\''\015'\'' Q <err |
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 08/20] t5551: handle v2 protocol when checking curl trace
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (5 preceding siblings ...)
  2023-02-28 23:39 ` [PATCH 07/20] t5551: stop forcing clone to run with v0 protocol Junio C Hamano
@ 2023-02-28 23:39 ` Junio C Hamano
  2023-02-28 23:39 ` [PATCH 09/20] t5551: handle v2 protocol in upload-pack service test Junio C Hamano
  2023-02-28 23:40 ` [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:39 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

After cloning an http repository, we check the curl trace to make sure
the expected requests were made. But since the expected trace was never
updated to handle v2, it is only run when you ask the test suite to run
in v0 mode (which hardly anybody does).

Let's update it to handle both protocols. This isn't too hard since v2
just sends an extra header and an extra request. So we can just annotate
those extra lines and strip them out for v0 (and drop the annotations
for v2). I didn't bother handling v1 here, as it's not really of
practical interest (it would drop the extra v2 request, but still have
the "git-protocol" lines).

There's a similar tweak needed at the end. Since we check the
"accept-encoding" value loosely, we grep for it rather than finding it
in the verbatim trace. This grep insists that there are exactly 2
matches, but of course in v2 with the extra request there are 3. We
could tweak the number, but it's simpler still to just check that we saw
at least one match. The verbatim check already confirmed how many
instances of the header we have; we're really just checking here that
"gzip" is in the value (it's possible, of course, that the headers could
have different values, but that seems like an unlikely bug).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 4191174584..9d99cefb92 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -33,12 +33,13 @@ test_expect_success 'create http-accessible bare repository' '
 setup_askpass_helper
 
 test_expect_success 'clone http repository' '
-	cat >exp <<-EOF &&
+	cat >exp.raw <<-EOF &&
 	> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1
 	> accept: */*
 	> accept-encoding: ENCODINGS
 	> accept-language: ko-KR, *;q=0.9
 	> pragma: no-cache
+	{V2} > git-protocol: version=2
 	< $HTTP_PROTO 200 OK
 	< pragma: no-cache
 	< cache-control: no-cache, max-age=0, must-revalidate
@@ -48,13 +49,32 @@ test_expect_success 'clone http repository' '
 	> content-type: application/x-git-upload-pack-request
 	> accept: application/x-git-upload-pack-result
 	> accept-language: ko-KR, *;q=0.9
+	{V2} > git-protocol: version=2
 	> content-length: xxx
 	< HTTP/1.1 200 OK
 	< pragma: no-cache
 	< cache-control: no-cache, max-age=0, must-revalidate
 	< content-type: application/x-git-upload-pack-result
+	{V2} > POST /smart/repo.git/git-upload-pack HTTP/1.1
+	{V2} > accept-encoding: ENCODINGS
+	{V2} > content-type: application/x-git-upload-pack-request
+	{V2} > accept: application/x-git-upload-pack-result
+	{V2} > accept-language: ko-KR, *;q=0.9
+	{V2} > git-protocol: version=2
+	{V2} > content-length: xxx
+	{V2} < HTTP/1.1 200 OK
+	{V2} < pragma: no-cache
+	{V2} < cache-control: no-cache, max-age=0, must-revalidate
+	{V2} < content-type: application/x-git-upload-pack-result
 	EOF
 
+	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
+	then
+		sed "/^{V2}/d" <exp.raw >exp
+	else
+		sed "s/^{V2} //" <exp.raw >exp
+	fi &&
+
 	GIT_TRACE_CURL=true LANGUAGE="ko_KR.UTF-8" \
 		git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&
 	test_cmp file clone/file &&
@@ -107,17 +127,11 @@ test_expect_success 'clone http repository' '
 		/^< transfer-encoding: /d
 	" >actual &&
 
-	# NEEDSWORK: If the overspecification of the expected result is reduced, we
-	# might be able to run this test in all protocol versions.
-	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
-	then
-		sed -e "s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/" \
-				actual >actual.smudged &&
-		test_cmp exp actual.smudged &&
+	sed -e "s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/" \
+			actual >actual.smudged &&
+	test_cmp exp actual.smudged &&
 
-		grep "accept-encoding:.*gzip" actual >actual.gzip &&
-		test_line_count = 2 actual.gzip
-	fi
+	grep "accept-encoding:.*gzip" actual >actual.gzip
 '
 
 test_expect_success 'fetch changes via http' '
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 09/20] t5551: handle v2 protocol in upload-pack service test
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (6 preceding siblings ...)
  2023-02-28 23:39 ` [PATCH 08/20] t5551: handle v2 protocol when checking curl trace Junio C Hamano
@ 2023-02-28 23:39 ` Junio C Hamano
  2023-02-28 23:40 ` [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:39 UTC (permalink / raw)
  To: git; +Cc: Jeff King

From: Jeff King <peff@peff.net>

We perform a clone and a fetch, and then check that we saw the expected
requests in Apache's access log. In the v2 protocol, there will be one
extra request to /git-upload-pack for each operation (since the initial
/info/refs probe is just used to upgrade the protocol).

As a result, this test is a noop unless the use of the v0 protocol is
forced. Which means that hardly anybody runs it, since you have to do so
manually.

Let's update it to handle v2 and run it always. We could do this by just
conditionally adding in the extra POST lines. But if we look at the
origin of the test in 7da4e2280c (test smart http fetch and push,
2009-10-30), the point is really just to make sure that the smart
git-upload-pack service was used at all. So rather than counting up the
individual requests, let's just make sure we saw each of the expected
types. This is a bit looser, but makes maintenance easier.

Since we're now matching with grep, we can also loosen the HTTP/1.1
match, which allows this test to pass when run with HTTP/2 via t5559.
That lets:

  GIT_TEST_PROTOCOL_VERSION=0 ./t5559-http-fetch-smart-http2.sh

run to completion, which previously failed (and of course it works if
you use v2, as well).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5551-http-fetch-smart.sh | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 9d99cefb92..b912958518 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -143,19 +143,9 @@ test_expect_success 'fetch changes via http' '
 '
 
 test_expect_success 'used upload-pack service' '
-	cat >exp <<-\EOF &&
-	GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
-	POST /smart/repo.git/git-upload-pack HTTP/1.1 200
-	GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200
-	POST /smart/repo.git/git-upload-pack HTTP/1.1 200
-	EOF
-
-	# NEEDSWORK: If the overspecification of the expected result is reduced, we
-	# might be able to run this test in all protocol versions.
-	if test "$GIT_TEST_PROTOCOL_VERSION" = 0
-	then
-		check_access_log exp
-	fi
+	strip_access_log >log &&
+	grep "GET  /smart/repo.git/info/refs?service=git-upload-pack HTTP/[0-9.]* 200" log &&
+	grep "POST /smart/repo.git/git-upload-pack HTTP/[0-9.]* 200" log
 '
 
 test_expect_success 'follow redirects (301)' '
-- 
2.40.0-rc0-32-ga0f05f6840


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 01/20] t5541: run "used receive-pack service" test earlier
  2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
                   ` (7 preceding siblings ...)
  2023-02-28 23:39 ` [PATCH 09/20] t5551: handle v2 protocol in upload-pack service test Junio C Hamano
@ 2023-02-28 23:40 ` Junio C Hamano
  8 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2023-02-28 23:40 UTC (permalink / raw)
  To: git; +Cc: Jeff King

Junio C Hamano <gitster@pobox.com> writes:

> From: Jeff King <peff@peff.net>

Sorry for these patches that were sent out by accident.  Please
disregard.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-02-28 23:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 23:38 [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano
2023-02-28 23:38 ` [PATCH 02/20] t5541: stop marking "used receive-pack service" test as v0 only Junio C Hamano
2023-02-28 23:38 ` [PATCH 03/20] t5541: simplify and move "no empty path components" test Junio C Hamano
2023-02-28 23:38 ` [PATCH 04/20] t5551: drop redundant grep for Accept-Language Junio C Hamano
2023-02-28 23:38 ` [PATCH 05/20] t5551: lower-case headers in expected curl trace Junio C Hamano
2023-02-28 23:38 ` [PATCH 06/20] t5551: handle HTTP/2 when checking " Junio C Hamano
2023-02-28 23:39 ` [PATCH 07/20] t5551: stop forcing clone to run with v0 protocol Junio C Hamano
2023-02-28 23:39 ` [PATCH 08/20] t5551: handle v2 protocol when checking curl trace Junio C Hamano
2023-02-28 23:39 ` [PATCH 09/20] t5551: handle v2 protocol in upload-pack service test Junio C Hamano
2023-02-28 23:40 ` [PATCH 01/20] t5541: run "used receive-pack service" test earlier Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).