Re: [PATCH] http: match headers case-insensitively when redacting

On Wed, Sep 22 2021, Jeff King wrote:

On Wed, Sep 22, 2021 at 09:32:41AM +0700, Bagas Sanjaya wrote:

quoted

quoted

but this has a few issues:

   - it's not necessarily portable. The http2 apache module might not be
     available on all systems. Further, the http2 module isn't compatible
     with the prefork mpm, so we have to switch to something else. But we
     don't necessarily know what's available. It would be nice if we
     could have conditional config, but IfModule only tells us if a
     module is already loaded, not whether it is available at all.

     This might be a non-issue. The http tests are already optional, and
     modern-enough systems may just have both of these. But...

   - if we do this, then we'd no longer be testing HTTP/1.1 at all. I'm
     not sure how much that matters since it's all handled by curl under
     the hood, but I'd worry that some detail leaks through. We'd
     probably want two scripts running similar tests, one with HTTP/2 and
     one with HTTP/1.1.

Maybe for httpd config we can say that if mpm_prefork isn't loaded, load
mpm_event and mod_http2.

That doesn't work. We can say "is mpm_prefork" loaded, and indeed we
already do, in order to load mpm_prefork! That's because the module may
or may not be built-in, and if not, we have to load it (or some mpm
module). See 296f0b3ea9 (t/lib-httpd/apache.conf: configure an MPM
module for apache 2.4, 2013-06-09).

But we have no way of knowing _which_ modules are available. It may just
be that "event" or "worker" (both of which support mod_http2) are
available close enough to everywhere that we can just guess.

quoted

And for testing both HTTP/2 and HTTP/1.1 did you mean sharing the same test
code (with adjustments for each protocol)?

Yes. I'd literally run the same battery of tests against both protocols
(see my other response to Taylor with a sketched-out example). I'm still
not sure it's entirely worth the effort, though. The underlying
transport should be pretty transparent to Git, with the exception of
things like debugging output.

Maybe I'm missing something, but it seems to me that trying to figure
out if we support http v2 or not beforehand is the wrong thing to do in
this case. Why don't we simply try to start the server, and fail and
skip_all="sorry, no httpv2" if it fails?

Then have 2 test files:

t1234-http-v1.sh
t1235-http-v2.sh

Where the latter includes the former (or is a symlink with a $0 check),
or both include a library. Doing it this way also means you'll get a
message you notice via "prove", since you won't run all v1 tests in one
file, then skip some v2.

It also means we could add "ssl" in that mix and have 4x files, and
unlike a GIT_TEST_* mode or shoving it all in one test we can run these
in parallel and test all combinations in one test run.