Repository navigation
Conversation
Since 722ff7f (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), receive-pack writes incoming objects to a temporary object directory. Child processes see it as their primary object directory, and tmp-objdir passes the repository's own object directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES. The connectivity check passes --alternate-refs to rev-list since 39b44ba (check_everything_connected: assume alternate ref tips are valid, 2019-07-01). Inside the quarantine, the repository's own object directory is one of the alternates, so --alternate-refs runs for-each-ref on the repository itself and lists all of its refs. That includes hidden refs. bcec678 (receive-pack: only use visible refs for connectivity check, 2022-11-17) added --exclude-hidden=receive to the check so that repositories with many hidden refs do not have to load all of them on each push. The hidden refs are excluded from --all, but they all come back through --alternate-refs. Skip an alternate whose repository is our own when listing alternate refs. This is done by comparing paths, which is a hack: it is mainly here to measure the cost of the bug, and a proper fix would have the quarantine tell its children which directory is ours. It is safe, though: alternate refs only serve as negative tips that are assumed to be complete, so dropping some can only make the traversal walk further, never make it accept a missing object. Add a test that our own refs are no longer listed, a test that the refs of a real alternate still are, and a perf test pushing a single commit into a repository with a visible branch and about 200,000 refs hidden by receive.hideRefs, and into a copy of it that hides nothing: Test HEAD~1 HEAD ----------------------------------------------------------------------- 5552.2: push, hidden refs 0.31(0.32+0.08) 0.10(0.06+0.03) -67.7% 5552.3: push, no hidden refs 1.25(1.08+0.68) 1.03(0.84+0.62) -17.6% With hidden refs, most of the time went to the for-each-ref child that listed the alternate refs. Without hidden refs, every ref was listed twice, once for --all and once for --alternate-refs. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Author
|
/submit |
|
Submitted as pull.2285.git.1793200705017.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is mainly a bug report, in patch form so that it comes with a
test and numbers. I don't expect it to be merged, at least not in
this form.
When a repository hides refs with receive.hideRefs, every push still
loads all of the hidden refs in the connectivity check, even though
bcec678 (receive-pack: only use visible refs for connectivity check,
2022-11-17) excludes them from it. With many hidden refs, that is most
of the time spent on a small push.
I stumbled across this while working on the boundary walk of the
connectivity check (the follow-up to kk/connectivity-check-incremental),
when the walk loaded every ref during a push even though it should
only have looked at a few of them.
The cause is that receive-pack runs the check inside the object
quarantine, and there the repository's own object directory is passed
to rev-list as an alternate. --alternate-refs then lists all of the
repository's own refs, hidden ones included. Without hidden refs, every
ref is listed twice instead. Setting core.alternateRefsPrefixes to the
visible namespaces works around it, though that also limits which refs
of real alternates are used.
The patch includes a small fix by comparing paths, mostly so that the
perf test can show the difference (about 0.31s -> 0.10s for a push
with 200,000 hidden refs). I am not really happy with it as a proper
fix though.
I also tried resolving the object directories that --alternate-refs
uses before the quarantine is created. That got a bit messy: it needed
a new internal rev-list option, and receive-pack had to carry the list
of directories through to each of its connectivity checks. And it only
fixes the check itself; other processes running inside the quarantine,
like pre-receive hooks, would still see the repository as its own
alternate.
So the question is really where this should be fixed: is there a better
way for processes running inside the quarantine to tell the
repository's own object directory apart from real alternates?
I am likely not the best person to answer that. Patrick, maybe you
could take a look, since you have recently reworked tmp-objdir and the
odb sources?
cc: Patrick Steinhardt ps@pks.im
cc: Jeff King peff@peff.net