connected: add incremental connectivity check

[spkrka]

This series adds an incremental mode for the connectivity
check, gated behind transfer.connectivityCheck=incremental
(no expected changes unless you opt in).

The intent is to solve the problem of the connectivity check
slowing down as the number of reachable objects from the
boundary grows.

It relates to the RFC I sent out earlier:

[RFC] check_connected: toward incoming-proportional cost
https://lore.kernel.org/git/CAL71e4Nf=-zCrfN7ghEVGq11irajJhtdxYZgKe0Ycux0qs1ZvQ@mail.gmail.com/

Design

The verifier runs inside the same rev-list subprocess that
check_connected() already spawns, triggered by a new internal
flag --verify-trees-incremental. After get_revision() collects
the incoming commits, the verifier processes them in topological
order (ancestors before descendants).

The idea is to keep a set of trusted objects, shared across the
incoming commits, that grows over time. We visit the new/untrusted
commit trees and do a comparison walk over the trees of their
parents. Entries discovered on the trusted parent side are
remembered as trusted, which lets later verification skip
matching objects and avoid descending into unchanged subtrees.

More algorithmic details are in
Documentation/technical/connectivity-check.adoc.

Benchmarks

I'll just mention a short summary here, to avoid repeating
what's already in the commit message. The incremental mode is
faster than the full mode when there are few commits to verify
and when the active object tree is large. In the happy case,
the work tracks the changed paths and their comparison trees
rather than the full reachable object closure, which
substantially reduces the dependence on total repository size.
I've seen speedups up to around 20x for the synthetic perf
tests.

Running against a large real-world repo (3.4 GB boundary
closure), the numbers are more dramatic. All timings use
rev-list directly with --not HEADN, isolating the tree
verification cost from the boundary-finding cost:

commits    full  incr.  speedup   full RSS  incr. RSS
      1    1.9s  0.01s    190x      3.4 GB     14 MB
     10    1.9s  0.04s     48x      3.4 GB    125 MB
    100    1.9s  0.37s      5x      3.4 GB    1.1 GB

The full mode takes ~1.9s regardless of commit count because
it is dominated by walking the boundary closure. Incremental
scales with the number of incoming commits and the paths they
touch. Memory follows the same pattern: incremental uses a
fraction of the full mode's RSS for small pushes, converging
only when many commits are verified.

There are also regression cases in the synthetic fixtures.
With long incoming histories, the extra parent-tree scans
accumulate; in the synthetic fixture incremental is about
1.5x slower at 10000 commits. Per-commit changes have less
impact than expected: even when every directory is touched,
incremental remains competitive; bypassing the object cache
for tree reads likely helps here.

I cannot establish how common these regression cases are. In
the cases I have tested, however, the regression has remained
modest; I have not been able to provoke a substantially larger
slowdown. My feeling is that this is an acceptable tradeoff
behind the opt-in config, since the target case (small pushes
to large repos) sees the largest speedup, while the regression
appears with long incoming histories.

A safety net for this regression could be to dynamically disable
the incremental mode if the number of incoming commits is too
large, but this is left out of the initial version to avoid
overly speculative code.

Deepening fetches currently fall back to the full check because
the full check omits --not --all for deepening -- there is no
existing-reference boundary at which the walk can stop. An
incremental approach is possible here too -- using the old
shallow roots as the trusted boundary and walking the deepened ancestry forward -- but that
is a separate change and left for future work. Deepening is
also less common than regular fetch and receive-pack, where the
speedup matters most.

Test coverage

Most correctness cases in t5412-connectivity-check.sh are run
in both full and incremental modes to check semantic equivalence.
Selected cases additionally assert trace2 tree/blob counts for
the incremental mode, to verify that unchanged portions of the
object graph are actually skipped. It covers:

• Corruption detection: missing blobs, missing trees, type
  mismatches, malformed trees (unparseable, mid-tree
  corruption)
• Tree optimization: trace2 assertions confirm unchanged
  subtrees are skipped, subtree moves, merge parent boundaries
• Root commits (no parents -- verifies full tree closure)
• Partial clones: missing promised blobs, missing promised
  trees, verification of local commits
• Replacement objects (with and without GIT_NO_REPLACE_OBJECTS)
• Shallow boundaries
• Deepening fetches (falls back to full check)
• Integration: real push, fetch, and clone

Most tests call git rev-list directly with the appropriate
flags; integration tests exercise the full check_connected()
path through push, fetch, and clone.

Alternatives considered

My first prototype ran the verifier in-process inside
connected.c. This required a second rev-list subprocess just
for boundary finding, _nofetch variants of several
object-reading functions to prevent lazy fetches in partial
clones, explicit shallow-file plumbing, and careful avoidance
of die() in all code paths reachable from the verifier. The
result worked but was fragile and touched many files.

Moving the verifier into the rev-list subprocess eliminated
all of those problems: in partial clones the existing
--exclude-promisor-objects handling already disables lazy
fetching, die() is isolated by the process boundary, shallow
and replacement semantics are established before the verifier
runs, and error routing comes for free via stderr.

So while I liked the idea of being less reliant on checking within
a subprocess, making that work ended up being a lot more complex.

Next steps

This series only addresses tree verification; the other
significant cost is finding the commit boundary, especially for
repos with many refs. I already have some prototypes for
optimizing that too, and if this ends up landing, that would be
something I would start polishing up.

Thanks,
Kristofer

cc: Kristofer Karlsson krka@spotify.com

[spkrka]

/cc Patrick Steinhardt ps@pks.im

[spkrka]

/cc Jeff King <peff@peff.net

[gitgitgadget]

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

[spkrka]

