On Fri, Nov 11 2022, Jeff King wrote: > On Thu, Nov 10, 2022 at 09:29:15PM -0500, Jeff King wrote: > >> We don't get the _whole_ test suite running with http2, but hopefully it >> gives us a fairly representative sample. And it does find this bug. >> >> I can try to work the above into patch form, but I may not get to it for >> a day or two. > > So here's a patch which finds the bug you're fixing. It's set up to be > prepared before your patch, which helps confirm the bug (and that we're > correctly using http/2 in the tests!). You'll want to unmark the !HTTP2 > prereqs as part of your patch. > > Alternatively, it could come after your patch to confirm the fix, in > which case it would omit those !HTTP2 prereqs from the get-go. > > Let me know if you'd like to pick it up as part of your series. > Otherwise, I can submit it separately in the on-top form (but my > preference is for you to take it, since it makes testing the fix > easier). > > -- >8 -- > Subject: t: run t5551 tests with both HTTP and HTTP/2 > > We have occasionally seen bugs that affect Git running only against an > HTTP/2 web server, not an HTTP one. For instance, b66c77a64e (http: > match headers case-insensitively when redacting, 2021-09-22). But since > we have no test coverage using HTTP/2, we only uncover these bugs in the > wild. > > That commit gives a recipe for converting our Apache setup to support > HTTP/2, but: > > - it's not necessarily portable > > - we don't want to just test HTTP/2; we really want to do a variety of > basic tests for _both_ protocols > > This patch handles both problems by running a duplicate of t5551 > (labeled as t5559 here) with an alternate-universe setup that enables > HTTP/2. So we'll continue to run t5551 as before, but run the same > battery of tests again with HTTP/2. If HTTP/2 isn't supported on a given > platform, then t5559 should bail during the webserver setup, and > gracefully skip all tests (unless GIT_TEST_HTTPD has been changed from > "auto" to "yes", where the point is to complain when webserver setup > fails). > > In theory other http-related test scripts could benefit from the same > duplication, but doing t5551 should give us a reasonable check of basic > functionality, and would have caught both bugs we've seen in the wild > with HTTP/2. Looking at the diff below, would it be much more work to just support a GIT_TEST_HTTP_VERSION=2? Then: > diff --git a/t/t5559-http-fetch-smart-http2.sh b/t/t5559-http-fetch-smart-http2.sh > new file mode 100755 > index 0000000000..9eece71c2c > --- /dev/null > +++ b/t/t5559-http-fetch-smart-http2.sh > @@ -0,0 +1,4 @@ > +#!/bin/sh > + > +HTTP_PROTO=HTTP/2 > +. ./t5551-http-fetch-smart.sh This would just skip_all if it was already set to 2, as we'd know we were testing it in other tests, and lib-httpd.sh would do GIT_TEST_HTTP_VERSION=1 by default. There's the "!HTTP2" additions, which you mention you'll be "fixing momentarily", but this is a stand-alone patch, so maybe I'm missing that context...
