connected: add incremental connectivity check - #2211
Open
spkrka wants to merge 5 commits into
Open
Conversation
spkrka
marked this pull request as ready for review
August 28, 2026 10:15
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
10 times, most recently
from
September 1, 2026 18:03
326b1e4 to
452d6b9
Compare
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
2 times, most recently
from
September 10, 2026 09:06
cf5a54d to
a361b72
Compare
When parse_loose_header() succeeds but the type string is not a known object type, read_object_info_from_path() calls die(). This is inconsistent with the other error paths in the same function, which call error() and return -1, letting the caller decide how to handle the failure. Replace the die() with the same error()-and-return pattern used by the surrounding code paths (ULHR_BAD, ULHR_TOO_LONG, and parse_loose_header failure). This changes observable behavior in a few places: - cat-file -s/-t/-p now prints the "error: invalid object type" diagnostic before the caller's own fatal message, matching the double-message pattern already produced by the too-long-header path. - cat-file --batch and --batch-check now report the object as "missing" on stdout (with the diagnostic on stderr) instead of dying, which is consistent with how other forms of corruption are handled in batch mode. - rev-parse disambiguation now shows candidates as "[bad object]" with full disambiguation hints instead of dying. This matches the behavior for other kinds of corrupt objects (e.g. zlib-damaged blobs). This also allows in-process callers such as the incremental connectivity verifier (introduced in a subsequent commit) to treat this corruption as a recoverable read failure instead of terminating the process. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Move the inline self-contained pack detection into a helper function. This makes check_connected() easier to follow and makes the detection logic reusable by the incremental path. No functional change. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
The connectivity check uses rev-list to find commits reachable
from the incoming tips but not from any local ref and then walks
their object closure. Commit traversal stops at the connectivity
boundary, but trees and blobs reachable from that boundary still
need to be walked so they can be marked uninteresting, allocating
a struct object for each one. On repositories where the boundary
commits have large trees, the connectivity check for small
incoming changes visits and tracks more objects than needed.
Add an alternative connectivity check that verifies incoming
commits incrementally against their parents.
Instead of traversing the full boundary closure, the new check
compares each new commit's tree with its parent trees. Already
trusted entries are skipped, changed subtrees are descended into
recursively, and blobs are checked for existence. This approach
thus avoids descending into untouched subtrees.
For example, consider a commit that changes one file under lib/
and also moves an unchanged subtree from src/ to dev/:
Parent tree New tree
+-- src/ (aaa) +-- dev/ (aaa)
+-- lib/ (bbb) +-- lib/ (ccc)
+-- foo.c (ddd) +-- foo.c (ddd)
+-- bar.c (eee) +-- bar.c (fff)
The verifier first scans the new root and collects aaa and ccc as
work items. It then scans the parent root, publishing aaa and bbb
into the trusted sets and recording bbb as the comparison base for
ccc.
When the work list is revisited, aaa is now trusted and skipped
even though it appears at a different path. The verifier descends
into ccc using bbb as its parent base. Scanning bbb similarly
makes ddd and eee trusted, leaving only the new fff blob to be
checked for existence.
Thus neither the moved subtree nor any other unchanged subtree is
recursively explored; only the changed lib/ subtree is descended
into, and only the new bar.c blob needs an existence check. The
root trees still need to be read and scanned as comparison bases.
New commits are processed with ancestors before descendants.
Parents outside the incoming commit set are on the already-connected
side of the boundary and provide the initial trusted bases. Once
an incoming commit has been verified, its tree can in turn be used
as a trusted base for descendant commits.
The verifier distinguishes trusted trees from expanded trees. A
trusted tree can be accepted without further verification. An
expanded tree has additionally published its direct non-gitlink
entries into the trusted sets. Expanded parent trees therefore
need not be read again for blob-only work, but may still be reread
when recursive verification needs same-path parent subtrees.
The algorithm has three stages:
1. Collect and peel tips. Non-commit tips are verified
immediately; tips already covered by a self-contained pack
verified by index-pack are skipped.
2. Find the set of new commits by delegating to
rev-list --stdin --not --all.
3. Process those commits in topological order, verifying each
one against its parents via recursive tree comparison.
Use non-dying object reads and parsers so that malformed incoming
objects are reported as connectivity failures rather than terminating
the caller. Two known limitations remain:
- Some lower-level object parsers (notably tree_entry_gently())
call error() internally on malformed data, so those diagnostics
can bypass the quiet and err_fd routing used by the rest of the
verifier. Fixing this would require broader API changes.
- The packfile layer can die() on certain corrupt packed objects
(e.g. an invalid delta type). These paths are only reachable
from pre-existing local corruption, not from incoming packs
(which are validated by index-pack before storage). The
rev-list code path is naturally protected because rev-list
runs as a subprocess; the incremental verifier runs in-process.
Gate the new algorithm behind transfer.connectivityCheck=incremental,
keeping rev-list as the default. Also fall back to the existing
rev-list path for shallow boundaries, partial clones,
replacement objects, and deepening fetches.
p5412 results (median of 3). Each commit modifies one file in
one directory, cycling through all directories.
Speedup scenarios (few commits, large tree):
files commits rev-list incr. speedup
(a) 50K, 1 0.05s 0.02s 2.5x
(b) 100K, 1 0.07s 0.02s 3.5x
(c) 200K, 1 0.14s 0.02s 7.0x
(d) 400K, 1 0.31s 0.02s 15.5x
(e) 800K, 1 0.58s 0.03s 19.3x
(f) 200K, 100 0.15s 0.06s 2.5x
Rows (a)-(e) show that scaling unrelated subtrees affects
rev-list much more strongly. Incremental still scans the root
trees, but does not descend into unchanged subtrees.
Breakeven (500 commits at 200K files):
(g) 200K, 500 0.24s 0.22s 1.1x
Regression (many commits in a linear chain):
files commits rev-list incr. regression
(h) 200K, 1000 0.35s 0.43s 1.2x
(i) 200K, 2000 0.52s 0.87s 1.7x
(j) 200K, 3000 0.68s 1.16s 1.7x
In this benchmark the regression reaches about 1.7x at
2000-3000 commits. Incremental rereads parent trees that were
previously read as child trees when they later serve as
comparison bases. For repeated changes along deep paths in a
linear history, the number of tree loads can approach twice that
of rev-list. This is a tree-read cost model rather than a bound
on wall-clock runtime.
Small fetches and pushes avoid most of the existing closure
traversal and show the largest speedups.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Remove the shallow_file guard from incremental_check_applicable() so the incremental algorithm can be used with shallow boundaries when configured. Add shallow commit handling to the incremental verifier: oidset_parse_file() reads the temporary shallow file and the resulting set is threaded through verify_new_commits and verify_commit_tree so that shallow commits are treated as roots. The boundary-finding rev-list receives --shallow-file so it respects shallow grafts. Add a test verifying that missing blobs behind the shallow boundary are detected. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Remove the repo_has_promisor_remote() guard from incremental_check_applicable() so the incremental algorithm is used for partial clones when configured. Add promisor object handling to the incremental verifier: when a tree or blob cannot be read, is_promisor_object() is checked before reporting an error. Missing promisor objects encountered while peeling tips are accepted without fetching them. The boundary-finding rev-list receives --exclude-promisor-objects so promisor commits do not enter the verification set. Add tests verifying that missing promised blobs and trees are accepted without triggering lazy fetches, and that locally created commits in a partial clone are verified correctly. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
from
September 10, 2026 09:10
a361b72 to
f4a0fab
Compare
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
1 participant
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 series adds an incremental connectivity check to
check_connected(), gated behind transfer.connectivityCheck=incremental
(so no expected changes unless you opt-in to it).
It relates to the RFC I sent out earlier:
The current connectivity check delegates to rev-list, which walks
the full object closure at the connectivity boundary. On a large
private repository with ~600K reachable trees and blobs, fetching
a single new commit takes ~1.8 seconds -- almost all of it spent
walking objects unrelated to the incoming change.
I think this is worth optimizing, since the connectivity check
is on the critical path in normal fetch and receive-pack flows.
The core idea here is to compare each incoming commit's tree
against its parent trees, descending only into entries that
differ. This shifts much of the tree-verification cost toward
changed paths rather than the full reachable closure. On the
same repository, the same fetch drops to ~140 ms (12.9x faster).
On linux.git it drops from 226 ms to 97 ms (2.3x).
Synthetic benchmarks (p5412) show 2.5x-19.3x speedups for
single-commit checks across tree sizes from 50K to 800K
files, with breakeven around 500 commits in the 200K-file
fixture and an observed regression of up to ~1.7x in the long
linear-chain cases tested. The tradeoff comes from rereading
parent trees as comparison bases. (Patch 2 contains the full
table and analysis.)
The series:
connected: extract get_self_contained_pack() helper
Pure refactor for reuse by the incremental path.
connected: add incremental connectivity check
Core implementation, tests, and performance tests. Falls back
to rev-list for shallow boundaries, partial clones, replacement
objects, and deepening fetches.
connected: enable incremental check with shallow boundaries
Adds shallow boundary handling and removes the shallow guard.
connected: enable incremental check for partial clones
Adds promisor object handling and removes the promisor guard.
Missing promisor objects are accepted without triggering lazy
fetches.
The series includes a test-tool helper (test-check-connected) that
exercises check_connected() directly, making it easier to test
specific object graph shapes and failure modes without going through
the full fetch/push machinery. The functional tests cover happy
paths, corrupt objects, missing objects, shallow boundaries, and
partial clones. There is also a perf test (p5412), though it may
be too specific for this series -- happy to drop it if a more
general regression test would be preferred.
Open questions
Some questions surfaced while building this:
Type confusion: the existing rev-list connectivity check does not
always detect a tree entry whose mode disagrees with the actual
object type. The incremental verifier retains this same
behavior -- it checks existence but not type correctness for
blobs. Should we tighten type checking in a follow-up covering
both the rev-list and incremental paths, or is the current
behavior sufficient as-is?
Replacement objects: the existing rev-list path follows replacement
refs, while git-prune explicitly disables them. The incremental
path falls back to rev-list when replacement objects are active.
Should connectivity checking operate on the underlying object graph
rather than the replaced one? Even the rev-list behavior here may
be worth reconsidering.
The tree verifier introduces private _nofetch variants of tree
reading and tag peeling to avoid triggering lazy promisor fetches
during verification. This felt a bit awkward and introduces some
code duplication, but trying to make the existing functions support
nofetch started to become a can of worms so this at least keeps
the change smaller than it otherwise would have been. Is this
something that should be refactored first or is keeping the
duplication acceptable?
I am not sure how to reason about the regression in some edge-cases.
Perhaps it's not a problem since it's gated behind a config flag,
but that might mean the flag needs to remain permanently?
Alternatively, it would be possible to build some type of heuristic
to decide which algorithm to use based on how many commits need to
be verified, or how many files exist in the repository.
Or is a constant factor regression in some unfavorable cases an
acceptable price to pay for helping the happy cases?
Next steps
I don't want to digress too much here, but I think it's
useful to view this as one part of making connectivity-check
cost less dependent on total repository size. This series
addresses tree verification; the other substantial part is
finding the commit boundary.
In the same single-commit case on my large private repository, a
separate prototype using an in-process boundary walk reduces the
total incremental connectivity check from ~140 ms to ~14 ms.
That work is not included here; boundary discovery has its own
tradeoffs and seems better reviewed separately.
Thanks,
Kristofer