/cc Elijah Newren newren@gmail.com

[gitgitgadget]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[spkrka]

/submit

[gitgitgadget]

Submitted as pull.2211.git.1789379276.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2211/spkrka/tree-diff-connectivity-v1-clean-v1

To fetch this version to local tag pr-2211/spkrka/tree-diff-connectivity-v1-clean-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2211/spkrka/tree-diff-connectivity-v1-clean-v1

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This series adds an incremental mode for the connectivity check, gated
> behind transfer.connectivityCheck=incremental (no expected changes unless
> you opt in).
>
> The intent is to solve the problem of the connectivity check slowing down as
> the number of reachable objects from the boundary grows.

Exciting benchmarks.

[gitgitgadget]

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +static void verify_blob(struct repository *repo,
> +			const struct object_id *oid,
> +			struct verify_state *vs)
> +{
> +	int type;
> +
> +	if (oidset_contains(&vs->trusted_blobs, oid))
> +		return;
> +
> +	vs->blobs_checked++;
> +	type = odb_read_object_info(repo->objects, oid, NULL);
> +	if (type == OBJ_BLOB) {
> +		oidset_insert(&vs->trusted_blobs, oid);
> +		return;
> +	}
> +	if (type >= 0)
> +		die(_("object %s is a %s, not a blob"),
> +		    oid_to_hex(oid), type_name(type));
> +	if (vs->exclude_promisor_objects &&
> +	    is_promisor_object(repo, oid))
> +		return;
> +	die(_("missing blob object '%s'"), oid_to_hex(oid));
> +}

I wonder if this is_promisor_object() call comes a bit too late, as
we earlier already have called odb_read_object_info() which may have
fetched it lazily from the promisor remote?  Or do we globally
disable promisor_remote_get_direct() call somehow without having to
pass OBJECT_INFO_SKIP_FETCH_OBJECT flag?

> +static void verify_commit_tree(struct repository *repo,
> +			       struct commit *commit,
> +			       struct verify_state *vs)
> +{
> +	struct oid_array base_trees = OID_ARRAY_INIT;
> +	struct commit_list *p;
> +
> +	/*
> +	 * Parent trees are trusted: boundary parents are already
> +	 * connected, and earlier incoming parents were verified
> +	 * first due to the topological processing order.
> +	 */
> +	for (p = commit->parents; p; p = p->next) {
> +		const struct object_id *tree_oid;
> +		parse_commit_or_die(p->item);
> +		tree_oid = get_commit_tree_oid(p->item);
> +		tree_map_add(vs->trees, tree_oid, TREE_TRUSTED);
> +		oid_array_append(&base_trees, tree_oid);
> +	}
> +
> +	verify_tree(repo, get_commit_tree_oid(commit),
> +		    &base_trees, vs, 0);
> +	oid_array_clear(&base_trees);
> +}

Do we assume that we do not have to deal with repository corruption
in any graceful way?  I am just wondering what happens when
get_commit_tree_oid() yields NULL after parse_commit_or_die() finds
p->item is a valid-looking commit object but the tree within it is
not, and we end up passing NULL to tree_map_add(), perhaps?

The same potential issue may exist in the get_commit_tree_oid() call
outside the look at the end on the incoming commit's tree.

[gitgitgadget]

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Mon, 14 Sept 2026 at 17:26, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> I wonder if this is_promisor_object() call comes a bit too late, as
> we earlier already have called odb_read_object_info() which may have
> fetched it lazily from the promisor remote?  Or do we globally
> disable promisor_remote_get_direct() call somehow without having to
> pass OBJECT_INFO_SKIP_FETCH_OBJECT flag?

Yes, I think it's safe due to the following mechanism:

1. If promisors exist, the connectivity-check will invoke
   rev-list with --exclude-promisor-objects.
2. rev-list in turn sets repo->fetch_if_missing = 0 on startup.
3. Then the odb read goes down into do_oid_object_info_extended()
   which respects that flag.

However, my paranoia kicked in so I re-ran my test for this,
after adding some temporary code inside
verify_commits_incremental():

    repo->fetch_if_missing = 1;

And fortunately, one of the tests failed as expected.

    Exactly 1 failure out of 62 tests: test 53
      "incremental: verifies new subtree when parent subtree is
       promised".

And the relevant assertion is this one:

    test_must_fail env GIT_NO_LAZY_FETCH=1 \
        git cat-file -e "$parent_subtree"

which ensures that the object was never fetched.

However, the test only catches this scenario for trees,
not blobs -- that's an oversight, I will add a matching
test for blobs too.

I think the code technically works as-is, but I could also try
to rewrite the code to stop depending on odb_read_object_info()
and instead use odb_read_object_info_extended() which allows
me to pass the flags.  That gives us belts and suspenders, which
may be nicer here.

> Do we assume that we do not have to deal with repository corruption
> in any graceful way?  I am just wondering what happens when
> get_commit_tree_oid() yields NULL after parse_commit_or_die() finds
> p->item is a valid-looking commit object but the tree within it is
> not, and we end up passing NULL to tree_map_add(), perhaps?
>
> The same potential issue may exist in the get_commit_tree_oid() call
> outside the look at the end on the incoming commit's tree.

You're right, this is an oversight.
I think I incorrectly assumed that parse_commit_or_die()
would catch any malformed commit.

I will add a NULL check and a die()-exit at the two call sites
in verify_commit_tree()

    die(_("unable to load root tree for commit %s"),
        oid_to_hex(&commit->object.oid));

Thanks for spotting these errors,
Kristofer

[gitgitgadget]

User Kristofer Karlsson <krka@spotify.com> has been added to the cc: list.