* [PATCH 1/3] t/lib-httpd: bump apache timeout
2026-06-28 7:57 ` [PATCH 0/3] fixing expensive http test timeouts Jeff King
@ 2026-06-28 8:00 ` Jeff King
2026-07-02 3:24 ` Michael Montalbo
2026-06-28 8:03 ` [PATCH 2/3] t5551: put many-tags case into its own repo Jeff King
` (2 subsequent siblings)
3 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-28 8:00 UTC (permalink / raw)
To: Michael Montalbo; +Cc: Patrick Steinhardt, git, Junio C Hamano
Since enabling more tests with 7a094d68a2 (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>
---
I didn't reference Michael's bugzilla report directly, because you can't
read it without a login. :(
Maybe it's worth doing anyway?
t/lib-httpd/apache.conf | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 40a690b0bb..4149fc1078 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -4,6 +4,7 @@ DocumentRoot www
LogFormat "%h %l %u %t \"%r\" %>s %b" common
CustomLog access.log common
ErrorLog error.log
+Timeout 600
<IfModule !mod_log_config.c>
LoadModule log_config_module modules/mod_log_config.so
</IfModule>
--
2.55.0.rc2.353.gf769b6597e
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 2/3] t5551: put many-tags case into its own repo
2026-06-28 7:57 ` [PATCH 0/3] fixing expensive http test timeouts Jeff King
2026-06-28 8:00 ` [PATCH 1/3] t/lib-httpd: bump apache timeout Jeff King
@ 2026-06-28 8:03 ` Jeff King
2026-06-28 21:44 ` Junio C Hamano
2026-06-28 8:07 ` [PATCH 3/3] t5551: pack refs after creating many tags Jeff King
2026-06-29 7:33 ` [PATCH 0/3] fixing expensive http test timeouts Patrick Steinhardt
3 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-28 8:03 UTC (permalink / raw)
To: Michael Montalbo; +Cc: Patrick Steinhardt, git, Junio C Hamano
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>
---
t/t5551-http-fetch-smart.sh | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e236e526f0..cd851f24b8 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -397,15 +397,16 @@ create_tags () {
}
test_expect_success 'create 2,000 tags in the repo' '
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
(
- cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
create_tags 1 2000
)
'
test_expect_success CMDLINE_LIMIT \
'clone the 2,000 tag repo to check OS command line overflow' '
- run_with_limited_cmdline git clone $HTTPD_URL/smart/repo.git too-many-refs &&
+ run_with_limited_cmdline git clone $HTTPD_URL/smart/many-tags.git too-many-refs &&
(
cd too-many-refs &&
git for-each-ref refs/tags >actual &&
@@ -483,12 +484,12 @@ test_expect_success 'test allowanysha1inwant with unreachable' '
test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
test_when_finished "rm -f tags" &&
(
- cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
create_tags 2001 50000
) &&
git -C too-many-refs fetch -q --tags &&
(
- cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
create_tags 50001 100000
) &&
git -C too-many-refs fetch -q --tags &&
--
2.55.0.rc2.353.gf769b6597e
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
2026-06-28 8:03 ` [PATCH 2/3] t5551: put many-tags case into its own repo Jeff King
@ 2026-06-28 21:44 ` Junio C Hamano
2026-06-29 0:34 ` Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2026-06-28 21:44 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, Patrick Steinhardt, git
Jeff King <peff@peff.net> writes:
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index e236e526f0..cd851f24b8 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -397,15 +397,16 @@ create_tags () {
> }
>
> test_expect_success 'create 2,000 tags in the repo' '
> + git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> (
> - cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> + cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> create_tags 1 2000
> )
> '
While all the other repositories used in this tests are bare
repositories, this new one is a non-bare repository.
It shouldn't make any difference, but since I noticed it...
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
2026-06-28 21:44 ` Junio C Hamano
@ 2026-06-29 0:34 ` Jeff King
2026-06-29 14:42 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-29 0:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Montalbo, Patrick Steinhardt, git
On Sun, Jun 28, 2026 at 02:44:32PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> > index e236e526f0..cd851f24b8 100755
> > --- a/t/t5551-http-fetch-smart.sh
> > +++ b/t/t5551-http-fetch-smart.sh
> > @@ -397,15 +397,16 @@ create_tags () {
> > }
> >
> > test_expect_success 'create 2,000 tags in the repo' '
> > + git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> > (
> > - cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > + cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
> > create_tags 1 2000
> > )
> > '
>
> While all the other repositories used in this tests are bare
> repositories, this new one is a non-bare repository.
>
> It shouldn't make any difference, but since I noticed it...
Ah, yeah. It should work either way, but it is slightly confusing for it
to be non-bare. I'll wait to re-send (though if nothing else comes up,
it may be simpler for you to just amend on your side).
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
2026-06-29 0:34 ` Jeff King
@ 2026-06-29 14:42 ` Junio C Hamano
2026-06-29 20:36 ` Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2026-06-29 14:42 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, Patrick Steinhardt, git
Jeff King <peff@peff.net> writes:
> On Sun, Jun 28, 2026 at 02:44:32PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
>> > index e236e526f0..cd851f24b8 100755
>> > --- a/t/t5551-http-fetch-smart.sh
>> > +++ b/t/t5551-http-fetch-smart.sh
>> > @@ -397,15 +397,16 @@ create_tags () {
>> > }
>> >
>> > test_expect_success 'create 2,000 tags in the repo' '
>> > + git init "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
>> > (
>> > - cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
>> > + cd "$HTTPD_DOCUMENT_ROOT_PATH/many-tags.git" &&
>> > create_tags 1 2000
>> > )
>> > '
>>
>> While all the other repositories used in this tests are bare
>> repositories, this new one is a non-bare repository.
>>
>> It shouldn't make any difference, but since I noticed it...
>
> Ah, yeah. It should work either way, but it is slightly confusing for it
> to be non-bare. I'll wait to re-send (though if nothing else comes up,
> it may be simpler for you to just amend on your side).
OK. It seems both Patrick and you are in favor of using only [1/3]
& [2/3] but dropping [3/3]? If that is the concensus I can just
tweak this one and apply before 2.55 final.
Thanks.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/3] t5551: put many-tags case into its own repo
2026-06-29 14:42 ` Junio C Hamano
@ 2026-06-29 20:36 ` Jeff King
0 siblings, 0 replies; 40+ messages in thread
From: Jeff King @ 2026-06-29 20:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Montalbo, Patrick Steinhardt, git
On Mon, Jun 29, 2026 at 07:42:31AM -0700, Junio C Hamano wrote:
> > Ah, yeah. It should work either way, but it is slightly confusing for it
> > to be non-bare. I'll wait to re-send (though if nothing else comes up,
> > it may be simpler for you to just amend on your side).
>
> OK. It seems both Patrick and you are in favor of using only [1/3]
> & [2/3] but dropping [3/3]? If that is the concensus I can just
> tweak this one and apply before 2.55 final.
Yep, that sounds great. Looks like it already happened. :)
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-28 7:57 ` [PATCH 0/3] fixing expensive http test timeouts Jeff King
2026-06-28 8:00 ` [PATCH 1/3] t/lib-httpd: bump apache timeout Jeff King
2026-06-28 8:03 ` [PATCH 2/3] t5551: put many-tags case into its own repo Jeff King
@ 2026-06-28 8:07 ` Jeff King
2026-06-28 21:25 ` Junio C Hamano
2026-06-29 5:57 ` Patrick Steinhardt
2026-06-29 7:33 ` [PATCH 0/3] fixing expensive http test timeouts Patrick Steinhardt
3 siblings, 2 replies; 40+ messages in thread
From: Jeff King @ 2026-06-28 8:07 UTC (permalink / raw)
To: Michael Montalbo; +Cc: Patrick Steinhardt, git, Junio C Hamano
We have two tests that create 2,000 and 100,000 tags respectively.
After doing so, the resulting state can be a bit slow to work with when
using the "files" ref backend, as each of those refs is in its own file.
This isn't a very realistic scenario, as we'd expect most of those refs
to be packed. If they accrue over time along with objects, they'd get
packed by maintenance/gc runs. And if you have a process that creates a
ton of refs at once (like a big fast-import), the usual recommendation
is to run maintenance afterwards.
So let's follow that recommendation and pack the refs ourselves.
Unfortunately, this does not seem to produce an improvement to the
run-time of the test script! That's because after producing this state,
we perform only a few fetches of it. And packing the refs costs at least
as much as serving a ref advertisement (both have to iterate the refs,
but packing additionally must write .lock files as we pack).
My wall-clock time was slightly improved (but within the noise) with
this patch, but my user and system CPU time were slightly worse!
However, on a loaded system with I/O bottlenecks, it may be a net win.
That's somewhat of a guess, though.
It would be nice if we had a way to generate all of these refs without
writing so many individual files. But even if we taught the ref code to
write large cases directly to the packed-refs file, we'd still need to
take individual locks. The real solution is a backend like reftable,
which shaves ~30% off of the test runtime.
Signed-off-by: Jeff King <peff@peff.net>
---
I'm iffy on whether this one is worth it.
If you apply just this patch without patch 2, then the run-time does
improve quite a bit. The cost of packing is amortized by the improved
performance for all of those subsequent tests (but after patch 2, they
never even see the unpacked state).
Likewise, I suspect this would make our timeout problems go away even
without patch 1.
So the whole series _could_ be reduced to just this one patch. But
hopefully the reasoning given in the earlier patches makes sense, at
which point this one is kind of superfluous.
t/t5551-http-fetch-smart.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index cd851f24b8..e2e729216f 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -393,6 +393,7 @@ create_tags () {
tag=$(perl -e "print \"bla\" x 30") &&
sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
git update-ref --stdin <input &&
+ git pack-refs --all &&
rm input
}
--
2.55.0.rc2.353.gf769b6597e
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-28 8:07 ` [PATCH 3/3] t5551: pack refs after creating many tags Jeff King
@ 2026-06-28 21:25 ` Junio C Hamano
2026-06-29 5:57 ` Patrick Steinhardt
1 sibling, 0 replies; 40+ messages in thread
From: Junio C Hamano @ 2026-06-28 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, Patrick Steinhardt, git
Jeff King <peff@peff.net> writes:
> So let's follow that recommendation and pack the refs ourselves.
> Unfortunately, this does not seem to produce an improvement to the
> run-time of the test script! That's because after producing this state,
> we perform only a few fetches of it. And packing the refs costs at least
> as much as serving a ref advertisement (both have to iterate the refs,
> but packing additionally must write .lock files as we pack).
Testing a pathological set-up with too many loose refs may have
extra value, as long as we are also testing the recommended set-up,
so ideally we should have both ;-) but if we have to pick only one
and drop the other, we probably should be testing the packed case.
> I'm iffy on whether this one is worth it.
I am ambivalent, too, about this change for the purpose of the
"yeek, apache times out while enumerating refs" issue. But see
above ;-)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-28 8:07 ` [PATCH 3/3] t5551: pack refs after creating many tags Jeff King
2026-06-28 21:25 ` Junio C Hamano
@ 2026-06-29 5:57 ` Patrick Steinhardt
2026-06-29 20:35 ` Jeff King
1 sibling, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-06-29 5:57 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
On Sun, Jun 28, 2026 at 04:07:10AM -0400, Jeff King wrote:
> We have two tests that create 2,000 and 100,000 tags respectively.
> After doing so, the resulting state can be a bit slow to work with when
> using the "files" ref backend, as each of those refs is in its own file.
>
> This isn't a very realistic scenario, as we'd expect most of those refs
> to be packed. If they accrue over time along with objects, they'd get
> packed by maintenance/gc runs. And if you have a process that creates a
> ton of refs at once (like a big fast-import), the usual recommendation
> is to run maintenance afterwards.
>
> So let's follow that recommendation and pack the refs ourselves.
> Unfortunately, this does not seem to produce an improvement to the
> run-time of the test script! That's because after producing this state,
> we perform only a few fetches of it. And packing the refs costs at least
> as much as serving a ref advertisement (both have to iterate the refs,
> but packing additionally must write .lock files as we pack).
> My wall-clock time was slightly improved (but within the noise) with
> this patch, but my user and system CPU time were slightly worse!
> However, on a loaded system with I/O bottlenecks, it may be a net win.
> That's somewhat of a guess, though.
>
> It would be nice if we had a way to generate all of these refs without
> writing so many individual files. But even if we taught the ref code to
> write large cases directly to the packed-refs file, we'd still need to
> take individual locks. The real solution is a backend like reftable,
> which shaves ~30% off of the test runtime.
We kind of already have this with the `REF_TRANSACTION_FLAG_INITIAL`
flag, but right now it is only used when performing a clone or when
migrating references. Also, it requires an empty repository that has no
references yet.
It raises the question whether we could also extend git-fast-import(1)
to use it, as it would typically be run on an almost-empty repository.
It's the "almost" that kills it though, as we already do have at least
the HEAD reference. So it could be feasible, but it's not as trivial as
just setting the flag and then we're magically faster.
And besides, in this particular test here we run git-fast-import(1)
multiple times in the same repository, so it wouldn't help us.
We could of course extend all of this so that Git is able to write into
the packed-refs directly, even with preexisting refs. But I agree with
your sentiment: it doesn't feel worth it as the reftable backend fixes
scenarios like this anyway.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I'm iffy on whether this one is worth it.
>
> If you apply just this patch without patch 2, then the run-time does
> improve quite a bit. The cost of packing is amortized by the improved
> performance for all of those subsequent tests (but after patch 2, they
> never even see the unpacked state).
>
> Likewise, I suspect this would make our timeout problems go away even
> without patch 1.
>
> So the whole series _could_ be reduced to just this one patch. But
> hopefully the reasoning given in the earlier patches makes sense, at
> which point this one is kind of superfluous.
Agreed. I'd just merge the first two patches and drop this one here.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-29 5:57 ` Patrick Steinhardt
@ 2026-06-29 20:35 ` Jeff King
2026-06-30 9:05 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-29 20:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
On Mon, Jun 29, 2026 at 07:57:21AM +0200, Patrick Steinhardt wrote:
> > It would be nice if we had a way to generate all of these refs without
> > writing so many individual files. But even if we taught the ref code to
> > write large cases directly to the packed-refs file, we'd still need to
> > take individual locks. The real solution is a backend like reftable,
> > which shaves ~30% off of the test runtime.
>
> We kind of already have this with the `REF_TRANSACTION_FLAG_INITIAL`
> flag, but right now it is only used when performing a clone or when
> migrating references. Also, it requires an empty repository that has no
> references yet.
>
> It raises the question whether we could also extend git-fast-import(1)
> to use it, as it would typically be run on an almost-empty repository.
> It's the "almost" that kills it though, as we already do have at least
> the HEAD reference. So it could be feasible, but it's not as trivial as
> just setting the flag and then we're magically faster.
>
> And besides, in this particular test here we run git-fast-import(1)
> multiple times in the same repository, so it wouldn't help us.
>
> We could of course extend all of this so that Git is able to write into
> the packed-refs directly, even with preexisting refs. But I agree with
> your sentiment: it doesn't feel worth it as the reftable backend fixes
> scenarios like this anyway.
Yup. In the past I've pondered exposing this via update-ref, but I think
it's too weird and/or dangerous to do so. Especially because you are
still stuck creating all of the .lock files, so the performance is not
even that much better (though it does save you doing so _twice_ when you
then pack the refs).
So the performance option you really want is "YOLO, just write some
packed-refs without locking". But that is not something I think we want
to expose to users. ;)
We could do it ad-hoc within this test like so:
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index dcff0bc7d4..276c7ac002 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -389,11 +389,15 @@ create_tags () {
echo "from :$1"
done | git fast-import --export-marks=marks &&
+ # should be mostly a noop, but makes sure we have the right header
+ git pack-refs &&
+
# now assign tags to all the dangling commits we created above
+ # It is OK to write directly to the packed-refs file because we know
+ # that our entries are sorted by refname, and that they all
+ # come after what we wrote earlier.
tag=$(perl -e "print \"bla\" x 30") &&
- sed -e "s|^:\([^ ]*\) \(.*\)$|create refs/tags/$tag-\1 \2|" <marks >input &&
- git update-ref --stdin <input &&
- rm input
+ sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
}
test_expect_success 'create 2,000 tags in the repo' '
That gives us the same ~30% speedup that using reftables does, but it
still is quite gross and fragile. And it is not even strictly correct,
because we don't zero-pad the numbered tags (so our file is subtly out
of order). Plus it would need to be conditional on the ref backend
being used. Yuck.
There's one other thing you might find interesting. While poking at the
timings here the other day, I noticed that reftable is very eager to
stat the tables.list file. Try this:
git init --ref-format=reftable
blob=$(echo foo | git hash-object -w --stdin)
seq -f "create refs/tags/foo-%g $blob" 2000 |
strace -c git update-ref --stdin
We make 2000 fstat, which strace claims takes 85% of the time. I suspect
this is over-emphasized because strace inherently makes syscalls slow,
but running with perf also highlights it as a non-trivial cost.
It has been a long time since I've thought about reftable internals, but
it feels like we ought to be able to take the lock and then trust that
the stack has not been manipulated.
It may not be worth digging into too much, though. I can make 50,000
refs in 150ms on my system, which is probably good enough (especially
compared to the files backend).
-Peff
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-29 20:35 ` Jeff King
@ 2026-06-30 9:05 ` Patrick Steinhardt
2026-06-30 23:47 ` Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-06-30 9:05 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
On Mon, Jun 29, 2026 at 04:35:27PM -0400, Jeff King wrote:
> There's one other thing you might find interesting. While poking at the
> timings here the other day, I noticed that reftable is very eager to
> stat the tables.list file. Try this:
>
> git init --ref-format=reftable
> blob=$(echo foo | git hash-object -w --stdin)
> seq -f "create refs/tags/foo-%g $blob" 2000 |
> strace -c git update-ref --stdin
>
> We make 2000 fstat, which strace claims takes 85% of the time. I suspect
> this is over-emphasized because strace inherently makes syscalls slow,
> but running with perf also highlights it as a non-trivial cost.
Yeah, this rings a bell. If I remember correctly, this is because we
call `refs_resolve_ref_unsafe()` to verify whether the target already
exists. And as that function is generic, it wasn't easy to optimize it
by reusing the already-loaded reftable stack.
> It has been a long time since I've thought about reftable internals, but
> it feels like we ought to be able to take the lock and then trust that
> the stack has not been manipulated.
Yeah, that would certainly be an option to explore.
> It may not be worth digging into too much, though. I can make 50,000
> refs in 150ms on my system, which is probably good enough (especially
> compared to the files backend).
True, it's going to be much better compared to the "files" backend. But
that isn't enough reason to not optimize it even further -- doubly so if
it actually takes 85% of the time. Sounds like a low-hanging fruit to me
that can result in a significant speedup.
I'll probably not get to it anytime soon, but I'll create an issue to
keep track of it.
Patrick
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-30 9:05 ` Patrick Steinhardt
@ 2026-06-30 23:47 ` Jeff King
2026-06-30 23:58 ` weird quadratic reftable behavior, was: " Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-30 23:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
On Tue, Jun 30, 2026 at 11:05:22AM +0200, Patrick Steinhardt wrote:
> > We make 2000 fstat, which strace claims takes 85% of the time. I suspect
> > this is over-emphasized because strace inherently makes syscalls slow,
> > but running with perf also highlights it as a non-trivial cost.
>
> Yeah, this rings a bell. If I remember correctly, this is because we
> call `refs_resolve_ref_unsafe()` to verify whether the target already
> exists. And as that function is generic, it wasn't easy to optimize it
> by reusing the already-loaded reftable stack.
I think it's similar to that. I dug down and got some actual performance
numbers below for eliminating the fstat() calls, with the conclusion
that it's probably not worth spending too much time digging into it. But
details below for completeness.
I was wrong to say fstat() before, it's a regular stat(). The
interesting backtrace is:
#0 __GI___stat64 (file=0x555555ab0bf0 "/home/peff/tmp/.git/reftable/tables.list", buf=0x7fffffffd630)
at ../sysdeps/unix/sysv/linux/stat64.c:29
#1 0x0000555555832ad6 in stack_uptodate (st=0x555555ab0ae0) at reftable/stack.c:575
#2 0x0000555555832c5c in reftable_stack_reload (st=0x555555ab0ae0) at reftable/stack.c:625
#3 0x000055555581f690 in backend_for (out=0x7fffffffd798, store=0x555555ab0900,
refname=0x555555aae178 "refs/tags/foo-1", rewritten_ref=0x7fffffffd780, reload=1) at refs/reftable-backend.c:283
#4 0x0000555555824957 in reftable_be_reflog_exists (ref_store=0x555555ab0900,
refname=0x555555aae178 "refs/tags/foo-1") at refs/reftable-backend.c:2266
#5 0x000055555581393a in refs_reflog_exists (refs=0x555555ab0900, refname=0x555555aae178 "refs/tags/foo-1")
at refs.c:2995
#6 0x000055555581f730 in should_write_log (refs=0x555555ab0900, refname=0x555555aae178 "refs/tags/foo-1")
at refs/reftable-backend.c:301
#7 0x00005555558224df in write_transaction_table (writer=0x5555569abe30, cb_data=0x5555569ab820)
at refs/reftable-backend.c:1511
#8 0x000055555583354d in reftable_addition_add (add=0x5555569ab050,
write_table=0x5555558220f0 <write_transaction_table>, arg=0x5555569ab820) at reftable/stack.c:902
#9 0x0000555555822b2b in reftable_be_transaction_finish (ref_store=0x555555ab0900, transaction=0x555555ab0c80,
err=0x7fffffffdbc0) at refs/reftable-backend.c:1633
#10 0x0000555555813161 in ref_transaction_commit (transaction=0x555555ab0c80, err=0x7fffffffdbc0) at refs.c:2769
#11 0x00005555556956c9 in update_refs_stdin (flags=0) at builtin/update-ref.c:789
So we are asking about reflogs for each ref under the "only reflog if a
log already exists" rule. Which means we can easily disable it by
setting core.logallrefupdates to "always", giving us a way to measure
the impact. So we can try:
git init --ref-format=reftable
blob=$(echo foo | git hash-object -w --stdin)
seq -f "create refs/tags/foo-%g $blob" 50000 >input
cp -a .git/reftable reftable.orig
hyperfine \
-p 'rm -rf .git/reftable; cp -a reftable.orig .git/reftable' \
-L config true,always \
'git -c core.logallrefupdates={config} update-ref --stdin <input'
which yields:
Benchmark 1: git -c core.logallrefupdates=true update-ref --stdin <input
Time (mean ± σ): 128.8 ms ± 1.5 ms [User: 97.1 ms, System: 31.7 ms]
Range (min … max): 126.4 ms … 131.3 ms 23 runs
Benchmark 2: git -c core.logallrefupdates=always update-ref --stdin <input
Time (mean ± σ): 195.2 ms ± 1.7 ms [User: 182.1 ms, System: 13.0 ms]
Range (min … max): 191.9 ms … 197.6 ms 15 runs
So we saved all of those stat() calls, but writing the actual reflog
entries adds much more cost!
Sadly there is no "never" option for core.logallrefupdates. I hacked one
in and the result took ~96ms to run. So that tells us the cost of the
stat() checks: around 25% of the runtime.
Which is a big-ish percentage, but a small absolute number. It's 0.64us
per ref. Perhaps not worrying about too much.
The other interesting thing I noticed is that we seem to write() entries
individually with no buffering. Probably we could get an easy speedup
for big cases like this by using stdio in the fd_writer abstraction.
We're again pinching pennies to some degree, but I expect we may be able
to drop the 120ms case down to 60ms or so (just guessing based on the
extra reflog writes adding ~70ms).
There was one other oddity I didn't quite resolve. You may notice the
gross reftable.orig stuff in hyperfine. I originally wrote this as:
git for-each-ref --format="delete %(refname)" | git update-ref --stdin
but for some reason that causes the subsequent update-ref to loop
infinitely on merged_iter_next_entry(). It does so reliably, but I can't
reproduce it outside of hyperfine. Super weird, and I'm sure I'm missing
something obvious.
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread* weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-30 23:47 ` Jeff King
@ 2026-06-30 23:58 ` Jeff King
2026-07-01 6:17 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-06-30 23:58 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
On Tue, Jun 30, 2026 at 07:47:02PM -0400, Jeff King wrote:
> There was one other oddity I didn't quite resolve. You may notice the
> gross reftable.orig stuff in hyperfine. I originally wrote this as:
>
> git for-each-ref --format="delete %(refname)" | git update-ref --stdin
>
> but for some reason that causes the subsequent update-ref to loop
> infinitely on merged_iter_next_entry(). It does so reliably, but I can't
> reproduce it outside of hyperfine. Super weird, and I'm sure I'm missing
> something obvious.
Ah, maybe not infinite, but probably quadratic. The key is that you have
to delete a lot of refs and then try to insert them again. So with this
script:
nr=$1; shift
rm -rf .git
git init --ref-format=reftable
blob=$(echo foo | git hash-object -w --stdin)
seq -f "create refs/tags/foo-%g $blob" $nr >input
git update-ref --stdin <input
git for-each-ref --format="delete %(refname)" | git update-ref --stdin
time git update-ref --stdin <input
I get results like this:
nr | runtime
------------
1000 | 0.125s
2000 | 0.454s
4000 | 1.811s
8000 | 7.091s
So for every doubling of the input size, the runtime quadruples. I guess
it is iterating through some deleted tombstone entries, but I'm not sure
why.
That's probably a more interesting and productive performance problem to
work on than micro-optimizing out the last few microseconds of writing. :)
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-06-30 23:58 ` weird quadratic reftable behavior, was: " Jeff King
@ 2026-07-01 6:17 ` Patrick Steinhardt
2026-07-01 8:00 ` Jeff King
0 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-07-01 6:17 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
On Tue, Jun 30, 2026 at 07:58:50PM -0400, Jeff King wrote:
> On Tue, Jun 30, 2026 at 07:47:02PM -0400, Jeff King wrote:
>
> > There was one other oddity I didn't quite resolve. You may notice the
> > gross reftable.orig stuff in hyperfine. I originally wrote this as:
> >
> > git for-each-ref --format="delete %(refname)" | git update-ref --stdin
> >
> > but for some reason that causes the subsequent update-ref to loop
> > infinitely on merged_iter_next_entry(). It does so reliably, but I can't
> > reproduce it outside of hyperfine. Super weird, and I'm sure I'm missing
> > something obvious.
>
> Ah, maybe not infinite, but probably quadratic. The key is that you have
> to delete a lot of refs and then try to insert them again. So with this
> script:
>
> nr=$1; shift
> rm -rf .git
>
> git init --ref-format=reftable
> blob=$(echo foo | git hash-object -w --stdin)
> seq -f "create refs/tags/foo-%g $blob" $nr >input
> git update-ref --stdin <input
> git for-each-ref --format="delete %(refname)" | git update-ref --stdin
> time git update-ref --stdin <input
>
> I get results like this:
>
> nr | runtime
> ------------
> 1000 | 0.125s
> 2000 | 0.454s
> 4000 | 1.811s
> 8000 | 7.091s
>
> So for every doubling of the input size, the runtime quadruples. I guess
> it is iterating through some deleted tombstone entries, but I'm not sure
> why.
>
> That's probably a more interesting and productive performance problem to
> work on than micro-optimizing out the last few microseconds of writing. :)
This is a known issue, I think [1].
The problem here is the tombstoning: when you delete all references,
chances are that they are not truly gone but that every reference is
just tombstoned. The problem with this is that reading refs may now take
signifciantly more time as we cannot just say "this stack is empty".
Instead, we need to figure out that it is empty by processing all the
tombstones, and that takes a lot of time.
I remember that I did some digging back then and improved the status quo
quite significantly by optimizing `refs_verify_refname_available()`. I'm
sure there are more opportunities for optimization here though -- I have
a feeling that we for example exhaust the merged iterator until its end
when searching for a specific refname, where we could easily abort once
the observed tombstone name sorts lexicographically after the needle.
But eventually I decided to not care too much about this edge case, as
it seems very specific to this artificial benchmark scenario. Which of
course doesn't mean that it's not worth doing, I just had bigger fish to
fry and didn't get around to it yet.
Patrick
[1]: <Z602dzQggtDdcgCX@tapette.crustytoothpaste.net>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-07-01 6:17 ` Patrick Steinhardt
@ 2026-07-01 8:00 ` Jeff King
2026-07-01 9:04 ` Kristofer Karlsson
0 siblings, 1 reply; 40+ messages in thread
From: Jeff King @ 2026-07-01 8:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
On Wed, Jul 01, 2026 at 08:17:45AM +0200, Patrick Steinhardt wrote:
> This is a known issue, I think [1].
>
> The problem here is the tombstoning: when you delete all references,
> chances are that they are not truly gone but that every reference is
> just tombstoned. The problem with this is that reading refs may now take
> signifciantly more time as we cannot just say "this stack is empty".
> Instead, we need to figure out that it is empty by processing all the
> tombstones, and that takes a lot of time.
>
> I remember that I did some digging back then and improved the status quo
> quite significantly by optimizing `refs_verify_refname_available()`. I'm
> sure there are more opportunities for optimization here though -- I have
> a feeling that we for example exhaust the merged iterator until its end
> when searching for a specific refname, where we could easily abort once
> the observed tombstone name sorts lexicographically after the needle.
Yeah, it is (mostly) the same problem. About half the time is spent in
refs_verify_refnames_available().
The other half is in reftable_be_transaction_prepare(). Looks like it
makes individual calls to prepare_single_update(), which reads each ref.
And those reads are expensive because of all of the tombstones. It might
be possible to do an iterator merge or similar between the sorted list
of transaction refs and the reftable contents.
> But eventually I decided to not care too much about this edge case, as
> it seems very specific to this artificial benchmark scenario. Which of
> course doesn't mean that it's not worth doing, I just had bigger fish to
> fry and didn't get around to it yet.
Yeah, that's fair. I dug a bit further in case there was anything useful
to write up, but I don't have much to add beyond what's here and in the
thread you linked. We can let it live on in the archive for now.
-Peff
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-07-01 8:00 ` Jeff King
@ 2026-07-01 9:04 ` Kristofer Karlsson
2026-07-01 10:07 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Kristofer Karlsson @ 2026-07-01 9:04 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, Michael Montalbo, git, Junio C Hamano
On Wed, 1 Jul 2026 at 10:00, Jeff King <peff@peff.net> wrote:
> Yeah, it is (mostly) the same problem. About half the time is spent in
> refs_verify_refnames_available().
>
> The other half is in reftable_be_transaction_prepare(). Looks like it
> makes individual calls to prepare_single_update(), which reads each ref.
> And those reads are expensive because of all of the tombstones. It might
> be possible to do an iterator merge or similar between the sorted list
> of transaction refs and the reftable contents.
Hi, sorry for jumping in -- I found this interesting and started
poking at the code. I think both halves may share the same root
cause.
The merged iterator's suppress_deletions flag filters out tombstones
internally, which means higher-level code with prefix or refname
bounds never gets a chance to stop iteration early. By letting
tombstones pass through and filtering them one layer up in the
reftable backend, the existing bounds checks can kick in before
we scan through all the tombstones.
So instead of doing full scans inside merged_iter_next_void()
we can just delegate to merged_iter_next_entry() and instead
add a loop to reftable_be_reflog_exists() that skips
tombstones (but is amortized O(1)).
Now multiple call sites would need to add something like this
to compensate for returning tombstones:
if (reftable_log_record_is_deletion(&iter->log))
continue;
but it may be worth it if it reduces cost when there are many refs.
The key spot is reftable_ref_iterator_advance(), where the deletion
skip goes right after the existing prefix check -- so a tombstone
past the prefix stops iteration immediately instead of being
silently consumed. The same idea applies to reftable_backend_read_ref()
and the log iteration paths.
I have a local branch with this attempted fix. Rerunning the
benchmark:
Before:
nr=1000 0.306s
nr=2000 0.945s
nr=4000 3.816s
nr=8000 14.93s
After:
nr=1000 0.020s
nr=2000 0.044s
nr=4000 0.071s
nr=8000 0.145s
nr=16000 0.258s
nr=32000 0.591s
I can send a proper patch if needed/wanted, but I might have missed
something silly here.
Thanks,
Kristofer
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-07-01 9:04 ` Kristofer Karlsson
@ 2026-07-01 10:07 ` Patrick Steinhardt
2026-07-03 12:09 ` Kristofer Karlsson
0 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-07-01 10:07 UTC (permalink / raw)
To: Kristofer Karlsson; +Cc: Jeff King, Michael Montalbo, git, Junio C Hamano
On Wed, Jul 01, 2026 at 11:04:45AM +0200, Kristofer Karlsson wrote:
> On Wed, 1 Jul 2026 at 10:00, Jeff King <peff@peff.net> wrote:
>
> > Yeah, it is (mostly) the same problem. About half the time is spent in
> > refs_verify_refnames_available().
> >
> > The other half is in reftable_be_transaction_prepare(). Looks like it
> > makes individual calls to prepare_single_update(), which reads each ref.
> > And those reads are expensive because of all of the tombstones. It might
> > be possible to do an iterator merge or similar between the sorted list
> > of transaction refs and the reftable contents.
>
> Hi, sorry for jumping in -- I found this interesting and started
> poking at the code. I think both halves may share the same root
> cause.
>
> The merged iterator's suppress_deletions flag filters out tombstones
> internally, which means higher-level code with prefix or refname
> bounds never gets a chance to stop iteration early. By letting
> tombstones pass through and filtering them one layer up in the
> reftable backend, the existing bounds checks can kick in before
> we scan through all the tombstones.
Ah, interesting. This kind of hits int o the direction that I proposed
of stopping iteration once we hit the first reference that has a
lexicographically-larget refname. But I thought about having to solve it
generically in the iterator somehow, and I wasn't really looking forward
to that.
Your solutions works around that issue indeed. It of course doesn't
solve reference iteration, which would still be expensive. But it does
solve the issue where we just want to look up a single reference, which
is what Peff noticed to be slow.
> So instead of doing full scans inside merged_iter_next_void()
> we can just delegate to merged_iter_next_entry() and instead
> add a loop to reftable_be_reflog_exists() that skips
> tombstones (but is amortized O(1)).
>
> Now multiple call sites would need to add something like this
> to compensate for returning tombstones:
>
> if (reftable_log_record_is_deletion(&iter->log))
> continue;
>
> but it may be worth it if it reduces cost when there are many refs.
I think it certainly might be.
> The key spot is reftable_ref_iterator_advance(), where the deletion
> skip goes right after the existing prefix check -- so a tombstone
> past the prefix stops iteration immediately instead of being
> silently consumed. The same idea applies to reftable_backend_read_ref()
> and the log iteration paths.
Yup.
> I have a local branch with this attempted fix. Rerunning the
> benchmark:
>
> Before:
> nr=1000 0.306s
> nr=2000 0.945s
> nr=4000 3.816s
> nr=8000 14.93s
>
> After:
> nr=1000 0.020s
> nr=2000 0.044s
> nr=4000 0.071s
> nr=8000 0.145s
> nr=16000 0.258s
> nr=32000 0.591s
>
> I can send a proper patch if needed/wanted, but I might have missed
> something silly here.
Nice gains. I certainly think it would make sense to polish this a bit
and then cast it into a patch.
Patrick
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
2026-07-01 10:07 ` Patrick Steinhardt
@ 2026-07-03 12:09 ` Kristofer Karlsson
0 siblings, 0 replies; 40+ messages in thread
From: Kristofer Karlsson @ 2026-07-03 12:09 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Michael Montalbo, git, Junio C Hamano
On Wed, 1 Jul 2026 at 12:07, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > I can send a proper patch if needed/wanted, but I might have missed
> > something silly here.
>
> Nice gains. I certainly think it would make sense to polish this a bit
> and then cast it into a patch.
>
> Patrick
I have a small draft here https://github.com/gitgitgadget/git/pull/2166
but I am honestly not sure if it's worth submitting as a patch - the
change is somewhat small, but spread out, and I failed to properly
reproduce the performance win in any realistic scenario (I had to
disable compaction to see the improvement).
I would want to rely on your expertise to know if this change
would be valuable to discuss as a patch at all.
Thanks,
Kristofer
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-28 7:57 ` [PATCH 0/3] fixing expensive http test timeouts Jeff King
` (2 preceding siblings ...)
2026-06-28 8:07 ` [PATCH 3/3] t5551: pack refs after creating many tags Jeff King
@ 2026-06-29 7:33 ` Patrick Steinhardt
2026-06-29 14:39 ` Junio C Hamano
3 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-06-29 7:33 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Montalbo, git, Junio C Hamano
On Sun, Jun 28, 2026 at 03:57:16AM -0400, Jeff King wrote:
> On Fri, Jun 26, 2026 at 04:26:28PM -0700, Michael Montalbo wrote:
>
> > I think Peff and Patrick's suggestion to just increase the Apache timeout
> > makes sense. I ran some experiments using a really long timeout with an
> > artificially slowed down CI runner and all the jobs made progress
> > (if slowly) without stalling, and eventually completed successfully:
> >
> > https://github.com/mmontalbo/git/actions/runs/28267019651
> >
> > I haven't spent a lot of time trying to figure out what the right timeout
> > value should be. An hour definitely seems like overkill, with something
> > on the order of 5-10 minutes seeming more reasonable, but I don't
> > have a principled number.
>
> Here are some patches to keep things moving along. I arbitrarily picked
> 10 minutes, because multiplying the 1-minute default by 10 felt right. ;)
>
> The first one just bumps the timeout and should make our problems go
> away. The other two are optimizations, but I'm on the fence on whether
> the final patch is worth it.
>
> Thanks again for all of the digging.
>
> [1/3]: t/lib-httpd: bump apache timeout
> [2/3]: t5551: put many-tags case into its own repo
> [3/3]: t5551: pack refs after creating many tags
By the way, the only reason why we at GitLab haven't been feeling the
pain is that we only enable GIT_TEST_LONG for GitHub. So I was wondering
whether we want to have something like the below patch on top.
Patrick
diff --git a/ci/lib.sh b/ci/lib.sh
index b939110a6e..57801586aa 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -215,6 +215,14 @@ then
test macos != "$CI_OS_NAME" || CI_OS_NAME=osx
CI_REPO_SLUG="$GITHUB_REPOSITORY"
CI_JOB_ID="$GITHUB_RUN_ID"
+
+ case "$GITHUB_EVENT_NAME" in
+ pull_request)
+ CI_EVENT=pull_request;;
+ push)
+ CI_EVENT=push;;
+ esac
+
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
@@ -239,6 +247,13 @@ then
CI_BRANCH="$CI_COMMIT_REF_NAME"
CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_PIPELINE_SOURCE" in
+ merge_request_event)
+ CI_EVENT=pull_request;;
+ push)
+ CI_EVENT=push;;
+ esac
+
case "$OS,$CI_JOB_IMAGE" in
Windows_NT,*)
CI_OS_NAME=windows
@@ -319,7 +334,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
# enable "expensive" tests for PR events.
# In order to catch bugs introduced at integration time by mismerges,
# enable the long tests for pushes to the integration branches as well.
-case "$GITHUB_EVENT_NAME,$CI_BRANCH" in
+case "$CI_EVENT,$CI_BRANCH" in
pull_request,*|push,*next*|push,*master*|push,*main*|push,*maint*)
export GIT_TEST_LONG=YesPlease
;;
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-29 7:33 ` [PATCH 0/3] fixing expensive http test timeouts Patrick Steinhardt
@ 2026-06-29 14:39 ` Junio C Hamano
2026-06-29 16:09 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2026-06-29 14:39 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Michael Montalbo, git
Patrick Steinhardt <ps@pks.im> writes:
> By the way, the only reason why we at GitLab haven't been feeling the
> pain is that we only enable GIT_TEST_LONG for GitHub. So I was wondering
> whether we want to have something like the below patch on top.
If we can afford the cycles, it would be good to have similarly
larger coverage on two different platforms (compared to leaving one
of them not doing as much as the other when we know it). On the
other hand, if we cannot cover _everything_ in one platform, it may
be a better use of the resources to have the other platform things
that are not covered already. I see that among different pipeline
sources, we are doing TEST_LONG for pull requests to any branch, and
pushes only to "cast in stone" branches. If there are other
branches that deserve to be tested with TEST_LONG upon other events
that the existing GitHub Actions CI does not trigger, it may be good
to have GitLab CI cover them, perhaps?
>
> Patrick
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index b939110a6e..57801586aa 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -215,6 +215,14 @@ then
> test macos != "$CI_OS_NAME" || CI_OS_NAME=osx
> CI_REPO_SLUG="$GITHUB_REPOSITORY"
> CI_JOB_ID="$GITHUB_RUN_ID"
> +
> + case "$GITHUB_EVENT_NAME" in
> + pull_request)
> + CI_EVENT=pull_request;;
> + push)
> + CI_EVENT=push;;
> + esac
> +
> CC="${CC_PACKAGE:-${CC:-gcc}}"
> DONT_SKIP_TAGS=t
> handle_failed_tests () {
> @@ -239,6 +247,13 @@ then
> CI_BRANCH="$CI_COMMIT_REF_NAME"
> CI_COMMIT="$CI_COMMIT_SHA"
>
> + case "$CI_PIPELINE_SOURCE" in
> + merge_request_event)
> + CI_EVENT=pull_request;;
> + push)
> + CI_EVENT=push;;
> + esac
> +
> case "$OS,$CI_JOB_IMAGE" in
> Windows_NT,*)
> CI_OS_NAME=windows
> @@ -319,7 +334,7 @@ export SKIP_DASHED_BUILT_INS=YesPlease
> # enable "expensive" tests for PR events.
> # In order to catch bugs introduced at integration time by mismerges,
> # enable the long tests for pushes to the integration branches as well.
> -case "$GITHUB_EVENT_NAME,$CI_BRANCH" in
> +case "$CI_EVENT,$CI_BRANCH" in
> pull_request,*|push,*next*|push,*master*|push,*main*|push,*maint*)
> export GIT_TEST_LONG=YesPlease
> ;;
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-29 14:39 ` Junio C Hamano
@ 2026-06-29 16:09 ` Patrick Steinhardt
2026-06-29 16:19 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-06-29 16:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Michael Montalbo, git
On Mon, Jun 29, 2026 at 07:39:59AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > By the way, the only reason why we at GitLab haven't been feeling the
> > pain is that we only enable GIT_TEST_LONG for GitHub. So I was wondering
> > whether we want to have something like the below patch on top.
>
> If we can afford the cycles, it would be good to have similarly
> larger coverage on two different platforms (compared to leaving one
> of them not doing as much as the other when we know it). On the
> other hand, if we cannot cover _everything_ in one platform, it may
> be a better use of the resources to have the other platform things
> that are not covered already. I see that among different pipeline
> sources, we are doing TEST_LONG for pull requests to any branch, and
> pushes only to "cast in stone" branches. If there are other
> branches that deserve to be tested with TEST_LONG upon other events
> that the existing GitHub Actions CI does not trigger, it may be good
> to have GitLab CI cover them, perhaps?
I'm a bit hesitant to do such a split, mostly because the canonical
source of truth that the project typically uses is GitHub's CI. So I
want us at GitLab to be able to catch the same issues that GitHub would
flag. And if GitLab's CI stopped detecting everything that GitHub does,
then the result would likely be that we often create merge requests on
both platforms, which would only result in more wasted resources.
Patrick
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-29 16:09 ` Patrick Steinhardt
@ 2026-06-29 16:19 ` Junio C Hamano
2026-06-30 9:05 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2026-06-29 16:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Michael Montalbo, git
Patrick Steinhardt <ps@pks.im> writes:
>> pushes only to "cast in stone" branches. If there are other
>> branches that deserve to be tested with TEST_LONG upon other events
>> that the existing GitHub Actions CI does not trigger, it may be good
>> to have GitLab CI cover them, perhaps?
>
> I'm a bit hesitant to do such a split, mostly because the canonical
> source of truth that the project typically uses is GitHub's CI. So I
> want us at GitLab to be able to catch the same issues that GitHub would
> flag. And if GitLab's CI stopped detecting everything that GitHub does,
> then the result would likely be that we often create merge requests on
> both platforms, which would only result in more wasted resources.
I didn't suggest splitting them into two circles that overlap but
each with area only it covers, though. GitLab's coverage can be
superset to GitHub's and that would satify what I suggested.
FWIW, I do not consider GitHub's CI "the canonical source" at all.
It is a very handy service to use to check how well we are doing,
but from time to time it has its own hiccups ;-).
What can we do to make the visibility of GitLab's CI more prominent?
I know where the CI jobs that are triggered when I push out the
integration branches are found at GitHub's website[*], but I do not
think I know the corresponding one at GitLab, for example, and I
think that is a shame.
[Footnote]
*1* I just made https://tinyurl.com/github-gitci that points at
https://github.com/git/git/actions/workflows/main.yml?query=event%3Apush+actor%3Agitster
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-29 16:19 ` Junio C Hamano
@ 2026-06-30 9:05 ` Patrick Steinhardt
2026-06-30 19:21 ` Junio C Hamano
0 siblings, 1 reply; 40+ messages in thread
From: Patrick Steinhardt @ 2026-06-30 9:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Michael Montalbo, git
On Mon, Jun 29, 2026 at 09:19:11AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> pushes only to "cast in stone" branches. If there are other
> >> branches that deserve to be tested with TEST_LONG upon other events
> >> that the existing GitHub Actions CI does not trigger, it may be good
> >> to have GitLab CI cover them, perhaps?
> >
> > I'm a bit hesitant to do such a split, mostly because the canonical
> > source of truth that the project typically uses is GitHub's CI. So I
> > want us at GitLab to be able to catch the same issues that GitHub would
> > flag. And if GitLab's CI stopped detecting everything that GitHub does,
> > then the result would likely be that we often create merge requests on
> > both platforms, which would only result in more wasted resources.
>
> I didn't suggest splitting them into two circles that overlap but
> each with area only it covers, though. GitLab's coverage can be
> superset to GitHub's and that would satify what I suggested.
Fair.
> FWIW, I do not consider GitHub's CI "the canonical source" at all.
> It is a very handy service to use to check how well we are doing,
> but from time to time it has its own hiccups ;-).
Well, GitLab of course has its own share of hiccups, like for example
the Chocolatey issues we've been facing.
> What can we do to make the visibility of GitLab's CI more prominent?
>
> I know where the CI jobs that are triggered when I push out the
> integration branches are found at GitHub's website[*], but I do not
> think I know the corresponding one at GitLab, for example, and I
> think that is a shame.
The pipelines of the official mirror can be found at [1]. We might for
example add something like the below patch to our README.md to make it
more discoverable.
Patrick
[1]: https://gitlab.com/git-scm/git/-/pipelines
diff --git a/README.md b/README.md
index d87bca1b8c..9ad77fdf7e 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,5 @@
-[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
+[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
Git - fast, scalable, distributed revision control system
=========================================================
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-30 9:05 ` Patrick Steinhardt
@ 2026-06-30 19:21 ` Junio C Hamano
2026-07-01 6:56 ` Patrick Steinhardt
0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2026-06-30 19:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Michael Montalbo, git
Patrick Steinhardt <ps@pks.im> writes:
> The pipelines of the official mirror can be found at [1]. We might for
> example add something like the below patch to our README.md to make it
> more discoverable.
>
> Patrick
>
> [1]: https://gitlab.com/git-scm/git/-/pipelines
>
> diff --git a/README.md b/README.md
> index d87bca1b8c..9ad77fdf7e 100644
> --- a/README.md
> +++ b/README.md
> @@ -1,4 +1,5 @@
> -[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> +[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> +[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
>
> Git - fast, scalable, distributed revision control system
> =========================================================
Oh, nice. We of course do not want to be heavily involved in
advertising offerings by commercial entities but I think these two
sites deserve one line each for their continued service to the
community ;-)
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 0/3] fixing expensive http test timeouts
2026-06-30 19:21 ` Junio C Hamano
@ 2026-07-01 6:56 ` Patrick Steinhardt
0 siblings, 0 replies; 40+ messages in thread
From: Patrick Steinhardt @ 2026-07-01 6:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Michael Montalbo, git
On Tue, Jun 30, 2026 at 12:21:30PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The pipelines of the official mirror can be found at [1]. We might for
> > example add something like the below patch to our README.md to make it
> > more discoverable.
> >
> > Patrick
> >
> > [1]: https://gitlab.com/git-scm/git/-/pipelines
> >
> > diff --git a/README.md b/README.md
> > index d87bca1b8c..9ad77fdf7e 100644
> > --- a/README.md
> > +++ b/README.md
> > @@ -1,4 +1,5 @@
> > -[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> > +[](https://github.com/git/git/actions?query=branch%3Amaster+event%3Apush)
> > +[](https://gitlab.com/git-scm/git/-/pipelines?ref=master)
> >
> > Git - fast, scalable, distributed revision control system
> > =========================================================
>
> Oh, nice. We of course do not want to be heavily involved in
> advertising offerings by commercial entities but I think these two
> sites deserve one line each for their continued service to the
> community ;-)
Okay, I'll send a patch then. Thanks!
Patrick
^ permalink raw reply [flat|nested] 40+ messages in thread