[pull] master from git:master#228
Merged
Merged
Conversation
Since 663d7ab (http: reject unsupported proxy URL schemes, 2026-05-05), set_curl_proxy_type() returns 0 only for the "http" and SOCKS variants via dedicated early returns, and -1 for everything else. The "https" branch configures the CURL handle for HTTPS proxying but then falls through to the trailing `return -1` intended for unknown schemes, so the caller in get_curl_handle() treats a perfectly valid https:// proxy URL as unsupported and refuses to use it. Noticed while looking into a Coverity report against the same function; the unchecked curl_easy_setopt() return values it flags are orthogonal to this fix. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since enabling more tests with 7a094d6 (ci: run expensive tests on push builds to integration branches, 2026-05-08), we sometimes see test failures or timeouts in GitHub CI. The culprit seems to be the "enormous ref negotiation" test in t5551, which creates ~100k tag refs in our http server-side repo. Iterating through the loose refs of this repo to generate a ref advertisement can take a long time, especially on a platform with slow I/O. On my otherwise unloaded local machine, a cold cache ref advertisement takes ~10s. On a busy CI machine running tests in parallel, it can presumably top 60s, which runs afoul of Apache's default CGI timeout. The result in t5551 is a test failure, where Apache simply hangs up the connection and the client reports an error. But worse, t5559 runs the same test with HTTP/2, and a bug in Apache causes the connection to hang indefinitely! We eventually see this as a CI timeout after 6 hours. Let's bump Apache's timeout to something much larger: 600 seconds. This doesn't eliminate the possibility of a timeout, but it makes it much less likely. It should eliminate both the test failures and the CI timeouts in practice, and it protects us from running into similar problems with other tests in the future. There are two counter-arguments to consider. One, could/should we just make the test faster? Probably yes. The biggest mistake here is having such an absurd number of unpacked refs on a system which is bottle-necked on I/O. But I think it's worth bumping the timeout so that we can fix this (and possibly other) correctness issues, and then consider performance separately (which we'll do in subsequent patches). And two, is this just papering over a problem that users might see in the real world? We could teach Git to handle this case more gracefully with optimizations or keep-alives. But I think it's really an artificial situation. You need a combination of this silly number of loose refs, plus a very heavily loaded system. If you were trying to run a real server and it took more than 60s to generate the ref advertisement, I don't think the timeout is your biggest problem. Your crappy service is, and you should adjust your resources to match your load. I.e., it is probably reasonable for Git to assume that advertisements happen fast-ish and don't need protocol-level keepalives. Though the patch here is small, tons of work went into analyzing the problem. Many thanks to the contributors credited below. Helped-by: Michael Montalbo <mmontalbo@gmail.com> Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We lost ability to use https:// proxies during this cycle; this is a hotfix for the regression. * js/http-https-proxy-fix: http: accept https:// proxies again
Most of the t5551 http fetch tests use a handful of refs. But there are a few test cases which check our handling of large numbers of refs. These tests use the same server-side repo, so all subsequent tests end up having to consider those extra refs, too. The result is that the test script is a bit slower than it needs to be. In a normal run, moving the "2,000 tags" test into its own repo drops my runtime for the whole script from ~2.7s to ~1.9s. This is a modest gain, but when we add the "--long" flag it gets much bigger. There we trigger a test (marked with EXPENSIVE) that adds 100,000 tags, and the script runtime jumps to ~95s. But if we use the same "many tags" repo for that, our runtime drops to just ~37s. This is a pretty easy win to drop the cost of the script. It may even be a larger gain on a heavily loaded system, since one of the main costs here is unpacked refs, which are heavy on system time and I/O costs. It's possible we are reducing test coverage, since all of those other tests were inadvertently using large ref advertisements (and thus could have uncovered some unexpected interaction). But that seems somewhat unlikely; the tests targeted at the large number of refs are doing roughly similar things to the other tests. Note that the real performance culprit is the 100k-tag --long test, not the 2k-tag one. So we could just let the 100k one use its own repo, and keep the 2k tags in the main repo. But since these two tests are somewhat interlinked, it's easier to just move them both (and it does provide a small gain even for the 2000-tag test). I also notice that the 2000-tag test is gated on the CMDLINE_LIMIT prereq, and without that the later EXPENSIVE test will fail (since we won't have a too-many-refs clone). Nobody seems to have noticed or complained after many years, and I left it alone for this patch. Signed-off-by: Jeff King <peff@peff.net> [jc: made the new "many-tags.git" bare to match the original "repo.git"] Signed-off-by: Junio C Hamano <gitster@pobox.com>
The Apache timeout in HTTP tests has been increased to prevent test failures on heavily loaded CI runners. The tests creating an enormous number of refs have been isolated to their own repositories to avoid slowing down subsequent tests. * jk/t5551-expensive-test-timeouts-fix: t5551: put many-tags case into its own repo t/lib-httpd: bump apache timeout
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )