{"thread":{"id":"59298","subject":"[PATCH 0/16] http test bug potpourri","startedAt":"2023-02-23T10:48:07Z","lastAt":"2023-02-24T03:01:21Z","messageCount":24,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":16},"messages":[{"id":"472526","messageId":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","threadId":"59298","inReplyTo":null,"subject":"[PATCH 0/16] http test bug potpourri","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:48:01Z","receivedAt":"2023-02-23T10:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"So all I wanted to do was this one-liner:\n\ndiff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\nindex 9eece71c2c..54aa9d3bff 100755\n--- a/t/t5559-http-fetch-smart-http2.sh\n+++ b/t/t5559-http-fetch-smart-http2.sh\n@@ -1,4 +1,5 @@\n #!/bin/sh\n \n HTTP_PROTO=HTTP/2\n+LIB_HTTPD_SSL=1\n . ./t5551-http-fetch-smart.sh\n\nbut somehow I'm 16 patches deep. Let me back up.\n\nI got bit once again by the \"oops, HTTP/2 tests in t5559 are sometimes\nflaky\" bug. One thing that came up in earlier discussion is that HTTP/2\nover TLS should be much more reliable, because it doesn't have to go\nthrough the funny HTTP-upgrade path.\n\nHence the patch above, which is also patch 16 here. And it does make the\nconsistent failure of t5551.30 go away. And it even makes --stress work\nlonger before a racy failure, though it still fails for me pretty\nconsistently within a few dozen runs.\n\nBut in doing so, I found out all sorts of neat things, like:\n\n - when I tested with HTTP/2 and TLS before, I was accidentally not\n using HTTP/2!\n\n - we even have a test that should detect which version is used, but\n it's a silent noop unless you set GIT_TEST_PROTOCOL_VERSION=0, which\n clearly nobody does\n\n - it turns out there are a bunch of tests which are skipped (some of\n which even fail!) unless you set that variable\n\nSo this series fixes the broken tests, adapts them to work with both v0\nand v2 Git protocol, makes them work with HTTP/2 when needed, sprinkles\nin a couple other fixes, and then finally does that one-liner.\n\nI'm actually not sure if the final patch is a good idea or not, but\ncertainly all of the fixes leading up to it are worth doing.\n\n [01/16]: t5541: run \"used receive-pack service\" test earlier\n [02/16]: t5541: stop marking \"used receive-pack service\" test as v0 only\n [03/16]: t5541: simplify and move \"no empty path components\" test\n [04/16]: t5551: drop redundant grep for Accept-Language\n [05/16]: t5551: lower-case headers in expected curl trace\n [06/16]: t5551: handle HTTP/2 when checking curl trace\n [07/16]: t5551: stop forcing clone to run with v0 protocol\n [08/16]: t5551: handle v2 protocol when checking curl trace\n [09/16]: t5551: handle v2 protocol in upload-pack service test\n [10/16]: t5551: simplify expected cookie file\n [11/16]: t5551: handle v2 protocol in cookie test\n [12/16]: t5551: drop curl trace lines without headers\n [13/16]: t/lib-httpd: respect $HTTPD_PROTO in expect_askpass()\n [14/16]: t/lib-httpd: enable HTTP/2 \"h2\" protocol, not just h2c\n [15/16]: t5559: fix test failures with LIB_HTTPD_SSL\n [16/16]: t5559: make SSL/TLS the default\n\n t/lib-httpd.sh | 6 +-\n t/lib-httpd/apache.conf | 2 +-\n t/t5541-http-push-smart.sh | 57 +++-------\n t/t5551-http-fetch-smart.sh | 170 ++++++++++++++++++------------\n t/t5559-http-fetch-smart-http2.sh | 1 +\n 5 files changed, 122 insertions(+), 114 deletions(-)\n\n-Peff\n"},{"id":"472527","messageId":"Y/dEy0m6gUgI3Bzn@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 01/16] t5541: run \"used receive-pack service\" test earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:49:47Z","receivedAt":"2023-02-23T10:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a test in t5541 that confirms that \"git push\" makes two requests\n(a GET to /info/refs, and a POST to /git-receive-pack). However, it's a\nnoop unless GIT_TEST_PROTOCOL_VERSION is set to \"0\", due to 8a1b0978ab\n(test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate,\n2019-12-23).\n\nThis means that almost nobody runs it. And indeed, it has been broken\nsince b0c4adcdd7 (remote-curl: send Accept-Language header to server,\n2022-07-11). But the fault is not in that commit, but in how brittle the\ntest is. It runs after several operations have been performed, which\nmeans that it expects to see the complete set of requests made so far in\nthe script. Commit b0c4adcdd7 added a new test, which means that the\n\"used receive-pack service\" test must be updated, too.\n\nLet's fix this by making the test less brittle. We'll move it higher in\nthe script, right after the first push has completed. And we'll clear\nthe access log right before doing the push, so we'll see only the\nrequests from that command.\n\nThis is technically testing less, in that we won't check that all of\nthose other requests also correctly used smart http. But there's no\nparticular reason think that if the first one did, the others wouldn't.\n\nAfter this patch, running:\n\n GIT_TEST_PROTOCOL_VERSION=0 ./t5541-http-push-smart.sh\n\npasses, whereas it did not before.\n\nSigned-off-by: Jeff King \n---\n t/t5541-http-push-smart.sh | 44 ++++++++++++++++----------------------\n 1 file changed, 18 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex fbad2d5ff5..ef39d14ed2 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -41,10 +41,6 @@ GET /smart/test_repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n POST /smart/test_repo.git/git-upload-pack HTTP/1.1 200\n EOF\n test_expect_success 'no empty path components' '\n-\t# Clear the log, so that it does not affect the \"used receive-pack\n-\t# service\" test which reads the log too.\n-\ttest_when_finished \">\\\"\\$HTTPD_ROOT_PATH\\\"/access.log\" &&\n-\n \t# In the URL, add a trailing slash, and see if git appends yet another\n \t# slash.\n \tcd \"$ROOT_PATH\" &&\n@@ -67,6 +63,10 @@ test_expect_success 'clone remote repository' '\n '\n \n test_expect_success 'push to remote repository (standard)' '\n+\t# Clear the log, so that the \"used receive-pack service\" test below\n+\t# sees just what we did here.\n+\t>\"$HTTPD_ROOT_PATH\"/access.log &&\n+\n \tcd \"$ROOT_PATH\"/test_repo_clone &&\n \t: >path2 &&\n \tgit add path2 &&\n@@ -80,6 +80,20 @@ test_expect_success 'push to remote repository (standard)' '\n \t test $HEAD = $(git rev-parse --verify HEAD))\n '\n \n+test_expect_success 'used receive-pack service' '\n+\tcat >exp <<-\\EOF &&\n+\tGET /smart/test_repo.git/info/refs?service=git-receive-pack HTTP/1.1 200\n+\tPOST /smart/test_repo.git/git-receive-pack HTTP/1.1 200\n+\tEOF\n+\n+\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n+\t# might be able to run this test in all protocol versions.\n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n+\tthen\n+\t\tcheck_access_log exp\n+\tfi\n+'\n+\n test_expect_success 'push to remote repository (standard) with sending Accept-Language' '\n \tcat >exp <<-\\EOF &&\n \t=> Send header: Accept-Language: ko-KR, *;q=0.9\n@@ -141,28 +155,6 @@ test_expect_success 'rejected update prints status' '\n '\n rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n \n-cat >exp <\n---\n t/t5541-http-push-smart.sh | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex ef39d14ed2..f8bf533c33 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -86,12 +86,7 @@ test_expect_success 'used receive-pack service' '\n \tPOST /smart/test_repo.git/git-receive-pack HTTP/1.1 200\n \tEOF\n \n-\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n-\t# might be able to run this test in all protocol versions.\n-\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n-\tthen\n-\t\tcheck_access_log exp\n-\tfi\n+\tcheck_access_log exp\n '\n \n test_expect_success 'push to remote repository (standard) with sending Accept-Language' '\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472529","messageId":"Y/dFN4GuCjqpIi2O@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 03/16] t5541: simplify and move \"no empty path components\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:51:35Z","receivedAt":"2023-02-23T10:51:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 9ee6bcd398 (t5541-http-push: add test for URLs with trailing\nslash, 2010-04-08) added a test that clones a URL with a trailing slash,\nand confirms that we don't send a doubled slash (like \"$url//info/refs\")\nto the server.\n\nBut this test makes no sense in t5541, which is about pushing. It should\nhave been added in t5551. Let's move it there.\n\nBut putting it at the end is tricky, since it checks the entire contents\nof the Apache access log. We could get around this by clearing the log\nbefore our test. But there's an even simpler solution: just make sure no\ndoubled slashes appear in the log (fortunately, \"http://\" does not\nappear in the log itself).\n\nAs a bonus, this also lets us drop the check for the v0 protocol (which\nis otherwise necessary since v2 makes multiple requests, and\ncheck_access_log insists on exactly matching the number of requests,\neven though we don't care about that here).\n\nSigned-off-by: Jeff King \n---\n t/t5541-http-push-smart.sh | 18 ------------------\n t/t5551-http-fetch-smart.sh | 9 +++++++++\n 2 files changed, 9 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex f8bf533c33..d0211cd8be 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -36,24 +36,6 @@ test_expect_success 'setup remote repository' '\n \n setup_askpass_helper\n \n-cat >exp <log &&\n+\t! grep \"//\" log\n+'\n+\n test_done\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472530","messageId":"Y/dFWqzaCeCbmGUs@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 04/16] t5551: drop redundant grep for Accept-Language","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:52:10Z","receivedAt":"2023-02-23T10:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b0c4adcdd7 (remote-curl: send Accept-Language header to server,\n2022-07-11) added tests to make sure the header is sent via HTTP.\nHowever, it checks in two places:\n\n 1. In the expected trace output, we check verbatim for the header and\n its value.\n\n 2. Afterwards, we grep for the header again in the trace file.\n\nThis (2) is probably cargo-culted from the earlier grep for\nAccept-Encoding. It is needed for the encoding because we smudge the\nvalue of that header when doing the verbatim check; see 1a53e692af\n(remote-curl: accept all encodings supported by curl, 2018-05-22).\n\nBut we don't do so for the language header, so any problem that the\n\"grep\" would catch in (2) would already have been caught by (1).\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 10b7e7cda2..29d489768e 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -103,10 +103,7 @@ test_expect_success 'clone http repository' '\n \t\ttest_cmp exp actual.smudged &&\n \n \t\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n-\t\ttest_line_count = 2 actual.gzip &&\n-\n-\t\tgrep \"Accept-Language: ko-KR, *\" actual >actual.language &&\n-\t\ttest_line_count = 2 actual.language\n+\t\ttest_line_count = 2 actual.gzip\n \tfi\n '\n \n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472531","messageId":"Y/dFyjOAliTgOHYV@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 05/16] t5551: lower-case headers in expected curl trace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:54:02Z","receivedAt":"2023-02-23T10:54:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a test in t5551 which checks the curl trace (after simplifying\nit a bit). It doesn't work with HTTP/2, because in that case curl\noutputs all of the headers in lower-case. Even though this test is run\nwith HTTP/2 by t5559, nobody has noticed because checking the trace only\nhappens if GIT_TEST_PROTOCOL_VERSION is manually set to \"0\".\n\nLet's fix this by lower-casing all of the header names in the trace, and\nthen checking for those in our expected code (this is easier than making\nHTTP/2 traces look like HTTP/1.1, since HTTP/1.1 uses title-casing).\n\nSadly, we can't quite do this in our existing sed script. This works if\nyou have GNU sed:\n\n s/^\\\\([><]\\\\) \\\\([A-Za-z0-9-]*:\\\\)/\\1 \\L\\2\\E/\n\nbut \\L is a GNU-ism, and I don't think there's a portable solution. We\ncould just \"tr A-Z a-z\" on the way in, of course, but that makes the\nnon-header parts harder to read (e.g., lowercase \"post\" requests). But\nto paraphrase Baron Munchausen, I have learned from experience that a\nmodicum of Perl can be most efficacious.\n\nNote that this doesn't quite get the test passing with t5559; there are\nmore fixes needed on top.\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 55 ++++++++++++++++++++-----------------\n 1 file changed, 30 insertions(+), 25 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 29d489768e..a81f852cbf 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -35,30 +35,35 @@ setup_askpass_helper\n test_expect_success 'clone http repository' '\n \tcat >exp <<-\\EOF &&\n \t> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n-\t> Accept: */*\n-\t> Accept-Encoding: ENCODINGS\n-\t> Accept-Language: ko-KR, *;q=0.9\n-\t> Pragma: no-cache\n+\t> accept: */*\n+\t> accept-encoding: ENCODINGS\n+\t> accept-language: ko-KR, *;q=0.9\n+\t> pragma: no-cache\n \t< HTTP/1.1 200 OK\n-\t< Pragma: no-cache\n-\t< Cache-Control: no-cache, max-age=0, must-revalidate\n-\t< Content-Type: application/x-git-upload-pack-advertisement\n+\t< pragma: no-cache\n+\t< cache-control: no-cache, max-age=0, must-revalidate\n+\t< content-type: application/x-git-upload-pack-advertisement\n \t> POST /smart/repo.git/git-upload-pack HTTP/1.1\n-\t> Accept-Encoding: ENCODINGS\n-\t> Content-Type: application/x-git-upload-pack-request\n-\t> Accept: application/x-git-upload-pack-result\n-\t> Accept-Language: ko-KR, *;q=0.9\n-\t> Content-Length: xxx\n+\t> accept-encoding: ENCODINGS\n+\t> content-type: application/x-git-upload-pack-request\n+\t> accept: application/x-git-upload-pack-result\n+\t> accept-language: ko-KR, *;q=0.9\n+\t> content-length: xxx\n \t< HTTP/1.1 200 OK\n-\t< Pragma: no-cache\n-\t< Cache-Control: no-cache, max-age=0, must-revalidate\n-\t< Content-Type: application/x-git-upload-pack-result\n+\t< pragma: no-cache\n+\t< cache-control: no-cache, max-age=0, must-revalidate\n+\t< content-type: application/x-git-upload-pack-result\n \tEOF\n \n \tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 LANGUAGE=\"ko_KR.UTF-8\" \\\n \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q /\n \t\t}\n \n-\t\t/^> User-Agent: /d\n-\t\t/^> Host: /d\n+\t\t/^> user-agent: /d\n+\t\t/^> host: /d\n \t\t/^> POST /,$ {\n \t\t\t/^> Accept: [*]\\\\/[*]/d\n \t\t}\n-\t\ts/^> Content-Length: .*/> Content-Length: xxx/\n+\t\ts/^> content-length: .*/> content-length: xxx/\n \t\t/^> 00..want /d\n \t\t/^> 00.*done/d\n \n-\t\t/^< Server: /d\n-\t\t/^< Expires: /d\n-\t\t/^< Date: /d\n-\t\t/^< Content-Length: /d\n-\t\t/^< Transfer-Encoding: /d\n+\t\t/^< server: /d\n+\t\t/^< expires: /d\n+\t\t/^< date: /d\n+\t\t/^< content-length: /d\n+\t\t/^< transfer-encoding: /d\n \t\" >actual &&\n \n \t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n \t# might be able to run this test in all protocol versions.\n \tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n-\t\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n+\t\tsed -e \"s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/\" \\\n \t\t\t\tactual >actual.smudged &&\n \t\ttest_cmp exp actual.smudged &&\n \n-\t\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n+\t\tgrep \"accept-encoding:.*gzip\" actual >actual.gzip &&\n \t\ttest_line_count = 2 actual.gzip\n \tfi\n '\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472532","messageId":"Y/dGUXaYrp6AqMxB@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 06/16] t5551: handle HTTP/2 when checking curl trace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:56:17Z","receivedAt":"2023-02-23T10:56:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check that the curl trace of a clone has the lines we expect, but\nthis won't work when we run the test under t5559, because a few details\nare different under HTTP/2 (but nobody noticed because it only happens\nwhen you manually set GIT_TEST_PROTOCOL_VERSION to \"0\").\n\nWe can handle both HTTP protocols with a few tweaks:\n\n - we'll drop the HTTP \"101 Switching Protocols\" response, as well as\n various protocol upgrade headers. These details aren't interesting\n to us. We just want to make sure the correct protocol was used (and\n we do in the main request/response lines).\n\n - successful HTTP/2 responses just say \"200\" and not \"200 OK\"; we can\n normalize these\n\n - replace HTTP/1.1 with a variable in the request/response lines. We\n can use the existing $HTTP_PROTO for this, as it's already set to\n \"HTTP/2\" when appropriate. We do need to tweak the fallback value to\n \"HTTP/1.1\" to match what curl will write (prior to this patch, the\n fallback value didn't matter at all; we only checked if it was the\n literal string \"HTTP/2\").\n\nNote that several lines still expect HTTP/1.1 unconditionally. The first\nrequest does so because the client requests an upgrade during the\nrequest. The POST request and response do so because you can't do an\nupgrade if there is a request body. (This will all be different if we\ntrigger HTTP/2 via ALPN, but the tests aren't yet capable of that).\n\nThis is enough to let:\n\n GIT_TEST_PROTOCOL_VERSION=0 ./t5559-http-fetch-smart-http2.sh\n\npass the \"clone http repository\" test (but there are some other failures\nlater on).\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex a81f852cbf..716c9dbb69 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-: ${HTTP_PROTO:=HTTP}\n+: ${HTTP_PROTO:=HTTP/1.1}\n test_description=\"test smart fetching over http via http-backend ($HTTP_PROTO)\"\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n@@ -33,13 +33,13 @@ test_expect_success 'create http-accessible bare repository' '\n setup_askpass_helper\n \n test_expect_success 'clone http repository' '\n-\tcat >exp <<-\\EOF &&\n+\tcat >exp <<-EOF &&\n \t> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n \t> accept: */*\n \t> accept-encoding: ENCODINGS\n \t> accept-language: ko-KR, *;q=0.9\n \t> pragma: no-cache\n-\t< HTTP/1.1 200 OK\n+\t< $HTTP_PROTO 200 OK\n \t< pragma: no-cache\n \t< cache-control: no-cache, max-age=0, must-revalidate\n \t< content-type: application/x-git-upload-pack-advertisement\n@@ -83,6 +83,14 @@ test_expect_success 'clone http repository' '\n \t\t\ts/^/> /\n \t\t}\n \n+\t\t/^< HTTP/ {\n+\t\t\ts/200$/200 OK/\n+\t\t}\n+\t\t/^< HTTP\\\\/1.1 101/d\n+\t\t/^[><] connection: /d\n+\t\t/^[><] upgrade: /d\n+\t\t/^> http2-settings: /d\n+\n \t\t/^> user-agent: /d\n \t\t/^> host: /d\n \t\t/^> POST /,$ {\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472533","messageId":"Y/dGl990U3gMCwY4@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 07/16] t5551: stop forcing clone to run with v0 protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T10:57:27Z","receivedAt":"2023-02-23T10:57:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the \"clone http repository\" test, we check the curl trace to make\nsure the expected requests were made. This whole script was marked to\nhandle only the v0 protocol in d790ee1707 (tests: fix protocol version\nfor overspecifications, 2019-02-25). That makes sense, since v2 requires\nan extra request, so tests as specific as this would fail unless\nmodified.\n\nLater, in preparation for v2 becoming the default, this was tweaked by\n8a1b0978ab (test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate,\n2019-12-23). There we run the trace check only if the user has\nexplicitly asked to test protocol version 0. But it also forced the\nclone itself to run with the v0 protocol.\n\nThis makes the check for \"can we expect a v0 trace\" silly; it will\nalways be v0. But much worse, it means that the clone we are testing is\nnot like the one that normal users would run. They would use the\ndefaults, which are now v2. And since this is supposed to be a basic\ncheck of clone-over-http, we should do the same.\n\nLet's fix this by dropping the extra v0 override. The test still passes\nbecause the trace checking only kicks in if we asked to use v0\nexplicitly (this is the same as before; even though we were running a v0\nclone, unless you specifically set GIT_TEST_PROTOCOL_VERSION=0, the\ntrace check was always skipped).\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 716c9dbb69..4191174584 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -55,7 +55,7 @@ test_expect_success 'clone http repository' '\n \t< content-type: application/x-git-upload-pack-result\n \tEOF\n \n-\tGIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 LANGUAGE=\"ko_KR.UTF-8\" \\\n+\tGIT_TRACE_CURL=true LANGUAGE=\"ko_KR.UTF-8\" \\\n \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n \ttr '\\''\\015'\\'' Q \n---\n t/t5551-http-fetch-smart.sh | 36 +++++++++++++++++++++++++-----------\n 1 file changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 4191174584..9d99cefb92 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -33,12 +33,13 @@ test_expect_success 'create http-accessible bare repository' '\n setup_askpass_helper\n \n test_expect_success 'clone http repository' '\n-\tcat >exp <<-EOF &&\n+\tcat >exp.raw <<-EOF &&\n \t> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n \t> accept: */*\n \t> accept-encoding: ENCODINGS\n \t> accept-language: ko-KR, *;q=0.9\n \t> pragma: no-cache\n+\t{V2} > git-protocol: version=2\n \t< $HTTP_PROTO 200 OK\n \t< pragma: no-cache\n \t< cache-control: no-cache, max-age=0, must-revalidate\n@@ -48,13 +49,32 @@ test_expect_success 'clone http repository' '\n \t> content-type: application/x-git-upload-pack-request\n \t> accept: application/x-git-upload-pack-result\n \t> accept-language: ko-KR, *;q=0.9\n+\t{V2} > git-protocol: version=2\n \t> content-length: xxx\n \t< HTTP/1.1 200 OK\n \t< pragma: no-cache\n \t< cache-control: no-cache, max-age=0, must-revalidate\n \t< content-type: application/x-git-upload-pack-result\n+\t{V2} > POST /smart/repo.git/git-upload-pack HTTP/1.1\n+\t{V2} > accept-encoding: ENCODINGS\n+\t{V2} > content-type: application/x-git-upload-pack-request\n+\t{V2} > accept: application/x-git-upload-pack-result\n+\t{V2} > accept-language: ko-KR, *;q=0.9\n+\t{V2} > git-protocol: version=2\n+\t{V2} > content-length: xxx\n+\t{V2} < HTTP/1.1 200 OK\n+\t{V2} < pragma: no-cache\n+\t{V2} < cache-control: no-cache, max-age=0, must-revalidate\n+\t{V2} < content-type: application/x-git-upload-pack-result\n \tEOF\n \n+\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n+\tthen\n+\t\tsed \"/^{V2}/d\" exp\n+\telse\n+\t\tsed \"s/^{V2} //\" exp\n+\tfi &&\n+\n \tGIT_TRACE_CURL=true LANGUAGE=\"ko_KR.UTF-8\" \\\n \t\tgit clone --quiet $HTTPD_URL/smart/repo.git clone 2>err &&\n \ttest_cmp file clone/file &&\n@@ -107,17 +127,11 @@ test_expect_success 'clone http repository' '\n \t\t/^< transfer-encoding: /d\n \t\" >actual &&\n \n-\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n-\t# might be able to run this test in all protocol versions.\n-\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n-\tthen\n-\t\tsed -e \"s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/\" \\\n-\t\t\t\tactual >actual.smudged &&\n-\t\ttest_cmp exp actual.smudged &&\n+\tsed -e \"s/^> accept-encoding: .*/> accept-encoding: ENCODINGS/\" \\\n+\t\t\tactual >actual.smudged &&\n+\ttest_cmp exp actual.smudged &&\n \n-\t\tgrep \"accept-encoding:.*gzip\" actual >actual.gzip &&\n-\t\ttest_line_count = 2 actual.gzip\n-\tfi\n+\tgrep \"accept-encoding:.*gzip\" actual >actual.gzip\n '\n \n test_expect_success 'fetch changes via http' '\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472535","messageId":"Y/dHVu3p125CgcYR@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 09/16] t5551: handle v2 protocol in upload-pack service test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:00:38Z","receivedAt":"2023-02-23T11:00:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We perform a clone and a fetch, and then check that we saw the expected\nrequests in Apache's access log. In the v2 protocol, there will be one\nextra request to /git-upload-pack for each operation (since the initial\n/info/refs probe is just used to upgrade the protocol).\n\nAs a result, this test is a noop unless the use of the v0 protocol is\nforced. Which means that hardly anybody runs it, since you have to do so\nmanually.\n\nLet's update it to handle v2 and run it always. We could do this by just\nconditionally adding in the extra POST lines. But if we look at the\norigin of the test in 7da4e2280c (test smart http fetch and push,\n2009-10-30), the point is really just to make sure that the smart\ngit-upload-pack service was used at all. So rather than counting up the\nindividual requests, let's just make sure we saw each of the expected\ntypes. This is a bit looser, but makes maintenance easier.\n\nSince we're now matching with grep, we can also loosen the HTTP/1.1\nmatch, which allows this test to pass when run with HTTP/2 via t5559.\nThat lets:\n\n GIT_TEST_PROTOCOL_VERSION=0 ./t5559-http-fetch-smart-http2.sh\n\nrun to completion, which previously failed (and of course it works if\nyou use v2, as well).\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 16 +++-------------\n 1 file changed, 3 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 9d99cefb92..b912958518 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -143,19 +143,9 @@ test_expect_success 'fetch changes via http' '\n '\n \n test_expect_success 'used upload-pack service' '\n-\tcat >exp <<-\\EOF &&\n-\tGET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n-\tPOST /smart/repo.git/git-upload-pack HTTP/1.1 200\n-\tGET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 200\n-\tPOST /smart/repo.git/git-upload-pack HTTP/1.1 200\n-\tEOF\n-\n-\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n-\t# might be able to run this test in all protocol versions.\n-\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n-\tthen\n-\t\tcheck_access_log exp\n-\tfi\n+\tstrip_access_log >log &&\n+\tgrep \"GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/[0-9.]* 200\" log &&\n+\tgrep \"POST /smart/repo.git/git-upload-pack HTTP/[0-9.]* 200\" log\n '\n \n test_expect_success 'follow redirects (301)' '\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472536","messageId":"Y/dHi+I6a2yLT4HY@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 10/16] t5551: simplify expected cookie file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:01:31Z","receivedAt":"2023-02-23T11:01:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After making an HTTP request that should store cookies, we check that\nthe expected values are in the cookie file. We don't want to look at the\nwhole file, because it has noisy comments at the top that we shouldn't\ndepend on. But we strip out the interesting bits using \"tail -3\", which\nis brittle. It requires us to put an extra blank line in our expected\noutput, and it would fail to notice any reordering or extra content in\nthe cookie file.\n\nInstead, let's just grep for non-blank lines that are not comments,\nwhich more directly describes what we're interested in.\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex b912958518..2f15a707d4 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -294,7 +294,6 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \t127.0.0.1\tFALSE\t/smart_cookies/\tFALSE\t0\tothername\tothervalue\n \tEOF\n \tsort >expect_cookies.txt <<-\\EOF &&\n-\n \t127.0.0.1\tFALSE\t/smart_cookies/\tFALSE\t0\tothername\tothervalue\n \t127.0.0.1\tFALSE\t/smart_cookies/repo.git/info/\tFALSE\t0\tname\tvalue\n \tEOF\n@@ -306,8 +305,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \t# might be able to run this test in all protocol versions.\n \tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n \tthen\n-\t\ttail -3 cookies.txt | sort >cookies_tail.txt &&\n-\t\ttest_cmp expect_cookies.txt cookies_tail.txt\n+\t\tgrep \"^[^#]\" cookies.txt | sort >cookies_stripped.txt &&\n+\t\ttest_cmp expect_cookies.txt cookies_stripped.txt\n \tfi\n '\n \n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472537","messageId":"Y/dHx146qUjob2oX@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 11/16] t5551: handle v2 protocol in cookie test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:02:31Z","receivedAt":"2023-02-23T11:02:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After making a request, we check that it stored the expected cookies.\nThis depends on the protocol version, because the cookies we store\ndepend on the exact requests we made (and for ls-remote, v2 will always\nhit /git-upload-pack to get the refs, whereas v0 is happy with the\ninitial ref advertisement).\n\nAs a result, hardly anybody runs this test, as you'd have to manually\nset GIT_TEST_PROTOCOL_VERSION=0 to do so.\n\nLet's teach it to handle both protocol versions. One way to do this\nwould be to make the expectation conditional on the protocol used. But\nthere's a simpler solution. The reason that v0 doesn't hit\n/git-upload-pack is that ls-remote doesn't fetch any objects. If we\ninstead do a fetch (making sure there's an actual object to grab), then\nboth v0 and v2 will hit the same endpoints and set the same cookies.\n\nNote that we do have to clean up our new tag here; otherwise it confuses\nthe later \"clone 2,000 tags\" test.\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 2f15a707d4..2e42271cb8 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -295,19 +295,22 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set\n \tEOF\n \tsort >expect_cookies.txt <<-\\EOF &&\n \t127.0.0.1\tFALSE\t/smart_cookies/\tFALSE\t0\tothername\tothervalue\n+\t127.0.0.1\tFALSE\t/smart_cookies/repo.git/\tFALSE\t0\tname\tvalue\n \t127.0.0.1\tFALSE\t/smart_cookies/repo.git/info/\tFALSE\t0\tname\tvalue\n \tEOF\n \tgit config http.cookiefile cookies.txt &&\n \tgit config http.savecookies true &&\n-\tgit ls-remote $HTTPD_URL/smart_cookies/repo.git main &&\n \n-\t# NEEDSWORK: If the overspecification of the expected result is reduced, we\n-\t# might be able to run this test in all protocol versions.\n-\tif test \"$GIT_TEST_PROTOCOL_VERSION\" = 0\n-\tthen\n-\t\tgrep \"^[^#]\" cookies.txt | sort >cookies_stripped.txt &&\n-\t\ttest_cmp expect_cookies.txt cookies_stripped.txt\n-\tfi\n+\ttest_when_finished \"\n+\t\tgit --git-dir=\\\"\\$HTTPD_DOCUMENT_ROOT_PATH/repo.git\\\" \\\n+\t\t\ttag -d cookie-tag\n+\t\" &&\n+\tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\ttag -m \"foo\" cookie-tag &&\n+\tgit fetch $HTTPD_URL/smart_cookies/repo.git cookie-tag &&\n+\n+\tgrep \"^[^#]\" cookies.txt | sort >cookies_stripped.txt &&\n+\ttest_cmp expect_cookies.txt cookies_stripped.txt\n '\n \n test_expect_success 'transfer.hiderefs works over smart-http' '\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472538","messageId":"Y/dIY7wTfBuG9Rox@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 12/16] t5551: drop curl trace lines without headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:05:07Z","receivedAt":"2023-02-23T11:05:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We pick apart a curl trace, looking for \"=> Send header:\" and so on, and\nmatching against an expected set of requests and responses. We remove\n\"== Info\" lines entirely. However, our parser is fooled when running the\ntest with LIB_HTTPD_SSL on Ubuntu 20.04 (as found in our linux-gcc CI\njob), as curl hands us an \"Info\" buffer with a newline, and we get:\n\n == Info: successfully set certificate verify locations:\n == Info: CAfile: /etc/ssl/certs/ca-certificates.crt\n CApath: /etc/ssl/certs\n => Send SSL data[...]\n\nwhich results in the \"CApath\" line ending up in the cleaned-up output,\ncausing the test to fail.\n\nArguably the tracing code should detect this and put it on two separate\n\"== Info\" lines. But this is actually a curl bug, fixed by their\n80d73bcca (tls: provide the CApath verbose log on its own line,\n2020-08-18). It's simpler to just work around it here.\n\nSince we are using GIT_TRACE_CURL, every line should just start with one\nof \"<=\", \"==\", or \"=>\", and we can throw away anything else. In fact, we\ncan just replace the pattern for deleting \"*\" lines. Those were from the\nold GIT_CURL_VERBOSE output, but we switched over in 14e24114d9\n(t5551-http-fetch-smart.sh: use the GIT_TRACE_CURL environment var,\n2016-09-05).\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 2e42271cb8..13b38c7ef5 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -86,7 +86,7 @@ test_expect_success 'clone http repository' '\n \t'\\'' |\n \tsed -e \"\n \t\ts/Q\\$//\n-\t\t/^[*] /d\n+\t\t/^[^<=]/d\n \t\t/^== Info:/d\n \t\t/^=> Send header, /d\n \t\t/^=> Send header:$/d\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472539","messageId":"Y/dIk5Zex6Sq2p8h@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 13/16] t/lib-httpd: respect $HTTPD_PROTO in expect_askpass()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:05:55Z","receivedAt":"2023-02-23T11:05:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the HTTP tests are run with LIB_HTTPD_SSL in the environment, then\nwe access the test server as https://. This causes expect_askpass to\ncomplain, because it tries to blindly match \"http://\" in the prompt\nshown to the user. We can adjust this to use $HTTPD_PROTO, which is set\nduring the setup phase.\n\nNote that this is enough for t5551 and t5559 to pass when run with\nhttps, but there are similar problems in other scripts that will need to\nbe fixed before the whole suite can run with LIB_HTTPD_SSL.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 5d2d56c445..8f59e9dff8 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -283,11 +283,11 @@ expect_askpass() {\n \t\tnone)\n \t\t\t;;\n \t\tpass)\n-\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n+\t\t\techo \"askpass: Password for '$HTTPD_PROTO://$2@$dest': \"\n \t\t\t;;\n \t\tboth)\n-\t\t\techo \"askpass: Username for 'http://$dest': \"\n-\t\t\techo \"askpass: Password for 'http://$2@$dest': \"\n+\t\t\techo \"askpass: Username for '$HTTPD_PROTO://$dest': \"\n+\t\t\techo \"askpass: Password for '$HTTPD_PROTO://$2@$dest': \"\n \t\t\t;;\n \t\t*)\n \t\t\tfalse\n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472540","messageId":"Y/dIxHI9iB5KGOR5@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 14/16] t/lib-httpd: enable HTTP/2 \"h2\" protocol, not just h2c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:06:44Z","receivedAt":"2023-02-23T11:06:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 73c49a4474 (t: run t5551 tests with both HTTP and HTTP/2,\n2022-11-11) added Apache config to enable HTTP/2. However, it only\nenabled the \"h2c\" protocol, which allows cleartext HTTP/2 (generally\nbased on an upgrade header during an HTTP/1.1 request). This is what\nt5559 is generally testing, since by default we don't set up SSL/TLS.\n\nHowever, it should be possible to run t5559 with LIB_HTTPD_SSL set. In\nthat case, Apache will advertise support for HTTP/2 via ALPN during the\nTLS handshake. But we need to tell it support \"h2\" (the non-cleartext\nversion) to do so. Without that, then curl does not even try to do the\nHTTP/1.1 upgrade (presumably because after seeing that we did TLS but\ndidn't get the ALPN indicator, it assumes it would be fruitless).\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd/apache.conf | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 51a4fbcf62..693b75e0ae 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -31,7 +31,7 @@ ErrorLog error.log\n \n \n LoadModule http2_module modules/mod_http2.so\n-Protocols h2c\n+Protocols h2 h2c\n \n \n \n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472541","messageId":"Y/dI8UEP0DFqqB1Q@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 15/16] t5559: fix test failures with LIB_HTTPD_SSL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:07:29Z","receivedAt":"2023-02-23T11:07:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One test needs to be tweaked in order for t5559 to pass with SSL/TLS set\nup. When we make our initial clone, we check that the curl trace of\nrequests is what we expected. But we need to fix two things:\n\n - along with ignoring \"data\" lines from the trace, we need to ignore\n \"SSL data\" lines\n\n - when TLS is used, the server is able to tell the client (via ALPN)\n that it supports HTTP/2 before the first HTTP request is made. So\n rather than request an upgrade using an HTTP header, it can just\n speak HTTP/2 immediately\n\nWith this patch, running:\n\n LIB_HTTPD_SSL=1 ./t5559-http-fetch-smart-http2.sh\n\nworks, whereas it did not before.\n\nSigned-off-by: Jeff King \n---\n t/t5551-http-fetch-smart.sh | 23 ++++++++++++++++++-----\n 1 file changed, 18 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 13b38c7ef5..0908534f25 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -33,8 +33,19 @@ test_expect_success 'create http-accessible bare repository' '\n setup_askpass_helper\n \n test_expect_success 'clone http repository' '\n+\tif test_have_prereq HTTP2 && test \"$HTTPD_PROTO\" = \"https\"\n+\tthen\n+\t\t# ALPN lets us immediately use HTTP/2; likewise, POSTs with\n+\t\t# bodies can use it because they do not need to upgrade\n+\t\tINITIAL_PROTO=HTTP/2\n+\telse\n+\t\t# either we are not using HTTP/2, or the initial\n+\t\t# request is sent via HTTP/1.1 and asks for upgrade\n+\t\tINITIAL_PROTO=HTTP/1.1\n+\tfi &&\n+\n \tcat >exp.raw <<-EOF &&\n-\t> GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n+\t> GET /smart/repo.git/info/refs?service=git-upload-pack $INITIAL_PROTO\n \t> accept: */*\n \t> accept-encoding: ENCODINGS\n \t> accept-language: ko-KR, *;q=0.9\n@@ -44,25 +55,25 @@ test_expect_success 'clone http repository' '\n \t< pragma: no-cache\n \t< cache-control: no-cache, max-age=0, must-revalidate\n \t< content-type: application/x-git-upload-pack-advertisement\n-\t> POST /smart/repo.git/git-upload-pack HTTP/1.1\n+\t> POST /smart/repo.git/git-upload-pack $INITIAL_PROTO\n \t> accept-encoding: ENCODINGS\n \t> content-type: application/x-git-upload-pack-request\n \t> accept: application/x-git-upload-pack-result\n \t> accept-language: ko-KR, *;q=0.9\n \t{V2} > git-protocol: version=2\n \t> content-length: xxx\n-\t< HTTP/1.1 200 OK\n+\t< $INITIAL_PROTO 200 OK\n \t< pragma: no-cache\n \t< cache-control: no-cache, max-age=0, must-revalidate\n \t< content-type: application/x-git-upload-pack-result\n-\t{V2} > POST /smart/repo.git/git-upload-pack HTTP/1.1\n+\t{V2} > POST /smart/repo.git/git-upload-pack $INITIAL_PROTO\n \t{V2} > accept-encoding: ENCODINGS\n \t{V2} > content-type: application/x-git-upload-pack-request\n \t{V2} > accept: application/x-git-upload-pack-result\n \t{V2} > accept-language: ko-KR, *;q=0.9\n \t{V2} > git-protocol: version=2\n \t{V2} > content-length: xxx\n-\t{V2} < HTTP/1.1 200 OK\n+\t{V2} < $INITIAL_PROTO 200 OK\n \t{V2} < pragma: no-cache\n \t{V2} < cache-control: no-cache, max-age=0, must-revalidate\n \t{V2} < content-type: application/x-git-upload-pack-result\n@@ -96,6 +107,8 @@ test_expect_success 'clone http repository' '\n \t\ts/= Recv header://\n \t\t/^<= Recv data/d\n \t\t/^=> Send data/d\n+\t\t/^<= Recv SSL data/d\n+\t\t/^=> Send SSL data/d\n \t\t/^$/d\n \t\t/^< $/d\n \n-- \n2.39.2.981.g6157336f25\n\n"},{"id":"472542","messageId":"Y/dJOa2k6HEIq6oD@coredump.intra.peff.net","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"[PATCH 16/16] t5559: make SSL/TLS the default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-23T11:08:41Z","receivedAt":"2023-02-23T11:08:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of t5559 is run the regular t5551 tests with HTTP/2. But it\ndoes so with the \"h2c\" protocol, which uses cleartext upgrades from\nHTTP/1.1 to HTTP/2 (rather than learning about HTTP/2 support during the\nTLS negotiation).\n\nThis has a few problems:\n\n - it's not very indicative of the real world. In practice, most servers\n that support HTTP/2 will also support TLS.\n\n - support for upgrading does not seem as robust. In particular, we've\n run into bugs in some versions of Apache's mod_http2 that trigger\n only with the upgrade mode. See:\n\n https://lore.kernel.org/git/Y8ztIqYgVCPILJlO@coredump.intra.peff.net/\n\nSo the upside is that this change makes our HTTP/2 tests more robust and\nmore realistic. The downside is that if we can't set up SSL for any\nreason, we'll skip the tests (even though you _might_ have been able to\nrun the HTTP/2 tests the old way). We could probably have a conditional\nfallback, but it would be complicated for little gain, and it's not even\nclear it would help (i.e., would any test environment even have HTTP/2\nbut not SSL support?).\n\nSigned-off-by: Jeff King \n---\n t/t5559-http-fetch-smart-http2.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\nindex 9eece71c2c..54aa9d3bff 100755\n--- a/t/t5559-http-fetch-smart-http2.sh\n+++ b/t/t5559-http-fetch-smart-http2.sh\n@@ -1,4 +1,5 @@\n #!/bin/sh\n \n HTTP_PROTO=HTTP/2\n+LIB_HTTPD_SSL=1\n . ./t5551-http-fetch-smart.sh\n-- \n2.39.2.981.g6157336f25\n"},{"id":"472582","messageId":"xmqqttzc3qeq.fsf@gitster.g","threadId":"59298","inReplyTo":"Y/dFN4GuCjqpIi2O@coredump.intra.peff.net","subject":"Re: [PATCH 03/16] t5541: simplify and move \"no empty path components\" test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T23:36:45Z","receivedAt":"2023-02-23T23:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> before our test. But there's an even simpler solution: just make sure no\n> doubled slashes appear in the log (fortunately, \"http://\" does not\n> appear in the log itself).\n\nInteresting ;-).\n\n> As a bonus, this also lets us drop the check for the v0 protocol (which\n> is otherwise necessary since v2 makes multiple requests, and\n> check_access_log insists on exactly matching the number of requests,\n> even though we don't care about that here).\n\nMakes me wonder why we do not clear the log for each test, but OK.\n"},{"id":"472583","messageId":"xmqqmt543qei.fsf@gitster.g","threadId":"59298","inReplyTo":"Y/dFWqzaCeCbmGUs@coredump.intra.peff.net","subject":"Re: [PATCH 04/16] t5551: drop redundant grep for Accept-Language","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T23:36:53Z","receivedAt":"2023-02-23T23:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Commit b0c4adcdd7 (remote-curl: send Accept-Language header to server,\n> 2022-07-11) added tests to make sure the header is sent via HTTP.\n> However, it checks in two places:\n>\n> 1. In the expected trace output, we check verbatim for the header and\n> its value.\n>\n> 2. Afterwards, we grep for the header again in the trace file.\n>\n> This (2) is probably cargo-culted from the earlier grep for\n> Accept-Encoding. It is needed for the encoding because we smudge the\n> value of that header when doing the verbatim check; see 1a53e692af\n> (remote-curl: accept all encodings supported by curl, 2018-05-22).\n>\n> But we don't do so for the language header, so any problem that the\n> \"grep\" would catch in (2) would already have been caught by (1).\n\nMakes sense.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> t/t5551-http-fetch-smart.sh | 5 +----\n> 1 file changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 10b7e7cda2..29d489768e 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -103,10 +103,7 @@ test_expect_success 'clone http repository' '\n> \t\ttest_cmp exp actual.smudged &&\n> \n> \t\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n> -\t\ttest_line_count = 2 actual.gzip &&\n> -\n> -\t\tgrep \"Accept-Language: ko-KR, *\" actual >actual.language &&\n> -\t\ttest_line_count = 2 actual.language\n> +\t\ttest_line_count = 2 actual.gzip\n> \tfi\n> '\n"},{"id":"472584","messageId":"xmqqfsaw3qea.fsf@gitster.g","threadId":"59298","inReplyTo":"Y/dIxHI9iB5KGOR5@coredump.intra.peff.net","subject":"Re: [PATCH 14/16] t/lib-httpd: enable HTTP/2 \"h2\" protocol, not just h2c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T23:37:01Z","receivedAt":"2023-02-23T23:37:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Commit 73c49a4474 (t: run t5551 tests with both HTTP and HTTP/2,\n> 2022-11-11) added Apache config to enable HTTP/2. However, it only\n> enabled the \"h2c\" protocol, which allows cleartext HTTP/2 (generally\n> based on an upgrade header during an HTTP/1.1 request). This is what\n> t5559 is generally testing, since by default we don't set up SSL/TLS.\n>\n> However, it should be possible to run t5559 with LIB_HTTPD_SSL set. In\n> that case, Apache will advertise support for HTTP/2 via ALPN during the\n> TLS handshake. But we need to tell it support \"h2\" (the non-cleartext\n> version) to do so. Without that, then curl does not even try to do the\n> HTTP/1.1 upgrade (presumably because after seeing that we did TLS but\n> didn't get the ALPN indicator, it assumes it would be fruitless).\n\nWow, that is tricky. Nicely found and fixed.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> t/lib-httpd/apache.conf | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n> index 51a4fbcf62..693b75e0ae 100644\n> --- a/t/lib-httpd/apache.conf\n> +++ b/t/lib-httpd/apache.conf\n> @@ -31,7 +31,7 @@ ErrorLog error.log\n> \n> \n> LoadModule http2_module modules/mod_http2.so\n> -Protocols h2c\n> +Protocols h2 h2c\n> \n> \n> \n"},{"id":"472585","messageId":"xmqq8rgo3qe6.fsf@gitster.g","threadId":"59298","inReplyTo":"Y/dEYYWKy/o96vBG@coredump.intra.peff.net","subject":"Re: [PATCH 0/16] http test bug potpourri","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T23:37:05Z","receivedAt":"2023-02-23T23:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So all I wanted to do was this one-liner:\n>\n> diff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh\n> index 9eece71c2c..54aa9d3bff 100755\n> --- a/t/t5559-http-fetch-smart-http2.sh\n> +++ b/t/t5559-http-fetch-smart-http2.sh\n> @@ -1,4 +1,5 @@\n> #!/bin/sh\n> \n> HTTP_PROTO=HTTP/2\n> +LIB_HTTPD_SSL=1\n> . ./t5551-http-fetch-smart.sh\n>\n> but somehow I'm 16 patches deep. Let me back up.\n>\n> I got bit once again by the \"oops, HTTP/2 tests in t5559 are sometimes\n> flaky\" bug. One thing that came up in earlier discussion is that HTTP/2\n> over TLS should be much more reliable, because it doesn't have to go\n> through the funny HTTP-upgrade path.\n>\n> Hence the patch above, which is also patch 16 here. And it does make the\n> consistent failure of t5551.30 go away. And it even makes --stress work\n> longer before a racy failure, though it still fails for me pretty\n> consistently within a few dozen runs.\n>\n> But in doing so, I found out all sorts of neat things, like:\n>\n> - when I tested with HTTP/2 and TLS before, I was accidentally not\n> using HTTP/2!\n>\n> - we even have a test that should detect which version is used, but\n> it's a silent noop unless you set GIT_TEST_PROTOCOL_VERSION=0, which\n> clearly nobody does\n>\n> - it turns out there are a bunch of tests which are skipped (some of\n> which even fail!) unless you set that variable\n>\n> So this series fixes the broken tests, adapts them to work with both v0\n> and v2 Git protocol, makes them work with HTTP/2 when needed, sprinkles\n> in a couple other fixes, and then finally does that one-liner.\n>\n> I'm actually not sure if the final patch is a good idea or not, but\n> certainly all of the fixes leading up to it are worth doing.\n\nThanks; this must have been a lot of work. From the \"test what the\nend users use, or at least something close to it\" standpoint, 16/16\ncertainly is the right thing to do, I would think.\n\n"},{"id":"472610","messageId":"Y/gcxkqZutHa90QC@coredump.intra.peff.net","threadId":"59298","inReplyTo":"xmqqttzc3qeq.fsf@gitster.g","subject":"Re: [PATCH 03/16] t5541: simplify and move \"no empty path components\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T02:11:18Z","receivedAt":"2023-02-24T02:11:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2023 at 03:36:45PM -0800, Junio C Hamano wrote:\n\n> > As a bonus, this also lets us drop the check for the v0 protocol (which\n> > is otherwise necessary since v2 makes multiple requests, and\n> > check_access_log insists on exactly matching the number of requests,\n> > even though we don't care about that here).\n> \n> Makes me wonder why we do not clear the log for each test, but OK.\n\nYes, I think that is a better strategy than the current code, but IMHO\nthese targeted greps are even better at showing what we really care\nabout. After my series, there are only two calls to check_access_log\nleft:\n\n - the one in t5541, from patch 2. This could be a grep, but since\n there is no v0/v2 confusion it was easy enough to leave it (and\n after my patch, it now is clearing and checking the activity of just\n one test)\n\n - one in t5561, which really does want to check the results after a\n ton of manual requests\n\nSo I think the state after my series is OK, at least from a maintenance\nperspective.\n\n-Peff\n"},{"id":"472611","messageId":"Y/gdVv63YSilKSYz@coredump.intra.peff.net","threadId":"59298","inReplyTo":"xmqq8rgo3qe6.fsf@gitster.g","subject":"Re: [PATCH 0/16] http test bug potpourri","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-24T02:13:42Z","receivedAt":"2023-02-24T02:13:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 23, 2023 at 03:37:05PM -0800, Junio C Hamano wrote:\n\n> > I'm actually not sure if the final patch is a good idea or not, but\n> > certainly all of the fixes leading up to it are worth doing.\n> \n> Thanks; this must have been a lot of work. From the \"test what the\n> end users use, or at least something close to it\" standpoint, 16/16\n> certainly is the right thing to do, I would think.\n\nYeah. My main concern is that we are now using SSL by default in the\ntest suite (or at least trying to; I _think_ we should fail gracefully,\nbut since it works on my system, I don't have any data beyond the fact\nthat CI seems OK with it). I think it's one of those things where we try\nit and see if anybody screams.\n\n-Peff\n"},{"id":"472613","messageId":"xmqqk0073gxx.fsf@gitster.g","threadId":"59298","inReplyTo":"Y/gdVv63YSilKSYz@coredump.intra.peff.net","subject":"Re: [PATCH 0/16] http test bug potpourri","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-24T03:01:14Z","receivedAt":"2023-02-24T03:01:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Feb 23, 2023 at 03:37:05PM -0800, Junio C Hamano wrote:\n>\n>> > I'm actually not sure if the final patch is a good idea or not, but\n>> > certainly all of the fixes leading up to it are worth doing.\n>> \n>> Thanks; this must have been a lot of work. From the \"test what the\n>> end users use, or at least something close to it\" standpoint, 16/16\n>> certainly is the right thing to do, I would think.\n>\n> Yeah. My main concern is that we are now using SSL by default in the\n> test suite (or at least trying to; I _think_ we should fail gracefully,\n> but since it works on my system, I don't have any data beyond the fact\n> that CI seems OK with it). I think it's one of those things where we try\n> it and see if anybody screams.\n\nTrue. If a platform has TLS/SSL implementation of acceptable\nquality and if we fail to build with it, that is something we want\nto learn about and help them make it work anyway, I suspect.\n\nThanks.\n"}]}