connected: add incremental connectivity check

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

Kristofer Karlsson (2):
Documentation: describe connectivity checking
connected: add incremental connectivity check via rev-list

 Documentation/config/transfer.adoc            |  20 +
 Documentation/rev-list-options.adoc           |   6 +
 .../technical/connectivity-check.adoc         | 243 +++++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  18 +
 connected.c                                   |  24 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5412-connectivity-check.perl |  44 ++
 t/perf/p5412-connectivity-check.sh            |  92 +++
 t/t5412-connectivity-check.sh                 | 655 ++++++++++++++++++
 tree-verify.c                                 | 306 ++++++++
 tree-verify.h                                 |  15 +
 13 files changed, 1426 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc
 create mode 100644 t/perf/generate-repo-p5412-connectivity-check.perl
 create mode 100755 t/perf/p5412-connectivity-check.sh
 create mode 100755 t/t5412-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

base-commit: 47ce80527c56f462cb97db4ca8125342204d3783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2211%2Fspkrka%2Ftree-diff-connectivity-v1-clean-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2211/spkrka/tree-diff-connectivity-v1-clean-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2211

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check.

---
 .../technical/connectivity-check.adoc         | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc

new file mode 100644
index 0000000000..d20bff6af6
@@ -0,0 +1,109 @@
+Connectivity checking
+=====================
+
+After receiving new objects via fetch, push (receive-pack), clone,
+or bundle, Git verifies that the new reference tips do not leave
+the repository in a state where reachable objects are missing.
+This verification is called the connectivity check.
+
+Connectivity invariant
+----------------------
+
+A repository is connected when every object reachable from its
+references is available locally (with exceptions noted below).
+
+The connectivity check maintains this invariant when references
+are updated.  It trusts the existing connected state and verifies
+that the new reference tips do not introduce references to
+unavailable objects.  Verification is permitted to stop when it
+reaches objects already reachable from trusted existing
+references, since their closure is already connected.  These
+trusted references include local references and references from
+alternate object stores.
+
+Without this check, a truncated or corrupted transfer could leave
+a repository in a state where later history walks encounter
+missing objects.
+
+Exceptions
+~~~~~~~~~~
+
+Gitlink entries (submodule references) are excluded from
+connectivity checking.  Their target objects belong to a separate
+repository.
+
+In partial clones, objects promised by a promisor remote are
+accepted as connected without requiring local existence.  The
+check excludes promisor objects from traversal so that it does
+not trigger on-demand fetches for them.
+
+Full connectivity check
+-----------------------
+
+`check_connected()` (see `connected.c`) normally performs the
+connectivity check using a `rev-list` subprocess, feeding the
+new reference tips via stdin.  A normal invocation is roughly:
+
+    git rev-list --objects --stdin --not --all --quiet
+        --alternate-refs [--exclude-promisor-objects]
+
+When promisor remotes are configured, `check_connected()` first
+attempts a fast path based on promisor packfiles.  If it falls
+back to the `rev-list` check, `--exclude-promisor-objects` is
+added so that the traversal does not trigger on-demand fetches.
+
+Consider the following graph after a fetch, where all reference
+tips point directly to commits.  For simplicity, only local
+references appear on the already-connected side; alternate refs
+play the same role.  N3 is a merge commit:
+
+            /-------------L2
+           /
+    C1---B1---C2---B2-----L1
+          \         \
+           N1        N3---T2
+            \       /
+             N2-----------T1
+
+    L1, L2:         local refs
+    T1, T2:         incoming tips (new refs)
+    N1, N2, N3:     incoming commits (N3 is a merge)
+    B1, B2:         boundary commits (already connected)
+    C1, C2:         already connected (but not boundary)
+
+The incoming set is the commits reachable from the incoming
+tips but not from the already-connected side.  Boundary commits
+are the already-connected commits at the edge of that set.  Here
+B1 is an ancestor of B2, which happens when incoming branches
+fork at different depths in the existing history.
+
+The check proceeds in three phases:
+
+1. Walk from the incoming tips (T1, T2) against the trusted
+   refs (L1, L2) to find the incoming set ({N1, N2, N3, T1, T2}).
+
+2. Walk the trees of the boundary commits (B1, B2) and mark
+   those objects uninteresting.  These trees are already trusted
+   because their commits are on the already-connected side.
+
+3. Walk the trees of each incoming commit and verify that every
+   referenced object is connected, stopping at objects already
+   marked uninteresting in phase 2.
+
+Deepening fetches
+~~~~~~~~~~~~~~~~~
+
+For deepening fetches (where the shallow boundary moves), the
+full check omits `--not --all`.  There is no existing-reference
+boundary at which the walk can stop.  Instead, traversal follows
+the effective shallow boundary supplied for the deepened
+repository.  The new content may be below the old shallow
+boundary even when the tips themselves have not changed.
+
+Non-commit tips
+~~~~~~~~~~~~~~~
+
+When a new reference points to a non-commit object, such as a
+tag, tree, or blob, that object is not part of the commit walk.
+These non-commit tips are handled by the subsequent object
+traversal.

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The full connectivity check uses rev-list to find commits reachable from the incoming tips but not from the already-connected side, 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.

The verifier processes incoming commits with ancestors first and compares each new tree against its trusted parent trees. Entries already seen on the trusted side are skipped by OID, so unchanged subtrees need not remain at the same path to be recognized. Changed subtrees are recursively compared against same-path parent subtrees, and new subtrees without a comparison base are verified from scratch.

See Documentation/technical/connectivity-check.adoc for the trust invariants and detailed algorithm.

The incremental check runs as an internal --verify-trees-incremental mode in rev-list. After the normal revision walk identifies the incoming commits, the verifier consumes them before the usual object traversal; any pending non-commit tips are still handled by the existing traversal.

Partial-clone semantics are preserved: objects promised by a promisor remote are accepted as connected, and on-demand fetching is prevented by excluding promisor objects from traversal. The new mode is selected by transfer.connectivityCheck=incremental, with full remaining the default. Deepening fetches fall back to the full check because they do not have the normal existing-reference boundary, and traversal instead follows the effective shallow boundary.

p5412 results (median of 3), scaling one dimension at a time. Each fixture is a repository with a flat tree of many directories containing 100 files each. The incoming set is a chain of commits on top of the existing history, with each commit changing files in different directories round-robin. The three axes vary the total repository tree size, the number of incoming commits, and the number of files changed per incoming commit.

Scaling tree size (10 incoming commits, 10 files/commit):

    files    full  incr.  full/incr
      5K    0.01s  0.01s    1.0x
     50K    0.04s  0.01s    4.0x
    200K    0.15s  0.01s   15.0x
    800K    0.61s  0.03s   20.3x

The full mode must walk the trees of boundary commits, which grows with overall tree size. Incremental still scans the root trees, but avoids descending into unchanged subtrees, so it grows much more slowly with repository size.

Scaling incoming commit count (200K files, 10 files/commit):

    commits    full  incr.  full/incr
          1    0.14s  0.01s   14.0x
         10    0.15s  0.01s   15.0x
        100    0.19s  0.05s    3.8x
        500    0.32s  0.27s    1.2x
       3000    1.23s  1.52s    0.8x
       5000    1.88s  2.43s    0.8x
      10000    3.72s  5.36s    0.7x

With many commits the per-commit overhead of scanning both the new and parent root trees accumulates and incremental becomes slower. Breakeven is between 500 and 3000 commits and the ratio stabilizes near 0.7x for this fixture.

Scaling files per incoming commit (200K files, 10 commits):

    files/commit    full  incr.  full/incr
               1    0.14s  0.01s   14.0x
              10    0.15s  0.01s   15.0x
             100    0.16s  0.04s    4.0x
             500    0.23s  0.14s    1.6x
            1000    0.34s  0.28s    1.2x
            2000    0.70s  0.63s    1.1x

Breakeven is around 1000 files/commit. At 2000 files/commit (every directory touched), incremental remains slightly faster; bypassing the object cache for tree reads likely helps here.

For small repositories both modes are fast enough that the difference is difficult to measure reliably.

Selected peak RSS measurements from the same fixtures:

    case                        full    incr.  ratio
    200K files, 10 commits      31 MB    12 MB   0.4x
    800K files, 10 commits     110 MB    26 MB   0.2x
    200K files, 5000 commits   155 MB   151 MB   1.0x

Incremental uses substantially less memory when it can prune most of the boundary tree walk. In the long-history case the two modes visit similar object sets and memory converges.

The regression cases in the CPU benchmarks are in wall-clock time rather than memory, primarily from scanning some trees more than once.

---
 Documentation/config/transfer.adoc            |  20 +
 Documentation/rev-list-options.adoc           |   6 +
 .../technical/connectivity-check.adoc         | 134 ++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  18 +
 connected.c                                   |  24 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5412-connectivity-check.perl |  44 ++
 t/perf/p5412-connectivity-check.sh            |  92 +++
 t/t5412-connectivity-check.sh                 | 655 ++++++++++++++++++
 tree-verify.c                                 | 306 ++++++++
 tree-verify.h                                 |  15 +
 13 files changed, 1317 insertions(+)
 create mode 100644 t/perf/generate-repo-p5412-connectivity-check.perl
 create mode 100755 t/perf/p5412-connectivity-check.sh
 create mode 100755 t/t5412-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

index f1ce50f4a6..b9939d3bde 100644
@@ -1,3 +1,23 @@
+transfer.connectivityCheck::
+	Choose which algorithm to use for the connectivity check
+	performed during object transfer operations such as
+	linkgit:git-fetch[1] and linkgit:git-receive-pack[1].
+	The connectivity check verifies that all objects reachable
+	from the incoming tips are available locally or, in a partial
+	clone, promised by a promisor remote.
+	The variants are as follows:
++
+--
+`full` (default);;
+	Walk the full object closure of the boundary commits.
+`incremental`;;
+	Verify incoming commits by diffing their trees against parent
+	trees, recursively descending only into entries that differ.
+	The largest benefits occur when incoming commits change a
+	small fraction of a large tree closure.
+	Falls back to `full` for deepening fetches.
+--
+
 transfer.credentialsInUrl::
 	A configured URL can contain plaintext credentials in the form
 	`<protocol>://<user>:<password>@<domain>/<path>`. You may want

index fd831f0ec6..7964b691c4 100644
@@ -1089,6 +1089,12 @@ we cannot get their Object ID though, an error will be raised.
 	stronger than `--missing=allow-promisor` because it limits the
 	traversal, rather than just silencing errors about missing
 	objects.
+
+`--verify-trees-incremental`::
+	(For internal use only.)  Verify tree connectivity
+	incrementally by comparing each commit's tree against its
+	parent trees.  Used by `check_connected()` when
+	`transfer.connectivityCheck` is set to `incremental`.
 endif::git-rev-list[]
 
 `--no-walk[=(sorted|unsorted)]`::

index d4b775953d..aa5b4e2b84 100644
@@ -1357,6 +1357,7 @@ LIB_OBJS += trailer.o
 LIB_OBJS += transport-helper.o
 LIB_OBJS += transport.o
 LIB_OBJS += tree-diff.o
+LIB_OBJS += tree-verify.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += tree.o
 LIB_OBJS += unpack-trees.o

index 6b596231ab..7741fdec9f 100644
@@ -28,6 +28,7 @@
 #include "commit-reach.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "tree-verify.h"
 
 struct rev_list_info {
 	struct rev_info *revs;

@@ -706,6 +707,7 @@ int cmd_rev_list(int argc,
 	int bisect_find_all = 0;
 	int use_bitmap_index = 0;
 	int filter_provided_objects = 0;
+	int verify_trees_incremental = 0;
 	const char *show_progress = NULL;
 	int ret = 0;
 

@@ -748,6 +750,8 @@ int cmd_rev_list(int argc,
 		if (!strcmp(arg, "--exclude-promisor-objects")) {
 			repo->fetch_if_missing = 0;
 			revs.exclude_promisor_objects = 1;
+		} else if (!strcmp(arg, "--verify-trees-incremental")) {
+			verify_trees_incremental = 1;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
 		} else if (!strcmp(arg, "-z")) {

@@ -822,6 +826,8 @@ int cmd_rev_list(int argc,
 
 		if (!strcmp(arg, "--exclude-promisor-objects"))
 			continue; /* already handled above */
+		if (!strcmp(arg, "--verify-trees-incremental"))
+			continue; /* already handled above */
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 

@@ -935,6 +941,18 @@ int cmd_rev_list(int argc,
 
 	prepare_maximal_independent(&revs);
 
+	if (verify_trees_incremental) {
+		struct commit *commit;
+		struct commit_list *new_commits = NULL;
+
+		while ((commit = get_revision(&revs)) != NULL)
+			commit_list_insert(commit, &new_commits);
+
+		verify_commits_incremental(repo, &new_commits,
+					   revs.exclude_promisor_objects);
+		commit_list_free(new_commits);
+	}
+
 	if (revs.tree_objects)
 		mark_edges_uninteresting(&revs, show_edge, 0);
 

index 929b9bd28d..e3dcd8e2b0 100644
@@ -1,6 +1,7 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "git-compat-util.h"
+#include "config.h"
 #include "gettext.h"
 #include "hex.h"
 #include "odb.h"

@@ -67,6 +68,26 @@ static int check_connected_promisor(oid_iterate_fn fn,
 	return 1;
 }
 
+static int incremental_check_applicable(struct check_connected_options *opt)
+{
+	const char *algorithm = NULL;
+
+	if (repo_config_get_string_tmp(the_repository,
+				       "transfer.connectivitycheck",
+				       &algorithm))
+		return 0;
+	if (!strcasecmp(algorithm, "full"))
+		return 0;
+	if (strcasecmp(algorithm, "incremental"))
+		die(_("unknown transfer.connectivityCheck algorithm '%s'"),
+		    algorithm);
+
+	if (opt->is_deepening_fetch)
+		return 0;
+
+	return 1;
+}
+
 /*
  * If we feed all the commits we want to verify to this command
  *

@@ -133,6 +154,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	if (opt->progress)
 		strvec_pushf(&rev_list.args, "--progress=%s",
 			     _("Checking connectivity"));
+	if (incremental_check_applicable(opt))
+		strvec_push(&rev_list.args,
+			    "--verify-trees-incremental");
 
 	rev_list.git_cmd = 1;
 	if (opt->env)

index 0a95d90d21..d94dedc26c 100644
@@ -562,6 +562,7 @@ libgit_sources = [
   'transport-helper.c',
   'transport.c',
   'tree-diff.c',
+  'tree-verify.c',
   'tree-walk.c',
   'tree.c',
   'unpack-trees.c',

index 7f53cca7d1..19a8246c44 100644
@@ -652,6 +652,7 @@ integration_tests = [
   't5409-colorize-remote-messages.sh',
   't5410-receive-pack.sh',
   't5411-proc-receive-hook.sh',
+  't5412-connectivity-check.sh',
   't5500-fetch-pack.sh',
   't5501-fetch-push-alternates.sh',
   't5502-quickfetch.sh',

new file mode 100644
index 0000000000..6aadadff70
@@ -0,0 +1,15 @@
+#ifndef TREE_VERIFY_H
+#define TREE_VERIFY_H
+
+struct commit_list;
+struct repository;
+
+/*
+ * Verify trees of commits incrementally against their parents.
+ * Dies on verification failure.
+ */
+void verify_commits_incremental(struct repository *repo,
+				struct commit_list **commits,
+				int exclude_promisor_objects);
+
+#endif /* TREE_VERIFY_H */

gitgitgadget

"Kristofer Karlsson via GitGitGadget" <[email protected]>

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.

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

"Kristofer Karlsson via GitGitGadget" <[email protected]>

Exciting benchmarks.

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.

Changes since v1
================

* Guard against get_commit_tree_oid() returning NULL in verify_commit_tree(), dying with a clear message instead of a NULL dereference. (Thanks Junio for catching this)
* Set fetch_if_missing = 0 in the rev-list early option parse when --verify-trees-incremental is seen, and add a BUG() assertion in verify_commits_incremental() to catch any future caller that forgets. This makes the lazy-fetch protection explicit rather than relying solely on --exclude-promisor-objects having set it earlier. (Thanks again to Junio)
* Add a test that verifies promised blobs are not lazy-fetched during the incremental check.
* Reuse the existing 3-argument type-mismatch format string to avoid adding new strings for localization.

Thanks, Kristofer

Kristofer Karlsson (2):
Documentation: describe connectivity checking
connected: add incremental connectivity check via rev-list

 Documentation/config/transfer.adoc            |  20 +
 Documentation/rev-list-options.adoc           |   6 +
 .../technical/connectivity-check.adoc         | 243 +++++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  19 +
 connected.c                                   |  24 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5412-connectivity-check.perl |  44 ++
 t/perf/p5412-connectivity-check.sh            |  92 +++
 t/t5412-connectivity-check.sh                 | 680 ++++++++++++++++++
 tree-verify.c                                 | 316 ++++++++
 tree-verify.h                                 |  15 +
 13 files changed, 1462 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc
 create mode 100644 t/perf/generate-repo-p5412-connectivity-check.perl
 create mode 100755 t/perf/p5412-connectivity-check.sh
 create mode 100755 t/t5412-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

base-commit: 47ce80527c56f462cb97db4ca8125342204d3783
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2211%2Fspkrka%2Ftree-diff-connectivity-v1-clean-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2211/spkrka/tree-diff-connectivity-v1-clean-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2211

Range-diff vs v1:

 1:  e55c5452db = 1:  97c11449ae Documentation: describe connectivity checking
 2:  ebe6c90cc5 ! 2:  6ad528f4bb connected: add incremental connectivity check via rev-list
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       			revs.exclude_promisor_objects = 1;
      +		} else if (!strcmp(arg, "--verify-trees-incremental")) {
      +			verify_trees_incremental = 1;
     ++			repo->fetch_if_missing = 0;
       		} else if (skip_prefix(arg, "--missing=", &arg)) {
       			parse_missing_action_value(repo, arg);
       		} else if (!strcmp(arg, "-z")) {
     @@ t/t5412-connectivity-check.sh (new)
      +	)
      +'
      +
     ++test_expect_success "$mode: does not lazy-fetch promised blob" '
     ++	test_when_finished "rm -rf nofetch-src nofetch-server.git nofetch-client" &&
     ++	git init nofetch-src &&
     ++	test_commit -C nofetch-src --no-tag base file.txt content &&
     ++	git clone --bare nofetch-src nofetch-server.git &&
     ++	git -C nofetch-server.git config uploadpack.allowfilter true &&
     ++	git -C nofetch-server.git config uploadpack.allowanysha1inwant true &&
     ++	git clone --no-checkout --filter=blob:none \
     ++		"file://$(pwd)/nofetch-server.git" nofetch-client &&
     ++	(
     ++		cd nofetch-client &&
     ++		promised_blob=$(git rev-parse HEAD:file.txt) &&
     ++		test_must_fail env GIT_NO_LAZY_FETCH=1 \
     ++			git cat-file -e "$promised_blob" &&
     ++		new_tree=$(printf "100644 blob %s\tfile.txt\n" \
     ++			"$promised_blob" |
     ++			git mktree --missing) &&
     ++		new_commit=$(git commit-tree "$new_tree" \
     ++			-m "root commit with promised blob") &&
     ++		check_connected "$new_commit" &&
     ++		test_must_fail env GIT_NO_LAZY_FETCH=1 \
     ++			git cat-file -e "$promised_blob"
     ++	)
     ++'
     ++
      +test_expect_success "$mode: accepts missing promised tree" '
      +	test_when_finished "rm -rf prom-tree-src prom-tree-server.git prom-tree-client" &&
      +	git init prom-tree-src &&
     @@ tree-verify.c (new)
      +		return;
      +	}
      +	if (type >= 0)
     -+		die(_("object %s is a %s, not a blob"),
     -+		    oid_to_hex(oid), type_name(type));
     ++		die(_("object %s is a %s, not a %s"),
     ++		    oid_to_hex(oid), type_name(type), "blob");
      +	if (vs->exclude_promisor_objects &&
      +	    is_promisor_object(repo, oid))
      +		return;
     @@ tree-verify.c (new)
      +
      +	if (buf && type != OBJ_TREE) {
      +		free(buf);
     -+		die(_("object %s is a %s, not a tree"),
     -+		    oid_to_hex(oid), type_name(type));
     ++		die(_("object %s is a %s, not a %s"),
     ++		    oid_to_hex(oid), type_name(type), "tree");
      +	}
      +	return buf;
      +}
     @@ tree-verify.c (new)
      +		const struct object_id *tree_oid;
      +		parse_commit_or_die(p->item);
      +		tree_oid = get_commit_tree_oid(p->item);
     ++		if (!tree_oid)
     ++			die(_("unable to load root tree for commit %s"),
     ++			    oid_to_hex(&p->item->object.oid));
      +		tree_map_add(vs->trees, tree_oid, TREE_TRUSTED);
      +		oid_array_append(&base_trees, tree_oid);
      +	}
      +
     ++	if (!get_commit_tree_oid(commit))
     ++		die(_("unable to load root tree for commit %s"),
     ++		    oid_to_hex(&commit->object.oid));
      +	verify_tree(repo, get_commit_tree_oid(commit),
      +		    &base_trees, vs, 0);
      +	oid_array_clear(&base_trees);
     @@ tree-verify.c (new)
      +	struct commit_list *iter;
      +	unsigned nr_before;
      +
     ++	if (repo->fetch_if_missing)
     ++		BUG("verify_commits_incremental must not be called "
     ++		    "with fetch_if_missing set");
     ++
      +	vs.trees = kh_init_oid_tree();
      +	vs.exclude_promisor_objects = exclude_promisor_objects;
      +

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check.

---
 .../technical/connectivity-check.adoc         | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc

new file mode 100644
index 0000000000..d20bff6af6
@@ -0,0 +1,109 @@
+Connectivity checking
+=====================
+
+After receiving new objects via fetch, push (receive-pack), clone,
+or bundle, Git verifies that the new reference tips do not leave
+the repository in a state where reachable objects are missing.
+This verification is called the connectivity check.
+
+Connectivity invariant
+----------------------
+
+A repository is connected when every object reachable from its
+references is available locally (with exceptions noted below).
+
+The connectivity check maintains this invariant when references
+are updated.  It trusts the existing connected state and verifies
+that the new reference tips do not introduce references to
+unavailable objects.  Verification is permitted to stop when it
+reaches objects already reachable from trusted existing
+references, since their closure is already connected.  These
+trusted references include local references and references from
+alternate object stores.
+
+Without this check, a truncated or corrupted transfer could leave
+a repository in a state where later history walks encounter
+missing objects.
+
+Exceptions
+~~~~~~~~~~
+
+Gitlink entries (submodule references) are excluded from
+connectivity checking.  Their target objects belong to a separate
+repository.
+
+In partial clones, objects promised by a promisor remote are
+accepted as connected without requiring local existence.  The
+check excludes promisor objects from traversal so that it does
+not trigger on-demand fetches for them.
+
+Full connectivity check
+-----------------------
+
+`check_connected()` (see `connected.c`) normally performs the
+connectivity check using a `rev-list` subprocess, feeding the
+new reference tips via stdin.  A normal invocation is roughly:
+
+    git rev-list --objects --stdin --not --all --quiet
+        --alternate-refs [--exclude-promisor-objects]
+
+When promisor remotes are configured, `check_connected()` first
+attempts a fast path based on promisor packfiles.  If it falls
+back to the `rev-list` check, `--exclude-promisor-objects` is
+added so that the traversal does not trigger on-demand fetches.
+
+Consider the following graph after a fetch, where all reference
+tips point directly to commits.  For simplicity, only local
+references appear on the already-connected side; alternate refs
+play the same role.  N3 is a merge commit:
+
+            /-------------L2
+           /
+    C1---B1---C2---B2-----L1
+          \         \
+           N1        N3---T2
+            \       /
+             N2-----------T1
+
+    L1, L2:         local refs
+    T1, T2:         incoming tips (new refs)
+    N1, N2, N3:     incoming commits (N3 is a merge)
+    B1, B2:         boundary commits (already connected)
+    C1, C2:         already connected (but not boundary)
+
+The incoming set is the commits reachable from the incoming
+tips but not from the already-connected side.  Boundary commits
+are the already-connected commits at the edge of that set.  Here
+B1 is an ancestor of B2, which happens when incoming branches
+fork at different depths in the existing history.
+
+The check proceeds in three phases:
+
+1. Walk from the incoming tips (T1, T2) against the trusted
+   refs (L1, L2) to find the incoming set ({N1, N2, N3, T1, T2}).
+
+2. Walk the trees of the boundary commits (B1, B2) and mark
+   those objects uninteresting.  These trees are already trusted
+   because their commits are on the already-connected side.
+
+3. Walk the trees of each incoming commit and verify that every
+   referenced object is connected, stopping at objects already
+   marked uninteresting in phase 2.
+
+Deepening fetches
+~~~~~~~~~~~~~~~~~
+
+For deepening fetches (where the shallow boundary moves), the
+full check omits `--not --all`.  There is no existing-reference
+boundary at which the walk can stop.  Instead, traversal follows
+the effective shallow boundary supplied for the deepened
+repository.  The new content may be below the old shallow
+boundary even when the tips themselves have not changed.
+
+Non-commit tips
+~~~~~~~~~~~~~~~
+
+When a new reference points to a non-commit object, such as a
+tag, tree, or blob, that object is not part of the commit walk.
+These non-commit tips are handled by the subsequent object
+traversal.

gitgitgadget

This is totally outside the topic of documentation updates, but it makes me wonder if we should pay attention to connectivity roots other than refs (like index entries) that we use when we run fsck.

Hmm, I'm not sure. I guess performance of the connectivity check is typically an issue on the server side only, much less so on the client side. And the server would of course typically not even have an index entry at all. Same for reflogs, at least in many setups.

I also wonder whether that'd really speed things up if we add more data sources. At GitLab we typically have the problem that we have too many connectivity roots with refs alone, and that is making the whole check painfully slow in some repositories. So adding more connectivity roots to it would probably be counterproductive.

Patrick

I have some followup work in this area (but I want to keep focus) and it's indeed true that number of refs used as seeds is also an important part of the problem.

I see connectivity and reachability as two separate things, though the current implementation has an invariant based on reachability implying (e.g. requiring) connectivity.

I think we could build up a stronger connectivity invariant for the common case, where we can cheaply check if an object is connectivity-closed regardless of its reachability state.

One idea is to utilize the fact that MIDX maps out a very specific set of packs, so we could compute which set of packs form a connectivity closure (and cheaply update that information as the MIDX changes).

Another idea would be to dynamically load refs as seeds in parallel with walking the graph. This creates a tradeoff between how many objects we need to check and how many refs we need to load (if we load fewer refs we just need to walk further to reach common ground).

Sorry, this is a bit hand-wave:y but my point is that I think regardless of how this is followed up to solve the bigger problems, I think optimizing the object-tree traversal is a useful first step.

Thanks,
Kristofer

Thanks, it's clear I'll need to do some more wordsmithing to improve the clarity here. I'll reroll/rewrite parts of it in some way (and aligned with the answers below).

Fair, I can make that more explicit in the text.

Agreed, I will make that more clear.

Good point, I will remove it and make the logic more self contained.

Yes, though I think this is important for understanding how the connectivity-check is implemented to see the relation to the rev-list walk. I have some hope that this dependency can go away in the future, since the general rev-list function is not necessarily the most optimal way to reason about connectivity.

That said, I am happy to remove this part of it's not deemed useful.

Good point, this is meant to reference back to the rev-list implementation but that's not very clear. And like the previous feedback, perhaps this should also go away (or be reworded in a more self contained way and less tied to rev-list)

Thanks,
Kristofer

From: Kristofer Karlsson <[email protected]>

The full connectivity check uses rev-list to find commits reachable from the incoming tips but not from the already-connected side, 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.

The verifier processes incoming commits with ancestors first and compares each new tree against its trusted parent trees. Entries already seen on the trusted side are skipped by OID, so unchanged subtrees need not remain at the same path to be recognized. Changed subtrees are recursively compared against same-path parent subtrees, and new subtrees without a comparison base are verified from scratch.

See Documentation/technical/connectivity-check.adoc for the trust invariants and detailed algorithm.

The incremental check runs as an internal --verify-trees-incremental mode in rev-list. After the normal revision walk identifies the incoming commits, the verifier consumes them before the usual object traversal; any pending non-commit tips are still handled by the existing traversal.

Partial-clone semantics are preserved: objects promised by a promisor remote are accepted as connected, and on-demand fetching is prevented by excluding promisor objects from traversal. The new mode is selected by transfer.connectivityCheck=incremental, with full remaining the default. Deepening fetches fall back to the full check because they do not have the normal existing-reference boundary, and traversal instead follows the effective shallow boundary.

p5412 results (median of 3), scaling one dimension at a time. Each fixture is a repository with a flat tree of many directories containing 100 files each. The incoming set is a chain of commits on top of the existing history, with each commit changing files in different directories round-robin. The three axes vary the total repository tree size, the number of incoming commits, and the number of files changed per incoming commit.

Scaling tree size (10 incoming commits, 10 files/commit):

    files    full  incr.  full/incr
      5K    0.01s  0.01s    1.0x
     50K    0.04s  0.01s    4.0x
    200K    0.15s  0.01s   15.0x
    800K    0.61s  0.03s   20.3x

The full mode must walk the trees of boundary commits, which grows with overall tree size. Incremental still scans the root trees, but avoids descending into unchanged subtrees, so it grows much more slowly with repository size.

Scaling incoming commit count (200K files, 10 files/commit):

    commits    full  incr.  full/incr
          1    0.14s  0.01s   14.0x
         10    0.15s  0.01s   15.0x
        100    0.19s  0.05s    3.8x
        500    0.32s  0.27s    1.2x
       3000    1.23s  1.52s    0.8x
       5000    1.88s  2.43s    0.8x
      10000    3.72s  5.36s    0.7x

With many commits the per-commit overhead of scanning both the new and parent root trees accumulates and incremental becomes slower. Breakeven is between 500 and 3000 commits and the ratio stabilizes near 0.7x for this fixture.

Scaling files per incoming commit (200K files, 10 commits):

    files/commit    full  incr.  full/incr
               1    0.14s  0.01s   14.0x
              10    0.15s  0.01s   15.0x
             100    0.16s  0.04s    4.0x
             500    0.23s  0.14s    1.6x
            1000    0.34s  0.28s    1.2x
            2000    0.70s  0.63s    1.1x

Breakeven is around 1000 files/commit. At 2000 files/commit (every directory touched), incremental remains slightly faster; bypassing the object cache for tree reads likely helps here.

For small repositories both modes are fast enough that the difference is difficult to measure reliably.

Selected peak RSS measurements from the same fixtures:

    case                        full    incr.  ratio
    200K files, 10 commits      31 MB    12 MB   0.4x
    800K files, 10 commits     110 MB    26 MB   0.2x
    200K files, 5000 commits   155 MB   151 MB   1.0x

Incremental uses substantially less memory when it can prune most of the boundary tree walk. In the long-history case the two modes visit similar object sets and memory converges.

The regression cases in the CPU benchmarks are in wall-clock time rather than memory, primarily from scanning some trees more than once.

---
 Documentation/config/transfer.adoc            |  20 +
 Documentation/rev-list-options.adoc           |   6 +
 .../technical/connectivity-check.adoc         | 134 ++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  19 +
 connected.c                                   |  24 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5412-connectivity-check.perl |  44 ++
 t/perf/p5412-connectivity-check.sh            |  92 +++
 t/t5412-connectivity-check.sh                 | 680 ++++++++++++++++++
 tree-verify.c                                 | 316 ++++++++
 tree-verify.h                                 |  15 +
 13 files changed, 1353 insertions(+)
 create mode 100644 t/perf/generate-repo-p5412-connectivity-check.perl
 create mode 100755 t/perf/p5412-connectivity-check.sh
 create mode 100755 t/t5412-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

index f1ce50f4a6..b9939d3bde 100644
@@ -1,3 +1,23 @@
+transfer.connectivityCheck::
+	Choose which algorithm to use for the connectivity check
+	performed during object transfer operations such as
+	linkgit:git-fetch[1] and linkgit:git-receive-pack[1].
+	The connectivity check verifies that all objects reachable
+	from the incoming tips are available locally or, in a partial
+	clone, promised by a promisor remote.
+	The variants are as follows:
++
+--
+`full` (default);;
+	Walk the full object closure of the boundary commits.
+`incremental`;;
+	Verify incoming commits by diffing their trees against parent
+	trees, recursively descending only into entries that differ.
+	The largest benefits occur when incoming commits change a
+	small fraction of a large tree closure.
+	Falls back to `full` for deepening fetches.
+--
+
 transfer.credentialsInUrl::
 	A configured URL can contain plaintext credentials in the form
 	`<protocol>://<user>:<password>@<domain>/<path>`. You may want

index fd831f0ec6..7964b691c4 100644
@@ -1089,6 +1089,12 @@ we cannot get their Object ID though, an error will be raised.
 	stronger than `--missing=allow-promisor` because it limits the
 	traversal, rather than just silencing errors about missing
 	objects.
+
+`--verify-trees-incremental`::
+	(For internal use only.)  Verify tree connectivity
+	incrementally by comparing each commit's tree against its
+	parent trees.  Used by `check_connected()` when
+	`transfer.connectivityCheck` is set to `incremental`.
 endif::git-rev-list[]
 
 `--no-walk[=(sorted|unsorted)]`::

index d4b775953d..aa5b4e2b84 100644
@@ -1357,6 +1357,7 @@ LIB_OBJS += trailer.o
 LIB_OBJS += transport-helper.o
 LIB_OBJS += transport.o
 LIB_OBJS += tree-diff.o
+LIB_OBJS += tree-verify.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += tree.o
 LIB_OBJS += unpack-trees.o

index 6b596231ab..beaec67d16 100644
@@ -28,6 +28,7 @@
 #include "commit-reach.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "tree-verify.h"
 
 struct rev_list_info {
 	struct rev_info *revs;

@@ -706,6 +707,7 @@ int cmd_rev_list(int argc,
 	int bisect_find_all = 0;
 	int use_bitmap_index = 0;
 	int filter_provided_objects = 0;
+	int verify_trees_incremental = 0;
 	const char *show_progress = NULL;
 	int ret = 0;
 

@@ -748,6 +750,9 @@ int cmd_rev_list(int argc,
 		if (!strcmp(arg, "--exclude-promisor-objects")) {
 			repo->fetch_if_missing = 0;
 			revs.exclude_promisor_objects = 1;
+		} else if (!strcmp(arg, "--verify-trees-incremental")) {
+			verify_trees_incremental = 1;
+			repo->fetch_if_missing = 0;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
 		} else if (!strcmp(arg, "-z")) {

@@ -822,6 +827,8 @@ int cmd_rev_list(int argc,
 
 		if (!strcmp(arg, "--exclude-promisor-objects"))
 			continue; /* already handled above */
+		if (!strcmp(arg, "--verify-trees-incremental"))
+			continue; /* already handled above */
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 

@@ -935,6 +942,18 @@ int cmd_rev_list(int argc,
 
 	prepare_maximal_independent(&revs);
 
+	if (verify_trees_incremental) {
+		struct commit *commit;
+		struct commit_list *new_commits = NULL;
+
+		while ((commit = get_revision(&revs)) != NULL)
+			commit_list_insert(commit, &new_commits);
+
+		verify_commits_incremental(repo, &new_commits,
+					   revs.exclude_promisor_objects);
+		commit_list_free(new_commits);
+	}
+
 	if (revs.tree_objects)
 		mark_edges_uninteresting(&revs, show_edge, 0);
 

index 929b9bd28d..e3dcd8e2b0 100644
@@ -1,6 +1,7 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "git-compat-util.h"
+#include "config.h"
 #include "gettext.h"
 #include "hex.h"
 #include "odb.h"

@@ -67,6 +68,26 @@ static int check_connected_promisor(oid_iterate_fn fn,
 	return 1;
 }
 
+static int incremental_check_applicable(struct check_connected_options *opt)
+{
+	const char *algorithm = NULL;
+
+	if (repo_config_get_string_tmp(the_repository,
+				       "transfer.connectivitycheck",
+				       &algorithm))
+		return 0;
+	if (!strcasecmp(algorithm, "full"))
+		return 0;
+	if (strcasecmp(algorithm, "incremental"))
+		die(_("unknown transfer.connectivityCheck algorithm '%s'"),
+		    algorithm);
+
+	if (opt->is_deepening_fetch)
+		return 0;
+
+	return 1;
+}
+
 /*
  * If we feed all the commits we want to verify to this command
  *

@@ -133,6 +154,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	if (opt->progress)
 		strvec_pushf(&rev_list.args, "--progress=%s",
 			     _("Checking connectivity"));
+	if (incremental_check_applicable(opt))
+		strvec_push(&rev_list.args,
+			    "--verify-trees-incremental");
 
 	rev_list.git_cmd = 1;
 	if (opt->env)

index 0a95d90d21..d94dedc26c 100644
@@ -562,6 +562,7 @@ libgit_sources = [
   'transport-helper.c',
   'transport.c',
   'tree-diff.c',
+  'tree-verify.c',
   'tree-walk.c',
   'tree.c',
   'unpack-trees.c',

index 7f53cca7d1..19a8246c44 100644
@@ -652,6 +652,7 @@ integration_tests = [
   't5409-colorize-remote-messages.sh',
   't5410-receive-pack.sh',
   't5411-proc-receive-hook.sh',
+  't5412-connectivity-check.sh',
   't5500-fetch-pack.sh',
   't5501-fetch-push-alternates.sh',
   't5502-quickfetch.sh',

new file mode 100644
index 0000000000..6aadadff70
@@ -0,0 +1,15 @@
+#ifndef TREE_VERIFY_H
+#define TREE_VERIFY_H
+
+struct commit_list;
+struct repository;
+
+/*
+ * Verify trees of commits incrementally against their parents.
+ * Dies on verification failure.
+ */
+void verify_commits_incremental(struct repository *repo,
+				struct commit_list **commits,
+				int exclude_promisor_objects);
+
+#endif /* TREE_VERIFY_H */

gitgitgadget

Okay. In the old world we basically mark evertyhing as uninteresting, including trees and blobs, ...

... whereas in the new world you propose to skip marking trees/blobs as uninteresting. Instead, the idea is to compare the trees/blobs of the old tips directly with the trees/blobs of the new tips and only verify those parts that have changed between the two?

This can of course cause us to verify significantly more objects in some scenarios. But it does have the consequence that we scale with the number of changes, not with the number of preexisting objects in the repository. And that's something I'd really appreciate, because marking reachable objects as uninteresting is extremely expensive.

One thing I wonder though... does this help with the scenario where we have tons of references or do we still end up passing "--not --all"? I have seen many times that parsing the refs by itself is dominating the time of the connectivity check quite significantly. So ideally, I'd like to have a solution that also catches this case. Your benchmarks do not cover that scenario though.

May I suggest splitting up this patch in the following way?

- One commit that introduces the new option, but for now only accepts "full" as the algorithm.

- One commit that introduces the benchmark.

- One commit that introduces the new flag for git-rev-list(1).

- One commit that then introduces the new strategy.

That may make it a bit easier to focus on the actual change.

[snip]

Yes, kind of, but I am not sure what you meant by old tips and new tips -- to be clear (and I should also write this more clearly) we only verify the commit-trees for all incoming commits, and the comparisons are always between a commit and its parents (i.e. X and X^1, X^2, ...). The key insight that makes the approach work is that we can skip any object we have seen from a trusted base (e.g. a commit that was already reachable before).

Yes, that's the goal in the happy case -- and I would argue it does not produce significantly more work in the worst case, due to caching and remembering everything we've seen so far (only more work within a 2x bound or so).

Yes, the problem of scaling out with number of refs is a real problem (and often a bigger one) and I want to address that too, but it's out of scope for this particular patch series.

Yes, I can definitely do that. Initially I was thinking that since it was basically only additions no obvious good in-between state, it wouldn't help much to split it up, but I think you convinced me there.

Parent is always defined relative to the commit we are currently verifying. For example, a push may come with 3 commits (let's call the tip T), and then we do the following comparisons:

    T   vs T^1, T^2
    T~1 vs T~1^1, T~1^2
    T~2 vs T~2^1, T~2^2

T~2^1 and T~2^2 must already exist and be reachable and so we can trust them to be connected. And since this is relying on memoizing already seen results, it's important to run the checks bottom-up (reverse topological order).

I will see if I can make this more obvious in the commit message or documentation somehow.

Yes, precisely.

That part works just as before -- rev-list finds the already-connected commits implicitly with the --not --all query. It actually finds all the new commits, but we can deduce the boundary from there (and the pre-existing rev-list code also does that).

Hm, I suppose I could just use the phrase "Example" here instead. Will change.

This is the same as before -- git rev-list produces the trust boundary based on reachability. I think the only new thing here is the inductive leap. Once we have verified a commit just above the trust boundary, that itself becomes a new trust boundary.

This is one of the advantages of running within a sub-process -- we can safely die without breaking things -- and this is in fact how the existing rev-list based implementation work, it will also die with an error message / return code that the parent process picks up.

My original implementation tried to do it all within a single process but it became painful because a lot of the internal machinery did not have non-fatal variants.

That said, I think long term it would be good to rework it into a single process and have the right infrastructure in place to avoid dying.

Thanks,
Kristofer

[snip]

This is the part that still eludes me though. How do we know that T~2^1 and T~2^2 must already exist and be reachable?

I think I was coming in with a false expectation that we're somehow getting rid of marking preexistingrefs as uninteresting, and that is where my confusion comes from. Because ultimately, that does not seem to be the case -- we still mark reference tips as uninteresting, as far as I can see. And then we can of course easily determine whether a specific commit is preexisting because we marked the boundary as uninteresting.

I was probably primed by my own earlier patch series in this context that focussed on refs, and that may be the reason why I had skewed expectations.

Yeah.

Ah, right, I forgot that we're running in a separate process. I think this will also become a bit clearer once this series is split up into smaller individual steps.

Thanks!

Patrick

Since T~2 is the bottom of the incoming commits, its parent commits must be part of the boundary -- by definition, since otherwise they would instead by included by rev-list --not --all.

Here is a concrete example with a push of 3 commits, where N1 is a merge commit and we have two pre-existing refs R1 and R2 (T, T~1, T~2 maps to N3, N2 and N1 respectively here):

B1---R1
\
N1---N2---N3
/
B2---R2

--not --all produces: {N1, N2, N3}

B1 and B2 are boundary (reachable from existing refs R1 and R2)

Verification (bottom-up):
N1: compare N1.tree vs B1.tree and B2.tree
(B1, B2 are boundary -- not in incoming set,
so connected by definition)
N2: compare N2.tree vs N1.tree
(N1 just verified, trusted inductively)
N3: compare N3.tree vs N2.tree
(N2 just verified, trusted inductively)

We never walk B1 or B2's full trees to mark objects uninteresting. We only read their root trees as comparison bases when they are direct parents of an incoming commit. That is where the savings come from.

So to directly answer your question: we know T~2's parents (B1 and B2 in this example) are connected because --not --all told us so -- they were not produced by the commit walk, which means they are reachable from existing refs. This is the same trust boundary the full check uses. The only new idea is the inductive step: once we verify a commit just above the boundary, it itself becomes trusted and extends the boundary upward.

Yes, precisely.

Yes, I've noted a few other workstreams that are sort of touching similar areas, though not directly the connectivity check, so it's a lot of context to juggle at the same time (and I will need to take that into account for the next steps).

Thanks,
Kristofer

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 t5413-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.

Changes since v2
================

Addresses Patrick's review (thanks!).

* Split from 2 commits to 4: documentation, perf test baseline, config plumbing + rev-list flag, then the incremental implementation.
* Documentation rewrites: added unreachable-objects caveat, removed code references to reduce staleness risk, replaced three-phase list with cost-focused prose, rewrote trust model to explicitly describe boundary mechanism and inductive argument, and several smaller clarifications.
* Extracted tree_oid variable in verify_commit_tree() to avoid calling get_commit_tree_oid(commit) twice.
* Rebased onto latest master to resolve a meson.build conflict (t5412 was taken by another topic). Renumbered test and perf files to t5413/p5413.

Changes since v1
================

* Guard against get_commit_tree_oid() returning NULL in verify_commit_tree(), dying with a clear message instead of a NULL dereference. (Thanks Junio for catching this)
* Set fetch_if_missing = 0 in the rev-list early option parse when --verify-trees-incremental is seen, and add a BUG() assertion in verify_commits_incremental() to catch any future caller that forgets. This makes the lazy-fetch protection explicit rather than relying solely on --exclude-promisor-objects having set it earlier. (Thanks again to Junio)
* Add a test that verifies promised blobs are not lazy-fetched during the incremental check.
* Reuse the existing 3-argument type-mismatch format string to avoid adding new strings for localization.

Thanks, Kristofer

Kristofer Karlsson (4):
Documentation: describe connectivity checking
p5413: add perf tests for connectivity check
connected: add transfer.connectivityCheck config and rev-list flag
connected: add incremental connectivity check via rev-list

 Documentation/config/transfer.adoc            |  20 +
 Documentation/rev-list-options.adoc           |   6 +
 .../technical/connectivity-check.adoc         | 255 +++++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  19 +
 connected.c                                   |  24 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5413-connectivity-check.perl |  44 ++
 t/perf/p5413-connectivity-check.sh            |  92 +++
 t/t5413-connectivity-check.sh                 | 680 ++++++++++++++++++
 tree-verify.c                                 | 316 ++++++++
 tree-verify.h                                 |  15 +
 13 files changed, 1474 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc
 create mode 100644 t/perf/generate-repo-p5413-connectivity-check.perl
 create mode 100755 t/perf/p5413-connectivity-check.sh
 create mode 100755 t/t5413-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2211%2Fspkrka%2Ftree-diff-connectivity-v1-clean-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2211/spkrka/tree-diff-connectivity-v1-clean-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2211

Range-diff vs v2:

1: 97c11449ae ! 1: 19ea8cb534 Documentation: describe connectivity checking
@@ Documentation/technical/connectivity-check.adoc (new)
+
+A repository is connected when every object reachable from its +references is available locally (with exceptions noted below).
++The reverse does not hold: objects that exist in the object ++database but are not reachable from any reference may be ++incomplete or lack a connected closure.
+
+The connectivity check maintains this invariant when references
-+are updated. It trusts the existing connected state and verifies -+that the new reference tips do not introduce references to -+unavailable objects. Verification is permitted to stop when it -+reaches objects already reachable from trusted existing -+references, since their closure is already connected. These -+trusted references include local references and references from -+alternate object stores.
++are updated. It trusts that all objects reachable from existing ++reference tips are already fully connected and verifies that the ++new reference tips do not introduce references to unavailable ++objects. Verification is permitted to stop when it reaches ++objects already reachable from existing references, since their ++closure is already connected. These trusted references include ++local references and references from alternate object stores.
+
+Without this check, a truncated or corrupted transfer could leave +a repository in a state where later history walks encounter
@@ Documentation/technical/connectivity-check.adoc (new)
+Full connectivity check
+-----------------------
+
-+check_connected() (see connected.c) normally performs the -+connectivity check using a rev-list subprocess, feeding the -+new reference tips via stdin. A normal invocation is roughly:
++The full connectivity check runs a rev-list subprocess, feeding ++the new reference tips via stdin. A normal invocation is roughly:
+
+ git rev-list --objects --stdin --not --all --quiet
+ --alternate-refs [--exclude-promisor-objects]
+
-+When promisor remotes are configured, check_connected() first -+attempts a fast path based on promisor packfiles. If it falls -+back to the rev-list check, --exclude-promisor-objects is -+added so that the traversal does not trigger on-demand fetches. ++When promisor remotes are configured, a fast path based on ++promisor packfiles is attempted first. If it falls back to ++the rev-list check, --exclude-promisor-objects is added so ++that the traversal does not trigger on-demand fetches.
+
+Consider the following graph after a fetch, where all reference +tips point directly to commits. For simplicity, only local
@@ Documentation/technical/connectivity-check.adoc (new)
+B1 is an ancestor of B2, which happens when incoming branches +fork at different depths in the existing history.
+
-+The check proceeds in three phases:
++The --not --all arguments establish the trust boundary: the ++incoming set is the commits reachable from the new tips but not ++from any existing reference.
+
-+1. Walk from the incoming tips (T1, T2) against the trusted
-+ refs (L1, L2) to find the incoming set ({N1, N2, N3, T1, T2}).
-+
-+2. Walk the trees of the boundary commits (B1, B2) and mark
-+ those objects uninteresting. These trees are already trusted
-+ because their commits are on the already-connected side.
-+
-+3. Walk the trees of each incoming commit and verify that every
-+ referenced object is connected, stopping at objects already
-+ marked uninteresting in phase 2.
++The traversal then walks the trees of the boundary commits and ++marks those objects uninteresting. Trees of incoming commits ++are verified against these marks, stopping at objects already ++covered. The cost of marking boundary trees uninteresting ++dominates when the boundary commits have large trees, even when ++the incoming changes are small.
+
+Deepening fetches
+~~~~~~~~~~~~~~~~~
@@ Documentation/technical/connectivity-check.adoc (new)
+
+When a new reference points to a non-commit object, such as a +tag, tree, or blob, that object is not part of the commit walk.
-+These non-commit tips are handled by the subsequent object -+traversal.
++These non-commit tips are verified during the tree and blob ++traversal that follows the commit walk within the same rev-list ++invocation.

 -:  ---------- > 2:  211794c925 p5413: add perf tests for connectivity check
 -:  ---------- > 3:  cdf28f1df6 connected: add transfer.connectivityCheck config and rev-list flag
 2:  6ad528f4bb ! 4:  efaada0b6a connected: add incremental connectivity check via rev-list
     @@ Commit message
          existing-reference boundary, and traversal instead follows the
          effective shallow boundary.
      
     -    p5412 results (median of 3), scaling one dimension at a time.
     +    p5413 results (median of 3), scaling one dimension at a time.
          Each fixture is a repository with a flat tree of many
          directories containing 100 files each.  The incoming set is a
          chain of commits on top of the existing history, with each
     @@ Commit message
      
          Signed-off-by: Kristofer Karlsson <[email protected]>
      
     - ## Documentation/config/transfer.adoc ##
     -@@
     -+transfer.connectivityCheck::
     -+	Choose which algorithm to use for the connectivity check
     -+	performed during object transfer operations such as
     -+	linkgit:git-fetch[1] and linkgit:git-receive-pack[1].
     -+	The connectivity check verifies that all objects reachable
     -+	from the incoming tips are available locally or, in a partial
     -+	clone, promised by a promisor remote.
     -+	The variants are as follows:
     -++
     -+--
     -+`full` (default);;
     -+	Walk the full object closure of the boundary commits.
     -+`incremental`;;
     -+	Verify incoming commits by diffing their trees against parent
     -+	trees, recursively descending only into entries that differ.
     -+	The largest benefits occur when incoming commits change a
     -+	small fraction of a large tree closure.
     -+	Falls back to `full` for deepening fetches.
     -+--
     -+
     - transfer.credentialsInUrl::
     - 	A configured URL can contain plaintext credentials in the form
     - 	`<protocol>://<user>:<password>@<domain>/<path>`. You may want
     -
     - ## Documentation/rev-list-options.adoc ##
     -@@ Documentation/rev-list-options.adoc: we cannot get their Object ID though, an error will be raised.
     - 	stronger than `--missing=allow-promisor` because it limits the
     - 	traversal, rather than just silencing errors about missing
     - 	objects.
     -+
     -+`--verify-trees-incremental`::
     -+	(For internal use only.)  Verify tree connectivity
     -+	incrementally by comparing each commit's tree against its
     -+	parent trees.  Used by `check_connected()` when
     -+	`transfer.connectivityCheck` is set to `incremental`.
     - endif::git-rev-list[]
     - 
     - `--no-walk[=(sorted|unsorted)]`::
     -
       ## Documentation/technical/connectivity-check.adoc ##
     -@@ Documentation/technical/connectivity-check.adoc: When a new reference points to a non-commit object, such as a
     - tag, tree, or blob, that object is not part of the commit walk.
     - These non-commit tips are handled by the subsequent object
     - traversal.
     +@@ Documentation/technical/connectivity-check.adoc: tag, tree, or blob, that object is not part of the commit walk.
     + These non-commit tips are verified during the tree and blob
     + traversal that follows the commit walk within the same rev-list
     + invocation.
      +
      +Incremental connectivity check
      +------------------------------
     @@ Documentation/technical/connectivity-check.adoc: When a new reference points to
      +`transfer.connectivityCheck=incremental`, avoids traversing the
      +full tree walk of the boundary commits.  Instead, it verifies
      +each incoming commit's tree against the already-trusted trees of
     -+its parents.
     ++its parent commits.
     ++
     ++The same `--not --all` boundary as the full check identifies the
     ++incoming commits.  The incremental mode reuses this boundary but
     ++replaces what happens afterwards: rather than walking boundary
     ++trees to mark objects uninteresting, it compares each incoming
     ++commit's tree against its parents' trees, processing commits in
     ++topological order (ancestors first).
      +
      +Trust model
      +~~~~~~~~~~~
      +
      +A tree is trusted when its transitive object closure is known to
     -+be connected.  Trees reachable from commits on the
     -+already-connected side of the boundary are therefore trusted.
     ++be connected.
      +
     -+Incoming commits are processed with ancestors before descendants.
     -+Once an incoming commit's tree has been verified, it is trusted
     -+and can be used as a comparison base for later descendants.
     ++Every parent of an incoming commit is either on the
     ++already-connected side of the `--not --all` boundary (a boundary
     ++parent whose tree is trusted by definition) or is an earlier
     ++incoming commit that was already verified due to the topological
     ++processing order.  In both cases the parent's tree is trusted, so
     ++all parent trees can be unconditionally used as comparison bases.
      +
     -+This gives an inductive correctness argument: every parent of the
     -+commit currently being verified is either already connected or is
     -+an earlier incoming commit whose tree has already been verified.
     ++Once an incoming commit's tree has been verified, it is trusted
     ++and can serve as a comparison base for later descendants.  This
     ++gives an inductive correctness argument: the base case is the
     ++trust boundary established by `--not --all`, and the inductive
     ++step is that verifying a commit extends the trusted set.
      +
      +Tree states
      +~~~~~~~~~~~
     @@ Documentation/technical/connectivity-check.adoc: When a new reference points to
      +during recursive descent.  If no comparison base is available,
      +the new subtree is verified from scratch.
      +
     -+Worked example
     -+~~~~~~~~~~~~~~
     ++Example
     ++~~~~~~~
      +
      +Consider a commit that changes one file under `lib/` and moves an
      +unchanged subtree from `src/` to `dev/`:
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       	int ret = 0;
       
      @@ builtin/rev-list.c: int cmd_rev_list(int argc,
     - 		if (!strcmp(arg, "--exclude-promisor-objects")) {
       			repo->fetch_if_missing = 0;
       			revs.exclude_promisor_objects = 1;
     -+		} else if (!strcmp(arg, "--verify-trees-incremental")) {
     + 		} else if (!strcmp(arg, "--verify-trees-incremental")) {
      +			verify_trees_incremental = 1;
     -+			repo->fetch_if_missing = 0;
     + 			repo->fetch_if_missing = 0;
       		} else if (skip_prefix(arg, "--missing=", &arg)) {
       			parse_missing_action_value(repo, arg);
     - 		} else if (!strcmp(arg, "-z")) {
     -@@ builtin/rev-list.c: int cmd_rev_list(int argc,
     - 
     - 		if (!strcmp(arg, "--exclude-promisor-objects"))
     - 			continue; /* already handled above */
     -+		if (!strcmp(arg, "--verify-trees-incremental"))
     -+			continue; /* already handled above */
     - 		if (skip_prefix(arg, "--missing=", &arg))
     - 			continue; /* already handled above */
     - 
      @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       
       	prepare_maximal_independent(&revs);
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       		mark_edges_uninteresting(&revs, show_edge, 0);
       
      
     - ## connected.c ##
     -@@
     - #define USE_THE_REPOSITORY_VARIABLE
     - 
     - #include "git-compat-util.h"
     -+#include "config.h"
     - #include "gettext.h"
     - #include "hex.h"
     - #include "odb.h"
     -@@ connected.c: static int check_connected_promisor(oid_iterate_fn fn,
     - 	return 1;
     - }
     - 
     -+static int incremental_check_applicable(struct check_connected_options *opt)
     -+{
     -+	const char *algorithm = NULL;
     -+
     -+	if (repo_config_get_string_tmp(the_repository,
     -+				       "transfer.connectivitycheck",
     -+				       &algorithm))
     -+		return 0;
     -+	if (!strcasecmp(algorithm, "full"))
     -+		return 0;
     -+	if (strcasecmp(algorithm, "incremental"))
     -+		die(_("unknown transfer.connectivityCheck algorithm '%s'"),
     -+		    algorithm);
     -+
     -+	if (opt->is_deepening_fetch)
     -+		return 0;
     -+
     -+	return 1;
     -+}
     -+
     - /*
     -  * If we feed all the commits we want to verify to this command
     -  *
     -@@ connected.c: int check_connected(oid_iterate_fn fn, void *cb_data,
     - 	if (opt->progress)
     - 		strvec_pushf(&rev_list.args, "--progress=%s",
     - 			     _("Checking connectivity"));
     -+	if (incremental_check_applicable(opt))
     -+		strvec_push(&rev_list.args,
     -+			    "--verify-trees-incremental");
     - 
     - 	rev_list.git_cmd = 1;
     - 	if (opt->env)
     -
       ## meson.build ##
      @@ meson.build: libgit_sources = [
         'transport-helper.c',
     @@ meson.build: libgit_sources = [
      
       ## t/meson.build ##
      @@ t/meson.build: integration_tests = [
     -   't5409-colorize-remote-messages.sh',
         't5410-receive-pack.sh',
         't5411-proc-receive-hook.sh',
     -+  't5412-connectivity-check.sh',
     +   't5412-receive-report-hook.sh',
     ++  't5413-connectivity-check.sh',
         't5500-fetch-pack.sh',
         't5501-fetch-push-alternates.sh',
         't5502-quickfetch.sh',
      
     - ## t/perf/generate-repo-p5412-connectivity-check.perl (new) ##
     + ## t/perf/p5413-connectivity-check.sh ##
      @@
     -+#!/usr/bin/perl
     -+#
     -+# Generate a fast-import stream for p5412 connectivity check benchmarks.
     -+#
     -+# Usage: generate-repo-p5412-connectivity-check.perl
     -+#            <dirs> <files_per_dir> <commits> [<hot_dirs>] [<files_per_commit>]
     -+#
     -+# Creates one initial commit with dirs*files_per_dir files, then
     -+# <commits> additional commits each modifying <files_per_commit>
     -+# files in directories chosen round-robin from 1..<hot_dirs>.
     -+
     -+use strict;
     -+use warnings;
     -+
     -+my ($nd, $nf, $nc, $hot, $fpc) = @ARGV;
     -+$hot = $nd if !$hot || $hot > $nd;
     -+$fpc = 1   if !$fpc;
     -+
     -+sub data {
     -+	printf "data %d\n%s\n", length($_[0]), $_[0];
     -+}
     -+
     -+# Initial tree: one commit with nd*nf files.
     -+printf "commit refs/heads/main\n";
     -+printf "committer perf <perf\@test.com> now\n";
     -+data("initial");
     -+for my $d (1..$nd) {
     -+	for my $f (1..$nf) {
     -+		printf "M 100644 inline d-%04d/f-%03d\n", $d, $f;
     -+		data(sprintf "%03d%03d", $d, $f);
     -+	}
     -+}
     -+
     -+# Subsequent commits (auto-chained by fast-import).
     -+for my $i (1..$nc) {
     -+	printf "commit refs/heads/main\n";
     -+	printf "committer perf <perf\@test.com> now\n";
     -+	data(sprintf "change-%03d", $i);
     -+	for my $j (0..$fpc-1) {
     -+		my $d = (($i + $j) % $hot) + 1;
     -+		printf "M 100644 inline d-%04d/f-001\n", $d;
     -+		data(sprintf "c%d-%d", $i, $j);
     -+	}
     -+}
     -
     - ## t/perf/p5412-connectivity-check.sh (new) ##
     -@@
     -+#!/bin/sh
     -+
     + #!/bin/sh
     + 
     +-test_description='performance of connectivity check
      +test_description='performance of connectivity check modes
     -+
     + 
     +-Benchmark the rev-list connectivity check, avoiding pack transfer
     +-noise.
      +Compare the default and incremental rev-list connectivity modes
      +directly, avoiding pack transfer noise.
     -+
     -+Each repository has a flat tree of many directories with 100 files
     -+in each.  Three axes are scaled independently: tree size, commit
     -+count, and files changed per commit.'
     -+
     -+. ./perf-lib.sh
     -+
     -+test_perf_fresh_repo
     -+
     -+generate="$TEST_DIRECTORY/perf/generate-repo-p5412-connectivity-check.perl"
     -+
     -+# $1=dirs  $2=files_per_dir  $3=commits  $4=hot_dirs (optional, default=all)
     -+# $5=files_per_commit (optional, default=1)
     -+test_perf_conn () {
     -+	local nd="$1" nf="$2" nc="$3" hot="${4:-$1}" fpc="${5:-1}"
     -+	local total=$(($nd * $nf))
     -+	local name="repo-${nd}d-${nf}f-${nc}c-${hot}h-${fpc}fpc"
     -+	local label="${total} files, ${nc} commits"
     -+	if test "$hot" -lt "$nd"
     -+	then
     -+		label="$label (${hot} hot dirs)"
     -+	fi
     -+	if test "$fpc" -gt 1
     -+	then
     -+		label="$label (${fpc} files/commit)"
     -+	fi
     -+
     -+	test_expect_success "setup $label" '
     -+		if test -d '"$name"'
     -+		then
     -+			true
     -+		else
     -+			git init '"$name"' &&
     -+			"$PERL_PATH" '"$generate"' '"$nd"' '"$nf"' '"$nc"' '"$hot"' '"$fpc"' |
     -+			git -C '"$name"' fast-import --date-format=now --quiet &&
     -+			(
     -+				cd '"$name"' &&
     -+				git rev-parse main~'"$nc"' >../'"${name}"'_old &&
     -+				git rev-parse main >../'"${name}"'_new &&
     -+				git update-ref refs/heads/main \
     -+					$(cat ../'"${name}"'_old) &&
     -+				git repack -ad &&
     -+				git config gc.auto 0
     -+			)
     -+		fi
     -+	'
     -+
     + 
     + Each repository has a flat tree of many directories with 100 files
     + in each.  Three axes are scaled independently: tree size, commit
     +@@ t/perf/p5413-connectivity-check.sh: test_perf_conn () {
     + 		fi
     + 	'
     + 
     +-	test_perf "$label" '
      +	test_perf "$label (full)" '
     -+		cat '"${name}"'_new |
     -+		git -C '"$name"' rev-list \
     -+			--objects --stdin --not --all --quiet \
     -+			--exclude-promisor-objects
     -+	'
     + 		cat '"${name}"'_new |
     + 		git -C '"$name"' rev-list \
     + 			--objects --stdin --not --all --quiet \
     + 			--exclude-promisor-objects
     + 	'
      +
      +	test_perf "$label (incremental)" '
      +		cat '"${name}"'_new |
     @@ t/perf/p5412-connectivity-check.sh (new)
      +			--objects --stdin --not --all --quiet \
      +			--exclude-promisor-objects
      +	'
     -+}
     -+
     -+# Scaling tree size (10 commits, 10 files/commit).
     -+test_perf_conn   50 100 10   50 10
     -+test_perf_conn  500 100 10  500 10
     -+test_perf_conn 2000 100 10 2000 10
     -+test_perf_conn 8000 100 10 8000 10
     -+
     -+# Scaling commit count (200K files, 10 files/commit).
     -+test_perf_conn 2000 100    1 2000 10
     -+test_perf_conn 2000 100   10 2000 10
     -+test_perf_conn 2000 100  100 2000 10
     -+test_perf_conn 2000 100   500 2000 10
     -+test_perf_conn 2000 100  3000 2000 10
     -+test_perf_conn 2000 100  5000 2000 10
     -+test_perf_conn 2000 100 10000 2000 10
     -+
     -+# Scaling files per commit (200K files, 10 commits).
     -+test_perf_conn 2000 100 10 2000   1
     -+test_perf_conn 2000 100 10 2000  10
     -+test_perf_conn 2000 100 10 2000  100
     -+test_perf_conn 2000 100 10 2000  500
     -+test_perf_conn 2000 100 10 2000 1000
     -+test_perf_conn 2000 100 10 2000 2000
     -+
     -+test_done
     + }
     + 
     + # Scaling tree size (10 commits, 10 files/commit).
      
     - ## t/t5412-connectivity-check.sh (new) ##
     + ## t/t5413-connectivity-check.sh (new) ##
      @@
      +#!/bin/sh
      +
     @@ tree-verify.c (new)
      +			       struct verify_state *vs)
      +{
      +	struct oid_array base_trees = OID_ARRAY_INIT;
     ++	const struct object_id *tree_oid;
      +	struct commit_list *p;
      +
      +	/*
     @@ tree-verify.c (new)
      +	 * 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);
      +		if (!tree_oid)
     @@ tree-verify.c (new)
      +		oid_array_append(&base_trees, tree_oid);
      +	}
      +
     -+	if (!get_commit_tree_oid(commit))
     ++	tree_oid = get_commit_tree_oid(commit);
     ++	if (!tree_oid)
      +		die(_("unable to load root tree for commit %s"),
      +		    oid_to_hex(&commit->object.oid));
     -+	verify_tree(repo, get_commit_tree_oid(commit),
     -+		    &base_trees, vs, 0);
     ++	verify_tree(repo, tree_oid, &base_trees, vs, 0);
      +	oid_array_clear(&base_trees);
      +}
      +

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check.

---
 .../technical/connectivity-check.adoc         | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc

new file mode 100644
index 0000000000..abef02debd
@@ -0,0 +1,110 @@
+Connectivity checking
+=====================
+
+After receiving new objects via fetch, push (receive-pack), clone,
+or bundle, Git verifies that the new reference tips do not leave
+the repository in a state where reachable objects are missing.
+This verification is called the connectivity check.
+
+Connectivity invariant
+----------------------
+
+A repository is connected when every object reachable from its
+references is available locally (with exceptions noted below).
+The reverse does not hold: objects that exist in the object
+database but are not reachable from any reference may be
+incomplete or lack a connected closure.
+
+The connectivity check maintains this invariant when references
+are updated.  It trusts that all objects reachable from existing
+reference tips are already fully connected and verifies that the
+new reference tips do not introduce references to unavailable
+objects.  Verification is permitted to stop when it reaches
+objects already reachable from existing references, since their
+closure is already connected.  These trusted references include
+local references and references from alternate object stores.
+
+Without this check, a truncated or corrupted transfer could leave
+a repository in a state where later history walks encounter
+missing objects.
+
+Exceptions
+~~~~~~~~~~
+
+Gitlink entries (submodule references) are excluded from
+connectivity checking.  Their target objects belong to a separate
+repository.
+
+In partial clones, objects promised by a promisor remote are
+accepted as connected without requiring local existence.  The
+check excludes promisor objects from traversal so that it does
+not trigger on-demand fetches for them.
+
+Full connectivity check
+-----------------------
+
+The full connectivity check runs a `rev-list` subprocess, feeding
+the new reference tips via stdin.  A normal invocation is roughly:
+
+    git rev-list --objects --stdin --not --all --quiet
+        --alternate-refs [--exclude-promisor-objects]
+
+When promisor remotes are configured, a fast path based on
+promisor packfiles is attempted first.  If it falls back to
+the `rev-list` check, `--exclude-promisor-objects` is added so
+that the traversal does not trigger on-demand fetches.
+
+Consider the following graph after a fetch, where all reference
+tips point directly to commits.  For simplicity, only local
+references appear on the already-connected side; alternate refs
+play the same role.  N3 is a merge commit:
+
+            /-------------L2
+           /
+    C1---B1---C2---B2-----L1
+          \         \
+           N1        N3---T2
+            \       /
+             N2-----------T1
+
+    L1, L2:         local refs
+    T1, T2:         incoming tips (new refs)
+    N1, N2, N3:     incoming commits (N3 is a merge)
+    B1, B2:         boundary commits (already connected)
+    C1, C2:         already connected (but not boundary)
+
+The incoming set is the commits reachable from the incoming
+tips but not from the already-connected side.  Boundary commits
+are the already-connected commits at the edge of that set.  Here
+B1 is an ancestor of B2, which happens when incoming branches
+fork at different depths in the existing history.
+
+The `--not --all` arguments establish the trust boundary: the
+incoming set is the commits reachable from the new tips but not
+from any existing reference.
+
+The traversal then walks the trees of the boundary commits and
+marks those objects uninteresting.  Trees of incoming commits
+are verified against these marks, stopping at objects already
+covered.  The cost of marking boundary trees uninteresting
+dominates when the boundary commits have large trees, even when
+the incoming changes are small.
+
+Deepening fetches
+~~~~~~~~~~~~~~~~~
+
+For deepening fetches (where the shallow boundary moves), the
+full check omits `--not --all`.  There is no existing-reference
+boundary at which the walk can stop.  Instead, traversal follows
+the effective shallow boundary supplied for the deepened
+repository.  The new content may be below the old shallow
+boundary even when the tips themselves have not changed.
+
+Non-commit tips
+~~~~~~~~~~~~~~~
+
+When a new reference points to a non-commit object, such as a
+tag, tree, or blob, that object is not part of the commit walk.
+These non-commit tips are verified during the tree and blob
+traversal that follows the commit walk within the same rev-list
+invocation.

gitgitgadget

Shouldn't both the Makefile (TECH_DOCS) and Meson (articles) be adapted to build this?

[snip]

Ah yes, will fix for next reroll.

Hm perhaps. Or I can remove it instead, I think it depends on how detailed we want this to be. I will take a deeper look at this.

Agreed, I think that would be easier to read. Will fix.

Thanks,
Kristofer

From: Kristofer Karlsson <[email protected]>

Add benchmarks for the rev-list connectivity check. Each fixture varies one dimension: total tree size, incoming commit count, or files changed per commit.

---
 ...enerate-repo-p5413-connectivity-check.perl | 44 ++++++++++
 t/perf/p5413-connectivity-check.sh            | 85 +++++++++++++++++++
 2 files changed, 129 insertions(+)
 create mode 100644 t/perf/generate-repo-p5413-connectivity-check.perl
 create mode 100755 t/perf/p5413-connectivity-check.sh

new file mode 100644
index 0000000000..9546996280
@@ -0,0 +1,44 @@
+#!/usr/bin/perl
+#
+# Generate a fast-import stream for p5412 connectivity check benchmarks.
+#
+# Usage: generate-repo-p5412-connectivity-check.perl
+#            <dirs> <files_per_dir> <commits> [<hot_dirs>] [<files_per_commit>]
+#
+# Creates one initial commit with dirs*files_per_dir files, then
+# <commits> additional commits each modifying <files_per_commit>
+# files in directories chosen round-robin from 1..<hot_dirs>.
+
+use strict;
+use warnings;
+
+my ($nd, $nf, $nc, $hot, $fpc) = @ARGV;
+$hot = $nd if !$hot || $hot > $nd;
+$fpc = 1   if !$fpc;
+
+sub data {
+	printf "data %d\n%s\n", length($_[0]), $_[0];
+}
+
+# Initial tree: one commit with nd*nf files.
+printf "commit refs/heads/main\n";
+printf "committer perf <perf\@test.com> now\n";
+data("initial");
+for my $d (1..$nd) {
+	for my $f (1..$nf) {
+		printf "M 100644 inline d-%04d/f-%03d\n", $d, $f;
+		data(sprintf "%03d%03d", $d, $f);
+	}
+}
+
+# Subsequent commits (auto-chained by fast-import).
+for my $i (1..$nc) {
+	printf "commit refs/heads/main\n";
+	printf "committer perf <perf\@test.com> now\n";
+	data(sprintf "change-%03d", $i);
+	for my $j (0..$fpc-1) {
+		my $d = (($i + $j) % $hot) + 1;
+		printf "M 100644 inline d-%04d/f-001\n", $d;
+		data(sprintf "c%d-%d", $i, $j);
+	}
+}

gitgitgadget

Ah good catch, will add.

Oops, I fixed the file collision but forgot to go back and clean up text references, will fix.

Thanks,
Kristofer

From: Kristofer Karlsson <[email protected]>

Add the transfer.connectivityCheck configuration option, accepting "full" (default) and "incremental". When set to "incremental", check_connected() passes --verify-trees-incremental to its rev-list subprocess.

The rev-list flag is accepted but has no effect yet; the incremental verification will be implemented in the next commit.

---
 Documentation/config/transfer.adoc  | 20 ++++++++++++++++++++
 Documentation/rev-list-options.adoc |  6 ++++++
 builtin/rev-list.c                  |  4 ++++
 connected.c                         | 24 ++++++++++++++++++++++++
 4 files changed, 54 insertions(+)

index f1ce50f4a6..b9939d3bde 100644
@@ -1,3 +1,23 @@
+transfer.connectivityCheck::
+	Choose which algorithm to use for the connectivity check
+	performed during object transfer operations such as
+	linkgit:git-fetch[1] and linkgit:git-receive-pack[1].
+	The connectivity check verifies that all objects reachable
+	from the incoming tips are available locally or, in a partial
+	clone, promised by a promisor remote.
+	The variants are as follows:
++
+--
+`full` (default);;
+	Walk the full object closure of the boundary commits.
+`incremental`;;
+	Verify incoming commits by diffing their trees against parent
+	trees, recursively descending only into entries that differ.
+	The largest benefits occur when incoming commits change a
+	small fraction of a large tree closure.
+	Falls back to `full` for deepening fetches.
+--
+
 transfer.credentialsInUrl::
 	A configured URL can contain plaintext credentials in the form
 	`<protocol>://<user>:<password>@<domain>/<path>`. You may want

index bd9f345690..35c4bf882f 100644
@@ -1102,6 +1102,12 @@ It is an error to use `--missing-only` without `--missing=print` or
 	stronger than `--missing=allow-promisor` because it limits the
 	traversal, rather than just silencing errors about missing
 	objects.
+
+`--verify-trees-incremental`::
+	(For internal use only.)  Verify tree connectivity
+	incrementally by comparing each commit's tree against its
+	parent trees.  Used by `check_connected()` when
+	`transfer.connectivityCheck` is set to `incremental`.
 endif::git-rev-list[]
 
 `--no-walk[=(sorted|unsorted)]`::

index 0faa833fac..9a76bb9c9c 100644
@@ -769,6 +769,8 @@ int cmd_rev_list(int argc,
 		if (!strcmp(arg, "--exclude-promisor-objects")) {
 			repo->fetch_if_missing = 0;
 			revs.exclude_promisor_objects = 1;
+		} else if (!strcmp(arg, "--verify-trees-incremental")) {
+			repo->fetch_if_missing = 0;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
 		} else if (!strcmp(arg, "--missing-only")) {

@@ -848,6 +850,8 @@ int cmd_rev_list(int argc,
 
 		if (!strcmp(arg, "--exclude-promisor-objects"))
 			continue; /* already handled above */
+		if (!strcmp(arg, "--verify-trees-incremental"))
+			continue; /* already handled above */
 		if (skip_prefix(arg, "--missing=", &arg))
 			continue; /* already handled above */
 

index 929b9bd28d..e3dcd8e2b0 100644
@@ -1,6 +1,7 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "git-compat-util.h"
+#include "config.h"
 #include "gettext.h"
 #include "hex.h"
 #include "odb.h"

@@ -67,6 +68,26 @@ static int check_connected_promisor(oid_iterate_fn fn,
 	return 1;
 }
 
+static int incremental_check_applicable(struct check_connected_options *opt)
+{
+	const char *algorithm = NULL;
+
+	if (repo_config_get_string_tmp(the_repository,
+				       "transfer.connectivitycheck",
+				       &algorithm))
+		return 0;
+	if (!strcasecmp(algorithm, "full"))
+		return 0;
+	if (strcasecmp(algorithm, "incremental"))
+		die(_("unknown transfer.connectivityCheck algorithm '%s'"),
+		    algorithm);
+
+	if (opt->is_deepening_fetch)
+		return 0;
+
+	return 1;
+}
+
 /*
  * If we feed all the commits we want to verify to this command
  *

@@ -133,6 +154,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	if (opt->progress)
 		strvec_pushf(&rev_list.args, "--progress=%s",
 			     _("Checking connectivity"));
+	if (incremental_check_applicable(opt))
+		strvec_push(&rev_list.args,
+			    "--verify-trees-incremental");
 
 	rev_list.git_cmd = 1;
 	if (opt->env)

gitgitgadget

"Kristofer Karlsson via GitGitGadget" <[email protected]>

Yes, I could add the repo pointer to the structure if needed. I initally just considered adding it as a function parameter (which would not be a large benefit, except pushing out the repository usage to the outer layers at least) but adding it to the struct would perhaps be cleaner.

I could also add a slightly larger patch to fix the entire file and remove USE_THE_REPOSITORY_VARIABLE. I don't think it would be overly messy.

I will play around a bit with both these ideas and see where it ends up for the next reroll.

Thanks,
Kristofer

Although I wouldn't mind if we decide to leave it outside the scope of the current topic in the end, it is worth considering when we see a patch adding new use of the global.

Thanks.

I think this step is a tiny bit awkward. My suggestion was rather aiming to introduce the configuration option, but initially only accept "full" for the current behaviour. The "--verify-trees-incremental" flag could then be implemented in the next commit, and the final commit would then tie all of this together by introducing the "incremental" value.

Patrick

Hm yes, I can try to do that.

Thanks,
Kristofer

From: Kristofer Karlsson <[email protected]>

The full connectivity check uses rev-list to find commits reachable from the incoming tips but not from the already-connected side, 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.

The verifier processes incoming commits with ancestors first and compares each new tree against its trusted parent trees. Entries already seen on the trusted side are skipped by OID, so unchanged subtrees need not remain at the same path to be recognized. Changed subtrees are recursively compared against same-path parent subtrees, and new subtrees without a comparison base are verified from scratch.

See Documentation/technical/connectivity-check.adoc for the trust invariants and detailed algorithm.

The incremental check runs as an internal --verify-trees-incremental mode in rev-list. After the normal revision walk identifies the incoming commits, the verifier consumes them before the usual object traversal; any pending non-commit tips are still handled by the existing traversal.

Partial-clone semantics are preserved: objects promised by a promisor remote are accepted as connected, and on-demand fetching is prevented by excluding promisor objects from traversal. The new mode is selected by transfer.connectivityCheck=incremental, with full remaining the default. Deepening fetches fall back to the full check because they do not have the normal existing-reference boundary, and traversal instead follows the effective shallow boundary.

p5413 results (median of 3), scaling one dimension at a time. Each fixture is a repository with a flat tree of many directories containing 100 files each. The incoming set is a chain of commits on top of the existing history, with each commit changing files in different directories round-robin. The three axes vary the total repository tree size, the number of incoming commits, and the number of files changed per incoming commit.

Scaling tree size (10 incoming commits, 10 files/commit):

    files    full  incr.  full/incr
      5K    0.01s  0.01s    1.0x
     50K    0.04s  0.01s    4.0x
    200K    0.15s  0.01s   15.0x
    800K    0.61s  0.03s   20.3x

The full mode must walk the trees of boundary commits, which grows with overall tree size. Incremental still scans the root trees, but avoids descending into unchanged subtrees, so it grows much more slowly with repository size.

Scaling incoming commit count (200K files, 10 files/commit):

    commits    full  incr.  full/incr
          1    0.14s  0.01s   14.0x
         10    0.15s  0.01s   15.0x
        100    0.19s  0.05s    3.8x
        500    0.32s  0.27s    1.2x
       3000    1.23s  1.52s    0.8x
       5000    1.88s  2.43s    0.8x
      10000    3.72s  5.36s    0.7x

With many commits the per-commit overhead of scanning both the new and parent root trees accumulates and incremental becomes slower. Breakeven is between 500 and 3000 commits and the ratio stabilizes near 0.7x for this fixture.

Scaling files per incoming commit (200K files, 10 commits):

    files/commit    full  incr.  full/incr
               1    0.14s  0.01s   14.0x
              10    0.15s  0.01s   15.0x
             100    0.16s  0.04s    4.0x
             500    0.23s  0.14s    1.6x
            1000    0.34s  0.28s    1.2x
            2000    0.70s  0.63s    1.1x

Breakeven is around 1000 files/commit. At 2000 files/commit (every directory touched), incremental remains slightly faster; bypassing the object cache for tree reads likely helps here.

For small repositories both modes are fast enough that the difference is difficult to measure reliably.

Selected peak RSS measurements from the same fixtures:

    case                        full    incr.  ratio
    200K files, 10 commits      31 MB    12 MB   0.4x
    800K files, 10 commits     110 MB    26 MB   0.2x
    200K files, 5000 commits   155 MB   151 MB   1.0x

Incremental uses substantially less memory when it can prune most of the boundary tree walk. In the long-history case the two modes visit similar object sets and memory converges.

The regression cases in the CPU benchmarks are in wall-clock time rather than memory, primarily from scanning some trees more than once.

---
 .../technical/connectivity-check.adoc         | 145 ++++
 Makefile                                      |   1 +
 builtin/rev-list.c                            |  15 +
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 t/perf/p5413-connectivity-check.sh            |  15 +-
 t/t5413-connectivity-check.sh                 | 680 ++++++++++++++++++
 tree-verify.c                                 | 316 ++++++++
 tree-verify.h                                 |  15 +
 9 files changed, 1185 insertions(+), 4 deletions(-)
 create mode 100755 t/t5413-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

index abef02debd..a1d2e9709c 100644
@@ -108,3 +108,148 @@ tag, tree, or blob, that object is not part of the commit walk.
 These non-commit tips are verified during the tree and blob
 traversal that follows the commit walk within the same rev-list
 invocation.
+
+Incremental connectivity check
+------------------------------
+
+The incremental mode, selected by
+`transfer.connectivityCheck=incremental`, avoids traversing the
+full tree walk of the boundary commits.  Instead, it verifies
+each incoming commit's tree against the already-trusted trees of
+its parent commits.
+
+The same `--not --all` boundary as the full check identifies the
+incoming commits.  The incremental mode reuses this boundary but
+replaces what happens afterwards: rather than walking boundary
+trees to mark objects uninteresting, it compares each incoming
+commit's tree against its parents' trees, processing commits in
+topological order (ancestors first).
+
+Trust model
+~~~~~~~~~~~
+
+A tree is trusted when its transitive object closure is known to
+be connected.
+
+Every parent of an incoming commit is either on the
+already-connected side of the `--not --all` boundary (a boundary
+parent whose tree is trusted by definition) or is an earlier
+incoming commit that was already verified due to the topological
+processing order.  In both cases the parent's tree is trusted, so
+all parent trees can be unconditionally used as comparison bases.
+
+Once an incoming commit's tree has been verified, it is trusted
+and can serve as a comparison base for later descendants.  This
+gives an inductive correctness argument: the base case is the
+trust boundary established by `--not --all`, and the inductive
+step is that verifying a commit extends the trusted set.
+
+Tree states
+~~~~~~~~~~~
+
+The verifier tracks tree OIDs in three states:
+
+untrusted::
+	The tree has not yet been established as connected.  This
+	is the implicit state of an OID not present in the state
+	map.
+
+trusted::
+	The tree is known to have a connected transitive closure,
+	but its direct entries have not yet been published into the
+	verifier's trusted object sets.
+
+expanded::
+	The tree is trusted and its direct non-gitlink entries
+	have also been published into the trusted object sets.
+
+State transitions are monotonic: a tree may move from untrusted
+to trusted to expanded, but never backwards.  An untrusted tree
+that is successfully verified goes directly to expanded.
+
+Blobs require only trusted/untrusted state: a blob becomes
+trusted when it is found in a trusted tree or when its existence
+and type have been verified directly.
+
+The trusted/expanded distinction is an optimization.  An expanded
+parent does not need to be reread merely to publish entries that
+are already trusted, though it may still be read when same-path
+subtree bases are needed for recursive comparison.
+
+Algorithm
+~~~~~~~~~
+
+At a high level:
+
+    verify(commits):
+        sort topologically (ancestors first)
+        for each commit:
+            mark parent root trees as trusted
+            verify_tree(commit.tree, parent root trees)
+
+    verify_tree(tree, base_trees):
+        if tree already trusted: return
+        read tree, collect entries not already trusted
+        for each available base tree:
+            publish its entries as trusted
+            record same-path subtrees as bases
+        for each collected entry:
+            if now trusted: skip
+            if blob: verify blob connectivity and type
+            if tree: verify_tree(entry, its recorded bases)
+        mark tree as expanded
+
+The important ordering within `verify_tree` is that entries from
+the new tree are collected before the base trees are scanned, but
+are processed only afterwards.  Trust learned from any base can
+therefore eliminate work before recursive verification begins.
+
+Same-path parent subtrees are passed down as comparison bases
+during recursive descent.  If no comparison base is available,
+the new subtree is verified from scratch.
+
+Example
+~~~~~~~
+
+Consider a commit that changes one file under `lib/` and 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)
+
+Scanning the parent makes `aaa` trusted even though it moved from
+`src/` to `dev/`, so that subtree is skipped.  The changed `ccc`
+subtree is compared against its same-path parent `bbb`; scanning
+`bbb` makes `ddd` and `eee` trusted, leaving only the new `fff`
+blob to be checked.
+
+This illustrates two properties:
+
+* Trust is OID-based rather than path-based.  An unchanged subtree
+  is recognized after a move.
+
+* Same-path parent subtrees provide recursive comparison bases.
+  These bases improve pruning efficiency but are not required for
+  correctness; a new subtree can always be verified from scratch.
+
+Multiple parents
+~~~~~~~~~~~~~~~~
+
+For a merge commit, root trees from all parents are comparison
+bases.  When several parents contain a same-path subtree, each
+matching subtree is collected as a recursive comparison base.
+
+Entries published from any trusted parent become globally trusted,
+so a matching tree or blob entry present in any parent can be
+skipped while verifying the merge tree.
+
+Missing comparison bases
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+A promised base tree may not be locally available for comparison.
+In that case the verifier skips that base and verifies the new
+subtree without it.  The missing comparison can reduce pruning but
+does not affect correctness.

index a96be506b5..fa42e295e2 100644
@@ -1339,6 +1339,7 @@ LIB_OBJS += trailer.o
 LIB_OBJS += transport-helper.o
 LIB_OBJS += transport.o
 LIB_OBJS += tree-diff.o
+LIB_OBJS += tree-verify.o
 LIB_OBJS += tree-walk.o
 LIB_OBJS += tree.o
 LIB_OBJS += unpack-trees.o

index 9a76bb9c9c..ae5012ec25 100644
@@ -28,6 +28,7 @@
 #include "commit-reach.h"
 #include "quote.h"
 #include "strbuf.h"
+#include "tree-verify.h"
 
 struct rev_list_info {
 	struct rev_info *revs;

@@ -727,6 +728,7 @@ int cmd_rev_list(int argc,
 	int bisect_find_all = 0;
 	int use_bitmap_index = 0;
 	int filter_provided_objects = 0;
+	int verify_trees_incremental = 0;
 	const char *show_progress = NULL;
 	int ret = 0;
 

@@ -770,6 +772,7 @@ int cmd_rev_list(int argc,
 			repo->fetch_if_missing = 0;
 			revs.exclude_promisor_objects = 1;
 		} else if (!strcmp(arg, "--verify-trees-incremental")) {
+			verify_trees_incremental = 1;
 			repo->fetch_if_missing = 0;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);

@@ -973,6 +976,18 @@ int cmd_rev_list(int argc,
 
 	prepare_maximal_independent(&revs);
 
+	if (verify_trees_incremental) {
+		struct commit *commit;
+		struct commit_list *new_commits = NULL;
+
+		while ((commit = get_revision(&revs)) != NULL)
+			commit_list_insert(commit, &new_commits);
+
+		verify_commits_incremental(repo, &new_commits,
+					   revs.exclude_promisor_objects);
+		commit_list_free(new_commits);
+	}
+
 	if (revs.tree_objects)
 		mark_edges_uninteresting(&revs, show_edge, 0);
 

index 0a95d90d21..d94dedc26c 100644
@@ -562,6 +562,7 @@ libgit_sources = [
   'transport-helper.c',
   'transport.c',
   'tree-diff.c',
+  'tree-verify.c',
   'tree-walk.c',
   'tree.c',
   'unpack-trees.c',

index f65eb04684..b23b4c90b0 100644
@@ -654,6 +654,7 @@ integration_tests = [
   't5410-receive-pack.sh',
   't5411-proc-receive-hook.sh',
   't5412-receive-report-hook.sh',
+  't5413-connectivity-check.sh',
   't5500-fetch-pack.sh',
   't5501-fetch-push-alternates.sh',
   't5502-quickfetch.sh',

index 3fabc82a87..ed195089ef 100755
@@ -1,9 +1,9 @@
 #!/bin/sh
 
-test_description='performance of connectivity check
+test_description='performance of connectivity check modes
 
-Benchmark the rev-list connectivity check, avoiding pack transfer
-noise.
+Compare the default and incremental rev-list connectivity modes
+directly, avoiding pack transfer noise.
 
 Each repository has a flat tree of many directories with 100 files
 in each.  Three axes are scaled independently: tree size, commit

@@ -51,12 +51,19 @@ test_perf_conn () {
 		fi
 	'
 
-	test_perf "$label" '
+	test_perf "$label (full)" '
 		cat '"${name}"'_new |
 		git -C '"$name"' rev-list \
 			--objects --stdin --not --all --quiet \
 			--exclude-promisor-objects
 	'
+
+	test_perf "$label (incremental)" '
+		cat '"${name}"'_new |
+		git -C '"$name"' rev-list --verify-trees-incremental \
+			--objects --stdin --not --all --quiet \
+			--exclude-promisor-objects
+	'
 }
 
 # Scaling tree size (10 commits, 10 files/commit).

new file mode 100644
index 0000000000..6aadadff70
@@ -0,0 +1,15 @@
+#ifndef TREE_VERIFY_H
+#define TREE_VERIFY_H
+
+struct commit_list;
+struct repository;
+
+/*
+ * Verify trees of commits incrementally against their parents.
+ * Dies on verification failure.
+ */
+void verify_commits_incremental(struct repository *repo,
+				struct commit_list **commits,
+				int exclude_promisor_objects);
+
+#endif /* TREE_VERIFY_H */

gitgitgadget

[snip]

Okay, so this is getting better the larger the trees.

And worse the more commits we have. So the new mode is only really applicable in scenarios where you know what your typical workload looks like. The big question is of course whether we can have the best of both worlds, where Git knows to switch between the two modes once it crosses the threshold.

I think this is where it gets tricky to reason about. The win from from larger trees pays off without bound -- the larger trees you have, the larger the win.

Scaling with commits on the other hand we have ceiling of how much it can regress. I am thinking that could be reasonable that anyone who enables the config flag for this mode could simply accept that it would likely speed things up in most cases but in other cases they could see a ~1.5x regression.

I am hoping that this tradeoff would then be acceptable, otherwise we would indeed need to add some way of switching, though I don't think there's a single good indicator value for when it's better to switch.

I could add a new config flag like transfer.connectivityCheckIncrementalMaxCommits to control it perhaps.

Yes, but which N? I think it would depend on the shape of the repository too -- size of object tree, merge-heavy vs linear.

I tried that first, but the natural size of enum is larger than unsigned char, so this reduces the memory usage.

Sure, I can change that. Or intmax_t since we'll need to cast to that later anyway for the trace2 output?

I think this would be a bigger refactor in general -- the existing connectivity check also dies as the way to report problems. This works since it's running as a subprocess.

If this is something we want to change, I think it would be good to do that refactoring separately and make the connectivity check subprocess only die() at the very edge, and making all of the internal library code support not dying. My first prototype tried to do this but the amount of changes to internal code got out of hand, so this version was the simplest I could make it.

But I'm happy to take a step back and revisit that approach if this is a blocker.

Hmm yes, perhaps. I will need to dig into that more.

Technically you could use any base, as long as it's trusted. That said, the trees here _do_ come from parent commits (not necessarily direct from the parent commit, it goes deeper down into the object tree).

I could rename it, though I think it would leak semantics that verify_tree() does not require.

True, will fix. I think I got confused because the code base use both styles in different places.

Yes, this is the trickiest part of the code. I tried to add comments here in a local draft but it became messy and did not help enough. I will make a new attempt at it though.

Hm, initially I was thinking it would get very messy with a manual stack or queue here, but maybe it would not be so bad. I will investigate if I can rework it nicely, but otherwise I think this version is reasonable, and aligns with other tree-walking code.

I think it would be unreachable, since we first collect the commits via the regular boundary walk. But now I started to doubt myself, I will try to re-validate this with a test that puts the boundary exactly at such a problematic commit.

Thanks for the detailed review!
Kristofer

[snip]

Yeah, making this configurable may be reasonable. The way I understand it, the new connectivity check is most likely to be usable for smaller, incremental pushes. And based on the above table you have, we see that up to 1k commits the new algorithm is faster. Sure, it's an artificial benchmark, but maybe this is a good enough cutoff for now?

[snip]

Hm, okay. Not a fan of using signed integers as counters, but so be it.

I mostly aim at not making _new_ library code die anymore. In any case, I don't think it's a blocker, but I nowadays consider this to be a code smell.

It would be great if we eventually deny-listed die() completely for anything compiled into libgit.a.

Patrick

I looked into this a bit more and I have two main notes:

First of all, I benchmarked multiple repos, both with small object trees (such as git.git), medium (linux.git) and some really big ones (proprietary).

    repo          files   commits   full    incr.  full/incr
    internal A    552K       1000   1.69s   0.66s    2.6x
    internal B    331K       1000   1.48s   0.57s    2.6x
    linux          95K        609   0.34s   0.12s    2.8x
    linux          95K      13965   1.86s   1.90s    1.0x
    git.git        4.9K       356   0.09s   0.13s    0.7x
    git.git        4.9K       976   0.23s   0.31s    0.7x

So for large and medium repos the cutoff of 1000 is conservative. Incremental mode is faster but the full mode is also good enough here.

For small repos like git, the incremental mode is slightly slower, but since the number of files is so small, both modes are still fast enough that it does not matter.

(I could not get an exact 1000 commit test synthetically by looking back in the history for all repos, since it depends a bit on the shape of the graph of commits.)

Thus, I think having a cutoff of 1000 is indeed a valid strategy here.

Second of all (and with the previous in mind), I think I would want to change the configuration into three modes:

- full (default): work as it does today.
- incremental: only use the incremental mode
- auto: decide on full or incremental depending
number of commits to process.

This would make things safe to roll out and start gaining more real life confidence before potentially changing the default to auto instead.

We could also make the cutoff configurable, but I am not sure that is needed in practice.

No worries, I will switch to size_t instead.

I have spent some more time thinking about this and I have changed my mind -- I think I was too stuck in the idea of all-or-nothing here, but I agree I can make this work in a mixed way -- not introducing any new die() but accept the cases where it already dies.

I will include this in the reroll (likely next week, don't want to rush things).

Thanks,
Kristofer

Yeah, thanks for evaluating this. Heuristics are of course going to go off rails at times, but I agree that 1000 seems like a good enough first iteration.

Hmm. As the heuristic can easily go wrong I think it would be a good idea to make it configurable. So I'd propose auto[=<cutoff>]. If left out we default to 1000, otherwise we pick whatever the user has specified.

[snip]

Thanks, I appreciate it when folks take their time :)

Patrick

"Kristofer Karlsson via GitGitGadget" <[email protected]>

This is exciting.

Hmm, ... OK.

OK.

I think so too, especially since I think it simplifies the follow-up where the scaling with number of refs can also be addressed and I hope to write up some more concrete ideas for that in the not too distant future.

This is where I would need feedback from you and the mailing list. I have been reasoning about this internally and decided to simply submit the smallest approach as a starting point.

Now with the config flag in place to gate usage as well as having the worst-case regression bounded in how bad it can be (2x more time, not worse in memory) I was thinking it may not need further protection. I was also not sure how common it would be to get pushes with so many commits that it becomes an issue, and that part is fundamentally hard to know for git as a general tool.

That said, I'd be happy to introduce some extra safety net here, perhaps add a fallback if there are more than N commits to verify. (Hardcoded based on synthetic benchmarks? Configurable?)

One partly annoying thing here may be that if we add fallback for the bad case, it forces us to keep both code paths and thus lose the potential for removing code. My dream scenario would be if there was a single implementation of the connectivity check that performs well enough in all scenarios, but maybe that will come anyway eventually and I should not speculate too much.

What would be your preferred approach here?

Thanks,
Kristofer

This series adds an incremental mode for the connectivity check, gated behind transfer.connectivityCheck=incremental or "auto" (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 option --connectivity-check=. 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 messages (5/6 and 6/6). 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 of up to about 18x in the synthetic perf tests.

Running against a large real-world repo (3.4 GB boundary closure), the numbers are more dramatic. These were measured on v1 and not re-run since; the real-repo numbers for v4 are in 6/6. 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.

In the cases I have tested the regression has remained modest, and it shows up with long incoming histories, while the target case (small pushes to large repositories) sees the largest speedup. That is what the "auto" mode is for: it uses the incremental check for small numbers of incoming commits and the full check otherwise, with a cutoff that can be configured.

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 t5413-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. The tests cover:

* Corruption detection: missing blobs, missing trees, type mismatches, malformed trees (unparseable, mid-tree corruption), and corrupt blobs (incremental only, since the full check does not read blobs)
* 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, missing promised parent commits, verification of local commits
* Replacement objects (with and without GIT_NO_REPLACE_OBJECTS)
* Shallow boundaries
* Deepening fetches (falls back to full check)
* Algorithm selection: clones and pushes into an empty repository use the full check, "auto" and its cutoff, and unknown config values
* Tree depth: the incremental check honors core.maxTreeDepth (t6700)
* 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, a die() in existing helpers only ends the subprocess, 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 doing the check in-process, 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.

Changes since v3
================

* Split, along the lines Patrick suggested, into three steps: the config option and the internal --connectivity-check= rev-list option with only "full" (4/6), the incremental check (5/6), and the "auto" mode (6/6), since choosing between the two is subtle enough to review on its own.

* The internal rev-list option --verify-trees-incremental is replaced by --connectivity-check=, which check_connected() now always passes. rev-list dies on an unknown mode, since only check_connected() passes it.

* An unknown transfer.connectivityCheck value now falls back to full with a warning instead of dying, so config written for a newer Git does not break fetches.

* "auto" takes an optional cutoff, "auto=", as Patrick suggested, defaulting to 1000.

* Clones and pushes into an empty repository use the full check even with incremental or auto, since there is no boundary to compare against (see 5/6). check_connected() now decides the deepening fallback before reading the config.

* New patch 3/6 drops the dependency on the_repository in connected.c, as Junio suggested. The repository is passed to check_connected() as a parameter rather than in check_connected_options, like other functions that take a repository.

* Use parse_commit_gently() instead of dying on a parent that cannot be parsed, which Patrick pointed out could happen in a partial clone with missing commits. As in the revision walk, a missing parent that is promised is skipped; added a test for that. More generally, the verifier adds no new die() calls: it returns errors, and rev-list dies once when the check fails.

* An object that cannot be read is now reported as "missing" only when it does not exist, and as "unable to read" otherwise, e.g. for a corrupt blob.

* Renamed variables to parent_trees / trusted_bases to make the trust direction clearer.

* size_t for counts and sizes.

* verify_tree() still recurses, bounded by core.maxTreeDepth like the other tree walkers. Its frame was made smaller by moving the tree descriptor scratch space into the shared state: with gcc -O2 its frame is 272 bytes, against 240 for process_tree(), so at the default depth of 2048 it needs about the same stack as "rev-list --objects". The listing of untrusted entries and the scan of the base trees are split out into helpers, without changing the logic. New t6700 tests check that the incremental check (given a boundary, so that it is not replaced by the full check) honors core.maxTreeDepth and fails gracefully at the default limit.

I tried a work queue as suggested, but it got messy, and it would be stricter than the code we already rely on: the full check's process_tree() recurses over trees in the same way, and be20128bfa (add core.maxTreeDepth) settled on bounding tree recursion rather than removing it. If we want an iterative walk, I think it belongs in a separate series that also covers process_tree().

* Also addressed from v3 review: the technical doc is listed in the Makefile and in meson.build, the perf tests require PERL, the leftover p5412 is now p5413, "verified against these marks" is reworded, int flags are bool, the tree state map's byte-sized values are explained in a comment, and the parent tree expansion in tree-verify.c has comments explaining it.

* Documentation: reworded the technical doc, including a shorter description of the promisor fast path; transfer.connectivityCheck documents the fallbacks to full; and the rev-list option description is trimmed to an internal-use note.

* The --connectivity-check parsing in rev-list moved into a small helper, with the mode and cutoff in a struct local to cmd_rev_list() instead of globals.

* Tests are faster and simpler: the auto cutoff is tested with "auto=" instead of building long histories, and the mode selection tests share two small helpers.

* p5413 passes the mode explicitly, and the tables in 5/6 were re-measured; they now report the best of 3 runs, which is what the perf framework reports, rather than "median of 3".

Changes since v2
================

Addresses Patrick's review (thanks!).

* Split from 2 commits to 4: documentation, perf test baseline, config plumbing + rev-list flag, then the incremental implementation.
* Documentation rewrites: added unreachable-objects caveat, removed code references to reduce staleness risk, replaced three-phase list with cost-focused prose, rewrote trust model to explicitly describe boundary mechanism and inductive argument, and several smaller clarifications.
* Extracted tree_oid variable in verify_commit_tree() to avoid calling get_commit_tree_oid(commit) twice.
* Rebased onto latest master to resolve a meson.build conflict (t5412 was taken by another topic). Renumbered test and perf files to t5413/p5413.

Changes since v1
================

* Guard against get_commit_tree_oid() returning NULL in verify_commit_tree(), dying with a clear message instead of a NULL dereference. (Thanks Junio for catching this)
* Set fetch_if_missing = 0 in the rev-list early option parse when --verify-trees-incremental is seen, and add a BUG() assertion in verify_commits_incremental() to catch any future caller that forgets. This makes the lazy-fetch protection explicit rather than relying solely on --exclude-promisor-objects having set it earlier. (Thanks again to Junio)
* Add a test that verifies promised blobs are not lazy-fetched during the incremental check.
* Reuse the existing 3-argument type-mismatch format string to avoid adding new strings for localization.

Thanks, Kristofer

Kristofer Karlsson (6):
Documentation: describe connectivity checking
p5413: add perf tests for connectivity check
connected: drop dependency on the_repository
connected: add transfer.connectivityCheck config
connected: add incremental connectivity check via rev-list
connected: add "auto" connectivity check mode

 Documentation/Makefile                        |   1 +
 Documentation/config/transfer.adoc            |  30 +
 Documentation/rev-list-options.adoc           |   5 +
 .../technical/connectivity-check.adoc         | 259 ++++++
 Documentation/technical/meson.build           |   1 +
 Makefile                                      |   1 +
 builtin/clone.c                               |   2 +-
 builtin/fetch.c                               |   5 +-
 builtin/receive-pack.c                        |  12 +-
 builtin/rev-list.c                            |  77 ++
 bundle.c                                      |   2 +-
 connected.c                                   |  67 +-
 connected.h                                   |   4 +-
 fetch-pack.c                                  |   3 +-
 meson.build                                   |   1 +
 t/meson.build                                 |   1 +
 ...enerate-repo-p5413-connectivity-check.perl |  44 +
 t/perf/p5413-connectivity-check.sh            |  92 ++
 t/t5413-connectivity-check.sh                 | 795 ++++++++++++++++++
 t/t6700-tree-depth.sh                         |  17 +
 tree-verify.c                                 | 427 ++++++++++
 tree-verify.h                                 |  15 +
 22 files changed, 1840 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/technical/connectivity-check.adoc
 create mode 100644 t/perf/generate-repo-p5413-connectivity-check.perl
 create mode 100755 t/perf/p5413-connectivity-check.sh
 create mode 100755 t/t5413-connectivity-check.sh
 create mode 100644 tree-verify.c
 create mode 100644 tree-verify.h

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2211%2Fspkrka%2Ftree-diff-connectivity-v1-clean-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2211/spkrka/tree-diff-connectivity-v1-clean-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2211

Range-diff vs v3:

1: 19ea8cb534 ! 1: e25824c21d Documentation: describe connectivity checking
@@ Commit message

+ ## Documentation/Makefile ##
+@@ Documentation/Makefile: TECH_DOCS += technical/bitmap-format
+ TECH_DOCS += technical/build-systems
+ TECH_DOCS += technical/bundle-uri
+ TECH_DOCS += technical/commit-graph
++TECH_DOCS += technical/connectivity-check
+ TECH_DOCS += technical/directory-rename-detection
+ TECH_DOCS += technical/hash-function-transition
+ TECH_DOCS += technical/large-object-promisors
+
## Documentation/technical/connectivity-check.adoc (new) ##
@@
+Connectivity checking
@@ Documentation/technical/connectivity-check.adoc (new)
+ git rev-list --objects --stdin --not --all --quiet
+ --alternate-refs [--exclude-promisor-objects]
+
-+When promisor remotes are configured, a fast path based on -+promisor packfiles is attempted first. If it falls back to -+the rev-list check, --exclude-promisor-objects is added so -+that the traversal does not trigger on-demand fetches. ++In a partial clone, objects received from a promisor remote do ++not need to be checked: anything they reference is promised by ++that remote and can be fetched on demand. So if every new tip is ++found in a promisor packfile, rev-list is not run at all. ++Otherwise it runs with --exclude-promisor-objects, so that the ++traversal stops at promisor objects instead of fetching them.
+
+Consider the following graph after a fetch, where all reference +tips point directly to commits. For simplicity, only local
@@ Documentation/technical/connectivity-check.adoc (new)
+
+The traversal then walks the trees of the boundary commits and +marks those objects uninteresting. Trees of incoming commits
-+are verified against these marks, stopping at objects already -+covered. The cost of marking boundary trees uninteresting ++are then walked, stopping at objects already marked as ++uninteresting. The cost of marking boundary trees uninteresting
+dominates when the boundary commits have large trees, even when +the incoming changes are small.
+
@@ Documentation/technical/connectivity-check.adoc (new)
+These non-commit tips are verified during the tree and blob +traversal that follows the commit walk within the same rev-list +invocation.
+
+ ## Documentation/technical/meson.build ##
+@@ Documentation/technical/meson.build: articles = [
+ 'build-systems.adoc',
+ 'bundle-uri.adoc',
+ 'commit-graph.adoc',
++ 'connectivity-check.adoc',
+ 'directory-rename-detection.adoc',
+ 'hash-function-transition.adoc',
+ 'large-object-promisors.adoc',
2: 211794c925 ! 2: 427b7315a5 p5413: add perf tests for connectivity check

     @@ t/perf/generate-repo-p5413-connectivity-check.perl (new)
      @@
      +#!/usr/bin/perl
      +#
     -+# Generate a fast-import stream for p5412 connectivity check benchmarks.
     ++# Generate a fast-import stream for p5413 connectivity check benchmarks.
      +#
     -+# Usage: generate-repo-p5412-connectivity-check.perl
     ++# Usage: generate-repo-p5413-connectivity-check.perl
      +#            <dirs> <files_per_dir> <commits> [<hot_dirs>] [<files_per_commit>]
      +#
      +# Creates one initial commit with dirs*files_per_dir files, then
     @@ t/perf/p5413-connectivity-check.sh (new)
      +		label="$label (${fpc} files/commit)"
      +	fi
      +
     -+	test_expect_success "setup $label" '
     ++	test_expect_success PERL "setup $label" '
      +		if test -d '"$name"'
      +		then
      +			true
     @@ t/perf/p5413-connectivity-check.sh (new)
      +		fi
      +	'
      +
     -+	test_perf "$label" '
     ++	test_perf "$label" --prereq PERL '
      +		cat '"${name}"'_new |
      +		git -C '"$name"' rev-list \
      +			--objects --stdin --not --all --quiet \
 -:  ---------- > 3:  3a7a1fb953 connected: drop dependency on `the_repository`
 3:  cdf28f1df6 ! 4:  87dbb1f055 connected: add transfer.connectivityCheck config and rev-list flag
     @@ Metadata
      Author: Kristofer Karlsson <[email protected]>
      
       ## Commit message ##
     -    connected: add transfer.connectivityCheck config and rev-list flag
     +    connected: add transfer.connectivityCheck config
      
     -    Add the transfer.connectivityCheck configuration option, accepting
     -    "full" (default) and "incremental".  When set to "incremental",
     -    check_connected() passes --verify-trees-incremental to its rev-list
     -    subprocess.
     +    Add the transfer.connectivityCheck configuration option to choose
     +    the algorithm used for the connectivity check.  For now the only
     +    accepted value is "full", which is the existing behavior and the
     +    default.  Unknown values fall back to "full" with a warning, so
     +    that configuration written for a newer version of Git does not
     +    break fetches and pushes; "full" is always a correct check.
      
     -    The rev-list flag is accepted but has no effect yet; the incremental
     -    verification will be implemented in the next commit.
     +    check_connected() now always tells its rev-list subprocess which
     +    algorithm to use with an internal --connectivity-check=<mode>
     +    option, which so far only accepts "full".  Passing the mode
     +    explicitly means rev-list knows it is running a connectivity
     +    check, so later optimizations can be limited to that case.
      
     +    This prepares for an incremental check added in the next commit.
     +
     +    Suggested-by: Patrick Steinhardt <[email protected]>
          Signed-off-by: Kristofer Karlsson <[email protected]>
      
       ## Documentation/config/transfer.adoc ##
     @@ Documentation/config/transfer.adoc
      +--
      +`full` (default);;
      +	Walk the full object closure of the boundary commits.
     -+`incremental`;;
     -+	Verify incoming commits by diffing their trees against parent
     -+	trees, recursively descending only into entries that differ.
     -+	The largest benefits occur when incoming commits change a
     -+	small fraction of a large tree closure.
     -+	Falls back to `full` for deepening fetches.
      +--
     +++
     ++Unknown values are treated as `full`, with a warning.
      +
       transfer.credentialsInUrl::
       	A configured URL can contain plaintext credentials in the form
     @@ Documentation/rev-list-options.adoc: It is an error to use `--missing-only` with
       	traversal, rather than just silencing errors about missing
       	objects.
      +
     -+`--verify-trees-incremental`::
     -+	(For internal use only.)  Verify tree connectivity
     -+	incrementally by comparing each commit's tree against its
     -+	parent trees.  Used by `check_connected()` when
     -+	`transfer.connectivityCheck` is set to `incremental`.
     ++`--connectivity-check=full`::
     ++	(For internal use only.)  Used by `check_connected()` to
     ++	select the connectivity check algorithm; see
     ++	`transfer.connectivityCheck` in linkgit:git-config[1].
       endif::git-rev-list[]
       
       `--no-walk[=(sorted|unsorted)]`::
      
       ## builtin/rev-list.c ##
     +@@
     + #include "quote.h"
     + #include "strbuf.h"
     + 
     ++enum connectivity_check_mode {
     ++	CONNECTIVITY_CHECK_FULL,
     ++};
     ++
     ++struct connectivity_check {
     ++	enum connectivity_check_mode mode;
     ++};
     ++
     ++static void parse_connectivity_check(struct repository *repo UNUSED,
     ++				     const char *arg,
     ++				     struct connectivity_check *check)
     ++{
     ++	if (!strcmp(arg, "full"))
     ++		check->mode = CONNECTIVITY_CHECK_FULL;
     ++	else
     ++		die(_("unknown --connectivity-check mode '%s'"), arg);
     ++}
     ++
     + struct rev_list_info {
     + 	struct rev_info *revs;
     + 	int flags;
     +@@ builtin/rev-list.c: int cmd_rev_list(int argc,
     + 	int bisect_find_all = 0;
     + 	int use_bitmap_index = 0;
     + 	int filter_provided_objects = 0;
     ++	struct connectivity_check connectivity_check = { 0 };
     + 	const char *show_progress = NULL;
     + 	int ret = 0;
     + 
      @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       		if (!strcmp(arg, "--exclude-promisor-objects")) {
       			repo->fetch_if_missing = 0;
       			revs.exclude_promisor_objects = 1;
     -+		} else if (!strcmp(arg, "--verify-trees-incremental")) {
     -+			repo->fetch_if_missing = 0;
     ++		} else if (skip_prefix(arg, "--connectivity-check=", &arg)) {
     ++			parse_connectivity_check(repo, arg, &connectivity_check);
       		} else if (skip_prefix(arg, "--missing=", &arg)) {
       			parse_missing_action_value(repo, arg);
       		} else if (!strcmp(arg, "--missing-only")) {
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       
       		if (!strcmp(arg, "--exclude-promisor-objects"))
       			continue; /* already handled above */
     -+		if (!strcmp(arg, "--verify-trees-incremental"))
     ++		if (starts_with(arg, "--connectivity-check="))
      +			continue; /* already handled above */
       		if (skip_prefix(arg, "--missing=", &arg))
       			continue; /* already handled above */
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
      
       ## connected.c ##
      @@
     - #define USE_THE_REPOSITORY_VARIABLE
     - 
       #include "git-compat-util.h"
      +#include "config.h"
       #include "gettext.h"
       #include "hex.h"
       #include "odb.h"
     -@@ connected.c: static int check_connected_promisor(oid_iterate_fn fn,
     +@@ connected.c: static int check_connected_promisor(struct repository *repo,
       	return 1;
       }
       
     -+static int incremental_check_applicable(struct check_connected_options *opt)
     ++/*
     ++ * Fills in the --connectivity-check mode to pass to rev-list.
     ++ */
     ++static void connectivity_check_mode(struct repository *repo,
     ++				    struct strbuf *mode)
      +{
      +	const char *algorithm = NULL;
      +
     -+	if (repo_config_get_string_tmp(the_repository,
     ++	if (repo_config_get_string_tmp(repo,
      +				       "transfer.connectivitycheck",
      +				       &algorithm))
     -+		return 0;
     ++		algorithm = "full";
      +	if (!strcasecmp(algorithm, "full"))
     -+		return 0;
     -+	if (strcasecmp(algorithm, "incremental"))
     -+		die(_("unknown transfer.connectivityCheck algorithm '%s'"),
     -+		    algorithm);
     -+
     -+	if (opt->is_deepening_fetch)
     -+		return 0;
     -+
     -+	return 1;
     ++		strbuf_addstr(mode, "full");
     ++	else {
     ++		warning(_("unknown transfer.connectivityCheck algorithm '%s', "
     ++			  "using 'full'"), algorithm);
     ++		strbuf_addstr(mode, "full");
     ++	}
      +}
      +
       /*
        * If we feed all the commits we want to verify to this command
        *
     -@@ connected.c: int check_connected(oid_iterate_fn fn, void *cb_data,
     +@@ connected.c: int check_connected(struct repository *repo,
     + 	struct child_process rev_list = CHILD_PROCESS_INIT;
     + 	FILE *rev_list_in;
     + 	struct check_connected_options defaults = CHECK_CONNECTED_INIT;
     ++	struct strbuf mode = STRBUF_INIT;
     + 	const struct object_id *oid;
     + 	int err = 0;
     + 	struct packed_git *new_pack = NULL;
     +@@ connected.c: int check_connected(struct repository *repo,
       	if (opt->progress)
       		strvec_pushf(&rev_list.args, "--progress=%s",
       			     _("Checking connectivity"));
     -+	if (incremental_check_applicable(opt))
     -+		strvec_push(&rev_list.args,
     -+			    "--verify-trees-incremental");
     ++	connectivity_check_mode(repo, &mode);
     ++	strvec_pushf(&rev_list.args, "--connectivity-check=%s", mode.buf);
     ++	strbuf_release(&mode);
       
       	rev_list.git_cmd = 1;
       	if (opt->env)
 4:  efaada0b6a ! 5:  d76dae801b connected: add incremental connectivity check via rev-list
     @@ Commit message
          See Documentation/technical/connectivity-check.adoc for the trust
          invariants and detailed algorithm.
      
     -    The incremental check runs as an internal
     -    --verify-trees-incremental mode in rev-list.  After the normal
     +    The incremental check runs inside rev-list, selected by the
     +    internal --connectivity-check option.  After the normal
          revision walk identifies the incoming commits, the verifier
          consumes them before the usual object traversal; any pending
          non-commit tips are still handled by the existing traversal.
      
          Partial-clone semantics are preserved: objects promised by a
          promisor remote are accepted as connected, and on-demand fetching
     -    is prevented by excluding promisor objects from traversal.  The
     -    new mode is selected by transfer.connectivityCheck=incremental,
     -    with full remaining the default.  Deepening fetches fall back to
     -    the full check because they do not have the normal
     +    is prevented by excluding promisor objects from traversal.
     +
     +    A new transfer.connectivityCheck value "incremental" selects the
     +    new mode, with "full" remaining the default.  Deepening fetches
     +    fall back to the full check because they do not have the normal
          existing-reference boundary, and traversal instead follows the
          effective shallow boundary.
      
     -    p5413 results (median of 3), scaling one dimension at a time.
     +    Clones and pushes into an empty repository also use the full
     +    check.  Without negative revisions there is no trusted boundary,
     +    and the incremental check would have to verify all of history
     +    from scratch like the full check does, with extra per-commit
     +    overhead on top.  On git.git, checking 70K commits without a
     +    boundary takes 5.3s with the full check and 9.2s with the
     +    incremental one.
     +
     +    p5413 results (best of 3), scaling one dimension at a time.
          Each fixture is a repository with a flat tree of many
          directories containing 100 files each.  The incoming set is a
          chain of commits on top of the existing history, with each
     @@ Commit message
              files    full  incr.  full/incr
                5K    0.01s  0.01s    1.0x
               50K    0.04s  0.01s    4.0x
     -        200K    0.15s  0.01s   15.0x
     -        800K    0.61s  0.03s   20.3x
     +        200K    0.13s  0.01s   13.0x
     +        800K    0.54s  0.03s   18.0x
      
          The full mode must walk the trees of boundary commits, which
          grows with overall tree size.  Incremental still scans the
     @@ Commit message
          Scaling incoming commit count (200K files, 10 files/commit):
      
              commits    full  incr.  full/incr
     -              1    0.14s  0.01s   14.0x
     -             10    0.15s  0.01s   15.0x
     -            100    0.19s  0.05s    3.8x
     -            500    0.32s  0.27s    1.2x
     -           3000    1.23s  1.52s    0.8x
     -           5000    1.88s  2.43s    0.8x
     -          10000    3.72s  5.36s    0.7x
     +              1    0.13s  0.01s   13.0x
     +             10    0.13s  0.01s   13.0x
     +            100    0.16s  0.06s    2.7x
     +            500    0.29s  0.25s    1.2x
     +           3000    1.14s  1.44s    0.8x
     +           5000    1.87s  2.39s    0.8x
     +          10000    3.45s  5.13s    0.7x
      
          With many commits the per-commit overhead of scanning both
          the new and parent root trees accumulates and incremental
     @@ Commit message
          Scaling files per incoming commit (200K files, 10 commits):
      
              files/commit    full  incr.  full/incr
     -                   1    0.14s  0.01s   14.0x
     -                  10    0.15s  0.01s   15.0x
     -                 100    0.16s  0.04s    4.0x
     -                 500    0.23s  0.14s    1.6x
     -                1000    0.34s  0.28s    1.2x
     -                2000    0.70s  0.63s    1.1x
     -
     -    Breakeven is around 1000 files/commit.  At 2000 files/commit
     -    (every directory touched), incremental remains slightly faster;
     -    bypassing the object cache for tree reads likely helps here.
     +                   1    0.13s  0.01s   13.0x
     +                  10    0.13s  0.01s   13.0x
     +                 100    0.15s  0.03s    5.0x
     +                 500    0.23s  0.13s    1.8x
     +                1000    0.34s  0.27s    1.3x
     +                2000    0.65s  0.56s    1.2x
      
     -    For small repositories both modes are fast enough that the
     -    difference is difficult to measure reliably.
     +    The advantage shrinks as more files change per commit, but
     +    incremental stays slightly faster even at 2000 files/commit,
     +    where every directory is touched.
      
          Selected peak RSS measurements from the same fixtures:
      
     @@ Commit message
              800K files, 10 commits     110 MB    26 MB   0.2x
              200K files, 5000 commits   155 MB   151 MB   1.0x
      
     -    Incremental uses substantially less memory when it can prune
     -    most of the boundary tree walk.  In the long-history case the
     -    two modes visit similar object sets and memory converges.
     +    Incremental uses less memory when it can prune enough of the
     +    boundary tree walk.  In the long-history case the two modes
     +    visit similar object sets and memory converges.
      
          The regression cases in the CPU benchmarks are in wall-clock
          time rather than memory, primarily from scanning some trees
     -    more than once.
     +    more than once: a commit's tree is read as a candidate when the
     +    commit is verified, and again as a trusted parent tree when its
     +    children are verified.
      
          Signed-off-by: Kristofer Karlsson <[email protected]>
      
     + ## Documentation/config/transfer.adoc ##
     +@@ Documentation/config/transfer.adoc: transfer.connectivityCheck::
     + --
     + `full` (default);;
     + 	Walk the full object closure of the boundary commits.
     ++`incremental`;;
     ++	Verify incoming commits by diffing their trees against parent
     ++	trees, recursively descending only into entries that differ.
     ++	The largest benefits occur when incoming commits change a
     ++	small fraction of a large tree closure, and it can be slower
     ++	than `full` when there are many incoming commits.
     ++	Falls back to `full` for clones, pushes into an empty
     ++	repository and deepening fetches.
     + --
     + +
     + Unknown values are treated as `full`, with a warning.
     +
     + ## Documentation/rev-list-options.adoc ##
     +@@ Documentation/rev-list-options.adoc: It is an error to use `--missing-only` without `--missing=print` or
     + 	traversal, rather than just silencing errors about missing
     + 	objects.
     + 
     +-`--connectivity-check=full`::
     ++`--connectivity-check=(full|incremental)`::
     + 	(For internal use only.)  Used by `check_connected()` to
     + 	select the connectivity check algorithm; see
     + 	`transfer.connectivityCheck` in linkgit:git-config[1].
     +
       ## Documentation/technical/connectivity-check.adoc ##
      @@ Documentation/technical/connectivity-check.adoc: tag, tree, or blob, that object is not part of the commit walk.
       These non-commit tips are verified during the tree and blob
     @@ Documentation/technical/connectivity-check.adoc: tag, tree, or blob, that object
      +replaces what happens afterwards: rather than walking boundary
      +trees to mark objects uninteresting, it compares each incoming
      +commit's tree against its parents' trees, processing commits in
     -+topological order (ancestors first).
     ++topological order (ancestors first).  This all happens inside
     ++the same `rev-list` invocation, and the promisor fast path above
     ++still runs first.
      +
      +Trust model
      +~~~~~~~~~~~
     @@ builtin/rev-list.c
       #include "strbuf.h"
      +#include "tree-verify.h"
       
     - struct rev_list_info {
     - 	struct rev_info *revs;
     -@@ builtin/rev-list.c: int cmd_rev_list(int argc,
     - 	int bisect_find_all = 0;
     - 	int use_bitmap_index = 0;
     - 	int filter_provided_objects = 0;
     -+	int verify_trees_incremental = 0;
     - 	const char *show_progress = NULL;
     - 	int ret = 0;
     + enum connectivity_check_mode {
     + 	CONNECTIVITY_CHECK_FULL,
     ++	CONNECTIVITY_CHECK_INCREMENTAL,
     + };
       
     -@@ builtin/rev-list.c: int cmd_rev_list(int argc,
     - 			repo->fetch_if_missing = 0;
     - 			revs.exclude_promisor_objects = 1;
     - 		} else if (!strcmp(arg, "--verify-trees-incremental")) {
     -+			verify_trees_incremental = 1;
     - 			repo->fetch_if_missing = 0;
     - 		} else if (skip_prefix(arg, "--missing=", &arg)) {
     - 			parse_missing_action_value(repo, arg);
     + struct connectivity_check {
     + 	enum connectivity_check_mode mode;
     + };
     + 
     +-static void parse_connectivity_check(struct repository *repo UNUSED,
     ++static void parse_connectivity_check(struct repository *repo,
     + 				     const char *arg,
     + 				     struct connectivity_check *check)
     + {
     + 	if (!strcmp(arg, "full"))
     + 		check->mode = CONNECTIVITY_CHECK_FULL;
     ++	else if (!strcmp(arg, "incremental"))
     ++		check->mode = CONNECTIVITY_CHECK_INCREMENTAL;
     + 	else
     + 		die(_("unknown --connectivity-check mode '%s'"), arg);
     ++
     ++	if (check->mode != CONNECTIVITY_CHECK_FULL)
     ++		repo->fetch_if_missing = 0;
     + }
     + 
     + struct rev_list_info {
      @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       
       	prepare_maximal_independent(&revs);
       
     -+	if (verify_trees_incremental) {
     ++	/*
     ++	 * Without a boundary of negative revisions, as in a clone or a
     ++	 * push into an empty repository, every incoming commit is new.
     ++	 * The incremental check would still work, but it would verify
     ++	 * the whole object graph like the full check, with extra
     ++	 * per-commit overhead on top, so use the full check.
     ++	 */
     ++	if (!revs.limited)
     ++		connectivity_check.mode = CONNECTIVITY_CHECK_FULL;
     ++
     ++	if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) {
      +		struct commit *commit;
      +		struct commit_list *new_commits = NULL;
      +
      +		while ((commit = get_revision(&revs)) != NULL)
      +			commit_list_insert(commit, &new_commits);
      +
     -+		verify_commits_incremental(repo, &new_commits,
     -+					   revs.exclude_promisor_objects);
     ++		if (verify_commits_incremental(repo, &new_commits,
     ++					       revs.exclude_promisor_objects))
     ++			die(_("connectivity check failed"));
      +		commit_list_free(new_commits);
      +	}
      +
     @@ builtin/rev-list.c: int cmd_rev_list(int argc,
       		mark_edges_uninteresting(&revs, show_edge, 0);
       
      
     + ## connected.c ##
     +@@ connected.c: static int check_connected_promisor(struct repository *repo,
     +  * Fills in the --connectivity-check mode to pass to rev-list.
     +  */
     + static void connectivity_check_mode(struct repository *repo,
     ++				    struct check_connected_options *opt,
     + 				    struct strbuf *mode)
     + {
     + 	const char *algorithm = NULL;
     + 
     ++	/*
     ++	 * Deepening fetches do not have the usual boundary of
     ++	 * existing references, so use the full check for them.
     ++	 */
     ++	if (opt->is_deepening_fetch) {
     ++		strbuf_addstr(mode, "full");
     ++		return;
     ++	}
     ++
     + 	if (repo_config_get_string_tmp(repo,
     + 				       "transfer.connectivitycheck",
     + 				       &algorithm))
     + 		algorithm = "full";
     + 	if (!strcasecmp(algorithm, "full"))
     + 		strbuf_addstr(mode, "full");
     ++	else if (!strcasecmp(algorithm, "incremental"))
     ++		strbuf_addstr(mode, "incremental");
     + 	else {
     + 		warning(_("unknown transfer.connectivityCheck algorithm '%s', "
     + 			  "using 'full'"), algorithm);
     +@@ connected.c: int check_connected(struct repository *repo,
     + 	if (opt->progress)
     + 		strvec_pushf(&rev_list.args, "--progress=%s",
     + 			     _("Checking connectivity"));
     +-	connectivity_check_mode(repo, &mode);
     ++	connectivity_check_mode(repo, opt, &mode);
     + 	strvec_pushf(&rev_list.args, "--connectivity-check=%s", mode.buf);
     + 	strbuf_release(&mode);
     + 
     +
       ## meson.build ##
      @@ meson.build: libgit_sources = [
         'transport-helper.c',
     @@ t/perf/p5413-connectivity-check.sh: test_perf_conn () {
       		fi
       	'
       
     --	test_perf "$label" '
     -+	test_perf "$label (full)" '
     +-	test_perf "$label" --prereq PERL '
     ++	test_perf "$label (full)" --prereq PERL '
       		cat '"${name}"'_new |
     - 		git -C '"$name"' rev-list \
     - 			--objects --stdin --not --all --quiet \
     - 			--exclude-promisor-objects
     - 	'
     -+
     -+	test_perf "$label (incremental)" '
     -+		cat '"${name}"'_new |
     -+		git -C '"$name"' rev-list --verify-trees-incremental \
     +-		git -C '"$name"' rev-list \
     ++		git -C '"$name"' rev-list --connectivity-check=full \
      +			--objects --stdin --not --all --quiet \
      +			--exclude-promisor-objects
      +	'
     - }
     - 
     - # Scaling tree size (10 commits, 10 files/commit).
     ++
     ++	test_perf "$label (incremental)" --prereq PERL '
     ++		cat '"${name}"'_new |
     ++		git -C '"$name"' rev-list --connectivity-check=incremental \
     + 			--objects --stdin --not --all --quiet \
     + 			--exclude-promisor-objects
     + 	'
      
       ## t/t5413-connectivity-check.sh (new) ##
      @@
     @@ t/t5413-connectivity-check.sh (new)
      +# Run a connectivity check via rev-list directly.  Uses $mode
      +# (set by the enclosing for-loop) to choose full or incremental.
      +check_connected () {
     -+	flags= &&
     -+	if test "$mode" = incremental
     -+	then
     -+		flags=--verify-trees-incremental
     -+	fi &&
      +	printf '%s\n' "$@" |
     -+	git rev-list $flags \
     ++	git rev-list --connectivity-check="$mode" \
      +		--objects --stdin --not --all --quiet \
      +		--exclude-promisor-objects
      +}
     @@ t/t5413-connectivity-check.sh (new)
      +		git update-ref -d refs/heads/main &&
      +		git update-ref -d HEAD &&
      +
     -+		echo "$original" >.git/test-oid
     ++		echo "$original" >.git/test-oid &&
     ++		echo "$orig_blob" >.git/test-blob
      +	)
      +'
      +
      +missing_oid=$(test_oid missing)
      +original_oid=$(cat replace-test/.git/test-oid)
     ++orig_blob_oid=$(cat replace-test/.git/test-blob)
      +
      +for mode in full incremental
      +do
      +
     ++# The full traversal reports a missing tree as "bad tree object".
     ++case "$mode" in
     ++full)
     ++	missing_tree="bad tree object"
     ++	;;
     ++incremental)
     ++	missing_tree="missing tree object"
     ++	;;
     ++esac
     ++
      +# Corruption detection: craft broken object graphs and verify detection.
      +# All tests use main-repo without modifying its refs or worktree.
      +
     @@ t/t5413-connectivity-check.sh (new)
      +			git mktree --missing) &&
      +		bad_commit=$(git commit-tree "$bad_tree" -p HEAD -m "bad") &&
      +		test_expect_code 128 check_connected "$bad_commit" 2>err &&
     -+		test_grep "missing blob object" err
     ++		test_grep "missing blob object.*$missing_oid" err
      +	)
      +'
      +
     @@ t/t5413-connectivity-check.sh (new)
      +			git mktree --missing) &&
      +		bad_commit=$(git commit-tree "$bad_tree" -p HEAD -m "bad") &&
      +		test_expect_code 128 check_connected "$bad_commit" 2>err &&
     -+		test_grep "bad tree object" err
     ++		test_grep "$missing_tree $missing_oid" err
      +	)
      +'
      +
     @@ t/t5413-connectivity-check.sh (new)
      +		bad_tree=$(printf "100644 blob ${missing_oid}\tfile.txt\n" |
      +			git mktree --missing) &&
      +		test_expect_code 128 check_connected "$bad_tree" 2>err &&
     -+		test_grep "missing blob object" err
     ++		test_grep "missing blob object.*$missing_oid" err
      +	)
      +'
      +
     @@ t/t5413-connectivity-check.sh (new)
      +		export GIT_NO_REPLACE_OBJECTS &&
      +		test_expect_code 128 check_connected \
      +			"$original_oid" 2>err &&
     -+		test_grep "missing blob object" err
     ++		test_grep "missing blob object.*$orig_blob_oid" err
      +	)
      +'
      +
     @@ t/t5413-connectivity-check.sh (new)
      +	)
      +'
      +
     ++test_expect_success "$mode: tolerates missing promisor parent commit" '
     ++	test_when_finished "rm -rf mp-src mp-server.git mp-client" &&
     ++	git init mp-src &&
     ++	test_commit -C mp-src --no-tag base file.txt &&
     ++	git clone --bare mp-src mp-server.git &&
     ++	git -C mp-server.git config uploadpack.allowfilter true &&
     ++	git -C mp-server.git config uploadpack.allowanysha1inwant true &&
     ++	git clone --filter=blob:none \
     ++		"file://$(pwd)/mp-server.git" mp-client &&
     ++	test_commit -C mp-src --no-tag c1 file.txt &&
     ++	test_commit -C mp-src --no-tag c2 file.txt &&
     ++	c1=$(git -C mp-src rev-parse HEAD~1) &&
     ++	c2=$(git -C mp-src rev-parse HEAD) &&
     ++	(
     ++		cd mp-client &&
     ++		# Add a promisor pack holding only c2, so its parent c1
     ++		# is missing but promised (as after a lazy fetch that
     ++		# omits dependents).
     ++		echo "$c2" |
     ++		git -C ../mp-src pack-objects --stdout >c2.pack &&
     ++		git index-pack --promisor --stdin <c2.pack &&
     ++		test_must_fail env GIT_NO_LAZY_FETCH=1 \
     ++			git cat-file -e "$c1" &&
     ++		tree=$(git rev-parse HEAD^{tree}) &&
     ++		local_commit=$(git hash-object -t commit -w --stdin <<-EOF
     ++		tree $tree
     ++		parent $c1
     ++		author A U Thor <[email protected]> 1112912053 -0700
     ++		committer C O Mitter <[email protected]> 1112912053 -0700
     ++
     ++		on top of a missing promisor commit
     ++		EOF
     ++		) &&
     ++		check_connected "$local_commit"
     ++	)
     ++'
     ++
      +test_expect_success "$mode: respects shallow boundary" '
      +	test_when_finished "rm -rf shallow-src shallow" &&
      +	git init shallow-src &&
     @@ t/t5413-connectivity-check.sh (new)
      +
      +done
      +
     ++# The full check only checks that blobs exist, but the incremental
     ++# check reads their headers and reports a corrupt blob.
     ++test_expect_success 'incremental: rejects corrupt blob' '
     ++	test_when_finished "rm -rf corrupt-repo" &&
     ++	git init corrupt-repo &&
     ++	(
     ++		cd corrupt-repo &&
     ++		test_commit base &&
     ++		blob=$(echo corrupt | git hash-object -w --stdin) &&
     ++		tree=$(printf "100644 blob $blob\tfile\n" | git mktree) &&
     ++		commit=$(git commit-tree "$tree" -p HEAD -m corrupt) &&
     ++		path=.git/objects/$(test_oid_to_path "$blob") &&
     ++		chmod +w "$path" &&
     ++		echo garbage >"$path" &&
     ++		mode=incremental &&
     ++		test_expect_code 128 check_connected "$commit" 2>err &&
     ++		test_grep "unable to read blob object $blob" err
     ++	)
     ++'
     ++
      +# Algorithm selection.
      +
     -+test_expect_success 'invalid transfer.connectivityCheck is rejected' '
     -+	test_when_finished "rm -rf invalid-cfg-src invalid-cfg-dst" &&
     -+	git init invalid-cfg-src &&
     -+	test_commit -C invalid-cfg-src --no-tag base file.txt &&
     -+	git clone invalid-cfg-src invalid-cfg-dst &&
     -+	test_commit -C invalid-cfg-src --no-tag update file.txt updated &&
     -+	git -C invalid-cfg-dst config transfer.connectivityCheck bogus &&
     -+	test_must_fail git -C invalid-cfg-dst fetch origin main 2>err &&
     -+	test_grep "unknown transfer.connectivityCheck" err
     ++# Run a command with its trace2 events in "trace" and its stderr
     ++# in "err".
     ++run_traced () {
     ++	rm -f trace &&
     ++	GIT_TRACE2_EVENT="$(pwd)/trace" "$@" 2>err
     ++}
     ++
     ++# Check which connectivity check the last run_traced used.
     ++expect_mode () {
     ++	case "$1" in
     ++	full)
     ++		test_grep ! trees_loaded trace
     ++		;;
     ++	incremental)
     ++		test_grep trees_loaded trace
     ++		;;
     ++	*)
     ++		BUG "unknown mode: $1"
     ++		;;
     ++	esac
     ++}
     ++
     ++test_expect_success 'setup source repository' '
     ++	git init src &&
     ++	test_commit -C src base file.txt &&
     ++	test_commit -C src --no-tag update file.txt updated
     ++'
     ++
     ++test_expect_success 'unknown transfer.connectivityCheck falls back to full' '
     ++	test_when_finished "rm -rf dst" &&
     ++	git init --bare dst &&
     ++	git -C dst fetch ../src base:refs/heads/main &&
     ++	for value in bogus
     ++	do
     ++		set_connectivity_check dst "$value" &&
     ++		run_traced git -C dst fetch ../src main &&
     ++		expect_mode full &&
     ++		test_grep "unknown transfer.connectivityCheck algorithm .$value., using .full." err ||
     ++		return 1
     ++	done
      +'
      +
      +test_expect_success 'push uses incremental when configured' '
     -+	test_when_finished "rm -rf int-src int-dst.git" &&
     -+	git init int-src &&
     -+	test_commit -C int-src --no-tag base file.txt &&
     -+	git clone --bare int-src int-dst.git &&
     -+	test_commit -C int-src --no-tag update file.txt updated &&
     -+	set_connectivity_check int-dst.git incremental &&
     -+	GIT_TRACE2_EVENT="$(pwd)/push-trace.txt" \
     -+		git -C int-src push ../int-dst.git main &&
     -+	test_trace2_data_singular connectivity trees_loaded 2 \
     -+		<push-trace.txt
     ++	test_when_finished "rm -rf dst" &&
     ++	git init --bare dst &&
     ++	git -C src push ../dst base:refs/heads/main &&
     ++	set_connectivity_check dst incremental &&
     ++	run_traced git -C src push ../dst main &&
     ++	expect_mode incremental
      +'
      +
      +test_expect_success 'fetch uses incremental when configured' '
     -+	test_when_finished "rm -rf fetch-src fetch-dst" &&
     -+	git init fetch-src &&
     -+	test_commit -C fetch-src --no-tag base file.txt &&
     -+	git clone fetch-src fetch-dst &&
     -+	test_commit -C fetch-src --no-tag update file.txt updated &&
     -+	set_connectivity_check fetch-dst incremental &&
     -+	GIT_TRACE2_EVENT="$(pwd)/fetch-trace.txt" \
     -+		git -C fetch-dst fetch origin main &&
     -+	test_trace2_data_singular connectivity trees_loaded 2 \
     -+		<fetch-trace.txt
     ++	test_when_finished "rm -rf dst" &&
     ++	git init --bare dst &&
     ++	git -C dst fetch ../src base:refs/heads/main &&
     ++	set_connectivity_check dst incremental &&
     ++	run_traced git -C dst fetch ../src main &&
     ++	expect_mode incremental
      +'
      +
     -+test_expect_success 'clone respects transfer.connectivityCheck' '
     -+	test_when_finished "rm -rf clone-src clone-dst" &&
     -+	git init clone-src &&
     -+	test_commit -C clone-src --no-tag base file.txt &&
     -+	GIT_TRACE2_EVENT="$(pwd)/clone-trace.txt" \
     -+		git -c transfer.connectivityCheck=incremental \
     -+		clone --no-local clone-src clone-dst &&
     -+	test_trace2_data_singular connectivity trees_loaded 0 \
     -+		<clone-trace.txt
     -+'
     ++for mode in incremental
     ++do
     ++	test_expect_success "push with $mode into an empty repository uses full" '
     ++		test_when_finished "rm -rf dst" &&
     ++		git init --bare dst &&
     ++		set_connectivity_check dst $mode &&
     ++		run_traced git -C src push ../dst main &&
     ++		expect_mode full
     ++	'
     ++
     ++	test_expect_success "clone with $mode uses full" '
     ++		test_when_finished "rm -rf dst" &&
     ++		run_traced git -c transfer.connectivityCheck=$mode \
     ++			clone --no-local src dst &&
     ++		expect_mode full
     ++	'
     ++done
      +
      +test_done
      
     + ## t/t6700-tree-depth.sh ##
     +@@ t/t6700-tree-depth.sh: test_expect_success 'default limit for rev-list fails gracefully' '
     + 	test_must_fail git rev-list --objects big >/dev/null
     + '
     + 
     ++# The incremental check needs a boundary; without negative revisions
     ++# rev-list falls back to the full check.
     ++test_expect_success 'limit recursion of incremental check' '
     ++	boundary=$(git commit-tree $EMPTY_TREE -m boundary) &&
     ++	git $small_ok rev-list --connectivity-check=incremental \
     ++		--objects small --not $boundary >/dev/null &&
     ++	test_must_fail git $small_no rev-list \
     ++		--connectivity-check=incremental --objects small \
     ++		--not $boundary >/dev/null
     ++'
     ++
     ++test_expect_success 'default limit for incremental check fails gracefully' '
     ++	boundary=$(git commit-tree $EMPTY_TREE -m boundary) &&
     ++	test_must_fail git rev-list --connectivity-check=incremental \
     ++		--objects big --not $boundary >/dev/null
     ++'
     ++
     + test_expect_success 'limit recursion of diff-tree -r' '
     + 	git $small_ok diff-tree -r $EMPTY_TREE small &&
     + 	test_must_fail git $small_no diff-tree -r $EMPTY_TREE small
     +
       ## tree-verify.c (new) ##
      @@
      +#include "git-compat-util.h"
     @@ tree-verify.c (new)
      +	TREE_EXPANDED  = 2,
      +};
      +
     ++/* Values are enum tree_state, stored as a byte to keep the map small. */
      +KHASH_INIT(oid_tree, struct object_id, unsigned char, 1,
      +	   oidhash_by_value, oideq_by_value)
      +
     @@ tree-verify.c (new)
      +	struct oid_array parent_trees;
      +};
      +
     ++/* The entries of a tree that still need to be verified. */
     ++struct work_list {
     ++	struct work_item *items;
     ++	size_t nr, alloc;
     ++	bool has_subtrees;
     ++};
     ++
     ++static void work_list_clear(struct work_list *list)
     ++{
     ++	for (size_t i = 0; i < list->nr; i++)
     ++		oid_array_clear(&list->items[i].parent_trees);
     ++	free(list->items);
     ++}
     ++
      +struct verify_state {
      +	kh_oid_tree_t *trees;
      +	struct oidset trusted_blobs;
     -+	int trees_loaded;
     -+	int blobs_checked;
     ++	size_t trees_loaded;
     ++	size_t blobs_checked;
      +	int exclude_promisor_objects;
     ++
     ++	/*
     ++	 * Scratch space for reading one tree at a time.  It lives here
     ++	 * rather than on the stack to keep the frame of the recursive
     ++	 * verify_tree() small.
     ++	 */
     ++	struct tree_desc desc;
     ++	struct name_entry entry;
      +};
      +
      +/*
     @@ tree-verify.c (new)
      + * same-path parent subtrees as recursive comparison bases.
      + * Returns the updated work-list cursor.
      + */
     -+static size_t collect_subtree_bases(struct work_item *work, size_t nr_work,
     -+				    size_t wi, const struct name_entry *entry)
     ++static size_t collect_subtree_bases(struct work_list *list, size_t wi,
     ++				    const struct name_entry *entry)
      +{
     -+	while (wi < nr_work) {
     -+		int cmp = base_name_compare(
     -+			work[wi].entry.path, work[wi].entry.pathlen,
     -+			work[wi].entry.mode,
     -+			entry->path, entry->pathlen,
     -+			entry->mode);
     ++	while (wi < list->nr) {
     ++		struct work_item *item = &list->items[wi];
     ++		int cmp = base_name_compare(item->entry.path,
     ++					    item->entry.pathlen,
     ++					    item->entry.mode,
     ++					    entry->path, entry->pathlen,
     ++					    entry->mode);
      +		if (cmp > 0)
      +			break;
      +		if (cmp < 0) {
      +			wi++;
      +			continue;
      +		}
     -+		if (S_ISDIR(work[wi].entry.mode) &&
     -+		    S_ISDIR(entry->mode))
     -+			oid_array_append(&work[wi].parent_trees,
     -+					 &entry->oid);
     ++		if (S_ISDIR(item->entry.mode) && S_ISDIR(entry->mode))
     ++			oid_array_append(&item->parent_trees, &entry->oid);
      +		return wi + 1;
      +	}
      +	return wi;
      +}
      +
     -+static void verify_blob(struct repository *repo,
     -+			const struct object_id *oid,
     -+			struct verify_state *vs)
     ++/*
     ++ * Report an object that could not be read, telling a missing object
     ++ * apart from one that exists but cannot be read, e.g. because it is
     ++ * corrupt.
     ++ */
     ++static int unreadable_object_error(struct repository *repo,
     ++				   const struct object_id *oid,
     ++				   const char *type)
     ++{
     ++	if (!odb_has_object(repo->objects, oid, 0))
     ++		return error(_("missing %s object %s"), type, oid_to_hex(oid));
     ++	return error(_("unable to read %s object %s"), type, oid_to_hex(oid));
     ++}
     ++
     ++static int verify_blob(struct repository *repo,
     ++		       const struct object_id *oid,
     ++		       struct verify_state *vs)
      +{
      +	int type;
      +
      +	if (oidset_contains(&vs->trusted_blobs, oid))
     -+		return;
     ++		return 0;
      +
      +	vs->blobs_checked++;
      +	type = odb_read_object_info(repo->objects, oid, NULL);
      +	if (type == OBJ_BLOB) {
      +		oidset_insert(&vs->trusted_blobs, oid);
     -+		return;
     ++		return 0;
      +	}
      +	if (type >= 0)
     -+		die(_("object %s is a %s, not a %s"),
     -+		    oid_to_hex(oid), type_name(type), "blob");
     ++		return error(_("object %s is a %s, not a %s"),
     ++			     oid_to_hex(oid), type_name(type), "blob");
      +	if (vs->exclude_promisor_objects &&
      +	    is_promisor_object(repo, oid))
     -+		return;
     -+	die(_("missing blob object '%s'"), oid_to_hex(oid));
     ++		return 0;
     ++	return unreadable_object_error(repo, oid, "blob");
      +}
      +
     -+static void *read_tree_object(struct object_database *odb,
     -+			      const struct object_id *oid,
     -+			      size_t *sizep)
     ++/*
     ++ * Read a tree object into *bufp.  Returns -1 if the object exists
     ++ * but is not a tree.  Otherwise returns 0, leaving *bufp NULL if the
     ++ * object could not be read.
     ++ */
     ++static int read_tree_object(struct object_database *odb,
     ++			    const struct object_id *oid,
     ++			    void **bufp, size_t *sizep)
      +{
      +	enum object_type type;
     -+	void *buf = odb_read_object(odb, oid, &type, sizep);
      +
     -+	if (buf && type != OBJ_TREE) {
     -+		free(buf);
     -+		die(_("object %s is a %s, not a %s"),
     -+		    oid_to_hex(oid), type_name(type), "tree");
     ++	*bufp = odb_read_object(odb, oid, &type, sizep);
     ++	if (*bufp && type != OBJ_TREE) {
     ++		FREE_AND_NULL(*bufp);
     ++		return error(_("object %s is a %s, not a %s"),
     ++			     oid_to_hex(oid), type_name(type), "tree");
      +	}
     -+	return buf;
     ++	return 0;
      +}
      +
     -+static void verify_tree(struct repository *repo,
     -+			const struct object_id *new_tree_oid,
     -+			const struct oid_array *base_trees,
     -+			struct verify_state *vs, int depth)
     ++/* List the entries of a tree that are not yet known to be trusted. */
     ++static void list_untrusted_entries(struct verify_state *vs,
     ++				   const struct object_id *tree_oid,
     ++				   void *tree_buf, size_t tree_size,
     ++				   struct work_list *list)
      +{
     -+	struct tree_desc desc;
     -+	struct name_entry entry;
     -+	struct work_item *work = NULL;
     -+	size_t nr_work = 0, alloc_work = 0;
     -+	int need_subtree_bases = 0;
     -+	size_t i, tree_size;
     -+	void *tree_buf;
     -+
     -+	if (depth > repo->settings.max_allowed_tree_depth)
     -+		die(_("exceeded maximum allowed tree depth"));
     -+
     -+	if (tree_map_get(vs->trees, new_tree_oid) >= TREE_TRUSTED)
     -+		return;
     -+
     -+	tree_buf = read_tree_object(repo->objects, new_tree_oid, &tree_size);
     -+	if (!tree_buf) {
     -+		if (vs->exclude_promisor_objects &&
     -+		    is_promisor_object(repo, new_tree_oid))
     -+			return;
     -+		die(_("bad tree object %s"),
     -+		    oid_to_hex(new_tree_oid));
     -+	}
     -+
     -+	vs->trees_loaded++;
     -+	init_tree_desc(&desc, new_tree_oid, tree_buf, tree_size);
     ++	struct name_entry *entry = &vs->entry;
      +
     -+	while (tree_entry(&desc, &entry)) {
     -+		if (S_ISGITLINK(entry.mode))
     ++	init_tree_desc(&vs->desc, tree_oid, tree_buf, tree_size);
     ++	while (tree_entry(&vs->desc, entry)) {
     ++		if (S_ISGITLINK(entry->mode))
      +			continue;
     -+		if (S_ISDIR(entry.mode)) {
     -+			if (tree_map_get(vs->trees, &entry.oid) >= TREE_TRUSTED)
     ++		if (S_ISDIR(entry->mode)) {
     ++			if (tree_map_get(vs->trees, &entry->oid) >=
     ++			    TREE_TRUSTED)
      +				continue;
     -+			need_subtree_bases = 1;
     ++			list->has_subtrees = true;
      +		} else {
     -+			if (oidset_contains(&vs->trusted_blobs, &entry.oid))
     ++			if (oidset_contains(&vs->trusted_blobs, &entry->oid))
      +				continue;
      +		}
     -+		ALLOC_GROW(work, nr_work + 1, alloc_work);
     -+		memset(&work[nr_work], 0, sizeof(work[nr_work]));
     -+		work[nr_work].entry = entry;
     -+		nr_work++;
     ++		ALLOC_GROW(list->items, list->nr + 1, list->alloc);
     ++		memset(&list->items[list->nr], 0, sizeof(list->items[0]));
     ++		list->items[list->nr++].entry = *entry;
      +	}
     ++}
     ++
     ++/*
     ++ * Scan one trusted base tree: expand it if that has not happened yet,
     ++ * and collect its same-path subtrees as bases for untrusted subtrees.
     ++ */
     ++static int scan_base_tree(struct repository *repo, struct verify_state *vs,
     ++			  const struct object_id *base_oid,
     ++			  struct work_list *list)
     ++{
     ++	int expanded = tree_map_get(vs->trees, base_oid) >= TREE_EXPANDED;
     ++	struct name_entry *entry = &vs->entry;
     ++	size_t wi = 0, base_size;
     ++	void *base_buf;
     ++
     ++	if (expanded && !list->has_subtrees)
     ++		return 0;
      +
     -+	if (!nr_work) {
     -+		free(tree_buf);
     -+		goto done;
     ++	if (read_tree_object(repo->objects, base_oid, &base_buf, &base_size))
     ++		return -1;
     ++	if (!base_buf) {
     ++		if (vs->exclude_promisor_objects &&
     ++		    is_promisor_object(repo, base_oid))
     ++			return 0;
     ++		return unreadable_object_error(repo, base_oid, "tree");
      +	}
      +
     -+	for (i = 0; base_trees && i < base_trees->nr; i++) {
     -+		const struct object_id *base_oid = &base_trees->oid[i];
     -+		int expanded = tree_map_get(vs->trees, base_oid) >= TREE_EXPANDED;
     -+		struct tree_desc base_desc;
     -+		struct name_entry scan_entry;
     -+		size_t wi = 0, base_size;
     -+		void *base_buf;
     ++	vs->trees_loaded++;
     ++	init_tree_desc(&vs->desc, base_oid, base_buf, base_size);
      +
     -+		if (expanded && !need_subtree_bases)
     ++	while (tree_entry(&vs->desc, entry)) {
     ++		if (S_ISGITLINK(entry->mode))
      +			continue;
      +
     -+		base_buf = read_tree_object(repo->objects, base_oid,
     -+					    &base_size);
     -+		if (!base_buf) {
     -+			if (vs->exclude_promisor_objects &&
     -+			    is_promisor_object(repo, base_oid))
     -+				continue;
     -+			die(_("bad tree object %s"),
     -+			    oid_to_hex(base_oid));
     ++		/* Same-path subtree as a base for an untrusted one. */
     ++		if (list->has_subtrees)
     ++			wi = collect_subtree_bases(list, wi, entry);
     ++
     ++		/* Expand: the base is trusted, so are its entries. */
     ++		if (!expanded) {
     ++			if (S_ISDIR(entry->mode))
     ++				tree_map_add(vs->trees, &entry->oid,
     ++					     TREE_TRUSTED);
     ++			else
     ++				oidset_insert(&vs->trusted_blobs,
     ++					      &entry->oid);
      +		}
     ++	}
      +
     -+		vs->trees_loaded++;
     -+		init_tree_desc(&base_desc, base_oid, base_buf, base_size);
     ++	if (!expanded)
     ++		tree_map_add(vs->trees, base_oid, TREE_EXPANDED);
      +
     -+		while (tree_entry(&base_desc, &scan_entry)) {
     -+			if (S_ISGITLINK(scan_entry.mode))
     -+				continue;
     ++	free(base_buf);
     ++	return 0;
     ++}
      +
     -+			if (need_subtree_bases)
     -+				wi = collect_subtree_bases(work, nr_work,
     -+							   wi, &scan_entry);
     -+
     -+			if (!expanded) {
     -+				if (S_ISDIR(scan_entry.mode))
     -+					tree_map_add(vs->trees,
     -+						     &scan_entry.oid,
     -+						     TREE_TRUSTED);
     -+				else
     -+					oidset_insert(&vs->trusted_blobs,
     -+						      &scan_entry.oid);
     -+			}
     -+		}
     ++/*
     ++ * Verify a tree against the trusted base trees, loading as little as
     ++ * possible:
     ++ *
     ++ * First list the entries that are not yet known to be trusted.  When
     ++ * the parent trees have been expanded before, only the changed entries
     ++ * remain.
     ++ *
     ++ * Then scan the trusted base trees (the same-path trees of the parent
     ++ * commits).  A base that has not been expanded yet is read so that its
     ++ * entries can be marked trusted; this often shows that listed entries
     ++ * are trusted after all.  An already expanded base is still read when
     ++ * there are untrusted subtrees, because its subtree at the same path is
     ++ * what lets the untrusted subtree skip its own unchanged entries in
     ++ * turn.
     ++ *
     ++ * Finally check that the remaining blobs exist, and recurse into the
     ++ * remaining subtrees with the same-path subtrees as their bases.
     ++ */
     ++static int verify_tree(struct repository *repo,
     ++		       const struct object_id *tree_oid,
     ++		       const struct oid_array *trusted_bases,
     ++		       struct verify_state *vs, int depth)
     ++{
     ++	struct work_list list = { 0 };
     ++	size_t i, tree_size;
     ++	void *tree_buf;
     ++	int ret = 0;
     ++
     ++	/*
     ++	 * Enforce core.maxTreeDepth like process_tree() in list-objects.c,
     ++	 * so that the full and incremental checks reject the same trees.
     ++	 */
     ++	if (depth > repo->settings.max_allowed_tree_depth)
     ++		return error(_("exceeded maximum allowed tree depth"));
     ++
     ++	if (tree_map_get(vs->trees, tree_oid) >= TREE_TRUSTED)
     ++		return 0;
      +
     -+		if (!expanded)
     -+			tree_map_add(vs->trees, base_oid, TREE_EXPANDED);
     ++	if (read_tree_object(repo->objects, tree_oid, &tree_buf, &tree_size))
     ++		return -1;
     ++	if (!tree_buf) {
     ++		/*
     ++		 * A missing tree that is promised can be fetched on demand,
     ++		 * so it is not an error, as in the full check.
     ++		 */
     ++		if (vs->exclude_promisor_objects &&
     ++		    is_promisor_object(repo, tree_oid))
     ++			return 0;
     ++		return unreadable_object_error(repo, tree_oid, "tree");
     ++	}
     ++
     ++	vs->trees_loaded++;
     ++	list_untrusted_entries(vs, tree_oid, tree_buf, tree_size, &list);
      +
     -+		free(base_buf);
     ++	/* Scan the base trees, unless nothing is left to verify. */
     ++	for (i = 0; list.nr && i < trusted_bases->nr; i++) {
     ++		ret = scan_base_tree(repo, vs, &trusted_bases->oid[i], &list);
     ++		if (ret)
     ++			goto done;
      +	}
      +
     -+	for (i = 0; i < nr_work; i++) {
     -+		if (S_ISDIR(work[i].entry.mode))
     -+			verify_tree(repo, &work[i].entry.oid,
     -+				    &work[i].parent_trees, vs,
     -+				    depth + 1);
     ++	/*
     ++	 * Check the remaining entries in tree order.  Verifying a subtree
     ++	 * can show that later entries are trusted, which then makes them
     ++	 * cheap to skip.
     ++	 */
     ++	for (i = 0; i < list.nr && !ret; i++) {
     ++		struct work_item *item = &list.items[i];
     ++
     ++		if (S_ISDIR(item->entry.mode))
     ++			ret = verify_tree(repo, &item->entry.oid,
     ++					  &item->parent_trees, vs, depth + 1);
      +		else
     -+			verify_blob(repo, &work[i].entry.oid, vs);
     ++			ret = verify_blob(repo, &item->entry.oid, vs);
      +	}
     -+
     -+	free(tree_buf);
     ++	if (!ret)
     ++		tree_map_add(vs->trees, tree_oid, TREE_EXPANDED);
      +
      +done:
     -+	tree_map_add(vs->trees, new_tree_oid, TREE_EXPANDED);
     -+	for (i = 0; i < nr_work; i++)
     -+		oid_array_clear(&work[i].parent_trees);
     -+	free(work);
     ++	free(tree_buf);
     ++	work_list_clear(&list);
     ++	return ret;
      +}
      +
     -+static void verify_commit_tree(struct repository *repo,
     -+			       struct commit *commit,
     -+			       struct verify_state *vs)
     ++static int verify_commit_tree(struct repository *repo,
     ++			      struct commit *commit,
     ++			      struct verify_state *vs)
      +{
     -+	struct oid_array base_trees = OID_ARRAY_INIT;
     ++	struct oid_array parent_trees = OID_ARRAY_INIT;
      +	const struct object_id *tree_oid;
      +	struct commit_list *p;
     ++	int ret;
      +
      +	/*
      +	 * Parent trees are trusted: boundary parents are already
      +	 * connected, and earlier incoming parents were verified
      +	 * first due to the topological processing order.
     ++	 *
     ++	 * The revision walk skips missing parents that are promised
     ++	 * (see process_parents()), so do the same here.  Such a parent
     ++	 * contributes no base, which only means less of the new tree
     ++	 * can be skipped.
      +	 */
      +	for (p = commit->parents; p; p = p->next) {
     -+		parse_commit_or_die(p->item);
     ++		if (repo_parse_commit_gently(repo, p->item, 1) < 0) {
     ++			if (vs->exclude_promisor_objects &&
     ++			    is_promisor_object(repo, &p->item->object.oid))
     ++				continue;
     ++			ret = error(_("unable to parse commit %s"),
     ++				    oid_to_hex(&p->item->object.oid));
     ++			goto out;
     ++		}
      +		tree_oid = get_commit_tree_oid(p->item);
     -+		if (!tree_oid)
     -+			die(_("unable to load root tree for commit %s"),
     -+			    oid_to_hex(&p->item->object.oid));
     ++		if (!tree_oid) {
     ++			ret = error(_("unable to load root tree for commit %s"),
     ++				    oid_to_hex(&p->item->object.oid));
     ++			goto out;
     ++		}
      +		tree_map_add(vs->trees, tree_oid, TREE_TRUSTED);
     -+		oid_array_append(&base_trees, tree_oid);
     ++		oid_array_append(&parent_trees, tree_oid);
      +	}
      +
      +	tree_oid = get_commit_tree_oid(commit);
     -+	if (!tree_oid)
     -+		die(_("unable to load root tree for commit %s"),
     -+		    oid_to_hex(&commit->object.oid));
     -+	verify_tree(repo, tree_oid, &base_trees, vs, 0);
     -+	oid_array_clear(&base_trees);
     ++	if (!tree_oid) {
     ++		ret = error(_("unable to load root tree for commit %s"),
     ++			    oid_to_hex(&commit->object.oid));
     ++		goto out;
     ++	}
     ++	ret = verify_tree(repo, tree_oid, &parent_trees, vs, 0);
     ++out:
     ++	oid_array_clear(&parent_trees);
     ++	return ret;
      +}
      +
     -+void verify_commits_incremental(struct repository *repo,
     -+				struct commit_list **commits,
     -+				int exclude_promisor_objects)
     ++int verify_commits_incremental(struct repository *repo,
     ++			       struct commit_list **commits,
     ++			       int exclude_promisor_objects)
      +{
      +	struct verify_state vs = { 0 };
      +	struct commit_list *iter;
      +	unsigned nr_before;
     ++	int ret = 0;
      +
      +	if (repo->fetch_if_missing)
      +		BUG("verify_commits_incremental must not be called "
     @@ tree-verify.c (new)
      +	 */
      +	nr_before = commit_list_count(*commits);
      +	sort_in_topological_order(commits, REV_SORT_IN_GRAPH_ORDER);
     -+	if (commit_list_count(*commits) < nr_before)
     -+		die(_("cycle detected in incoming commit graph"));
     ++	if (commit_list_count(*commits) < nr_before) {
     ++		ret = error(_("cycle detected in incoming commit graph"));
     ++		goto out;
     ++	}
      +
      +	*commits = commit_list_reverse(*commits);
      +
     -+	for (iter = *commits; iter; iter = iter->next)
     -+		verify_commit_tree(repo, iter->item, &vs);
     ++	for (iter = *commits; iter && !ret; iter = iter->next)
     ++		ret = verify_commit_tree(repo, iter->item, &vs);
      +
     ++out:
      +	kh_destroy_oid_tree(vs.trees);
      +	oidset_clear(&vs.trusted_blobs);
      +	trace2_data_intmax("connectivity", repo,
     -+			   "trees_loaded", vs.trees_loaded);
     ++			   "trees_loaded", (intmax_t)vs.trees_loaded);
      +	trace2_data_intmax("connectivity", repo,
     -+			   "blobs_checked", vs.blobs_checked);
     ++			   "blobs_checked", (intmax_t)vs.blobs_checked);
     ++	return ret;
      +}
      
       ## tree-verify.h (new) ##
     @@ tree-verify.h (new)
      +
      +/*
      + * Verify trees of commits incrementally against their parents.
     -+ * Dies on verification failure.
     ++ * Returns 0 on success, or reports an error and returns -1.
      + */
     -+void verify_commits_incremental(struct repository *repo,
     -+				struct commit_list **commits,
     -+				int exclude_promisor_objects);
     ++int verify_commits_incremental(struct repository *repo,
     ++			       struct commit_list **commits,
     ++			       int exclude_promisor_objects);
      +
      +#endif /* TREE_VERIFY_H */

-: ---------- > 6: f6031967e6 connected: add "auto" connectivity check mode

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check.

---
 Documentation/Makefile                        |   1 +
 .../technical/connectivity-check.adoc         | 112 ++++++++++++++++++
 Documentation/technical/meson.build           |   1 +
 3 files changed, 114 insertions(+)
 create mode 100644 Documentation/technical/connectivity-check.adoc

index f8dea4b395..1747f4a00b 100644
@@ -122,6 +122,7 @@ TECH_DOCS += technical/bitmap-format
 TECH_DOCS += technical/build-systems
 TECH_DOCS += technical/bundle-uri
 TECH_DOCS += technical/commit-graph
+TECH_DOCS += technical/connectivity-check
 TECH_DOCS += technical/directory-rename-detection
 TECH_DOCS += technical/hash-function-transition
 TECH_DOCS += technical/large-object-promisors

index 9ce11d5e48..c439a7a072 100644
@@ -11,6 +11,7 @@ articles = [
   'build-systems.adoc',
   'bundle-uri.adoc',
   'commit-graph.adoc',
+  'connectivity-check.adoc',
   'directory-rename-detection.adoc',
   'hash-function-transition.adoc',
   'large-object-promisors.adoc',

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add benchmarks for the rev-list connectivity check. Each fixture varies one dimension: total tree size, incoming commit count, or files changed per commit.

---
 ...enerate-repo-p5413-connectivity-check.perl | 44 ++++++++++
 t/perf/p5413-connectivity-check.sh            | 85 +++++++++++++++++++
 2 files changed, 129 insertions(+)
 create mode 100644 t/perf/generate-repo-p5413-connectivity-check.perl
 create mode 100755 t/perf/p5413-connectivity-check.sh

new file mode 100644
index 0000000000..bfa25f60fc
@@ -0,0 +1,44 @@
+#!/usr/bin/perl
+#
+# Generate a fast-import stream for p5413 connectivity check benchmarks.
+#
+# Usage: generate-repo-p5413-connectivity-check.perl
+#            <dirs> <files_per_dir> <commits> [<hot_dirs>] [<files_per_commit>]
+#
+# Creates one initial commit with dirs*files_per_dir files, then
+# <commits> additional commits each modifying <files_per_commit>
+# files in directories chosen round-robin from 1..<hot_dirs>.
+
+use strict;
+use warnings;
+
+my ($nd, $nf, $nc, $hot, $fpc) = @ARGV;
+$hot = $nd if !$hot || $hot > $nd;
+$fpc = 1   if !$fpc;
+
+sub data {
+	printf "data %d\n%s\n", length($_[0]), $_[0];
+}
+
+# Initial tree: one commit with nd*nf files.
+printf "commit refs/heads/main\n";
+printf "committer perf <perf\@test.com> now\n";
+data("initial");
+for my $d (1..$nd) {
+	for my $f (1..$nf) {
+		printf "M 100644 inline d-%04d/f-%03d\n", $d, $f;
+		data(sprintf "%03d%03d", $d, $f);
+	}
+}
+
+# Subsequent commits (auto-chained by fast-import).
+for my $i (1..$nc) {
+	printf "commit refs/heads/main\n";
+	printf "committer perf <perf\@test.com> now\n";
+	data(sprintf "change-%03d", $i);
+	for my $j (0..$fpc-1) {
+		my $d = (($i + $j) % $hot) + 1;
+		printf "M 100644 inline d-%04d/f-001\n", $d;
+		data(sprintf "c%d-%d", $i, $j);
+	}
+}

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The next commits will need to use the repository to read configuration in check_connected(). Rather than accessing the_repository, refactor the code to instead require the caller to pass in a repository so that we can remove USE_THE_REPOSITORY_VARIABLE.

---
 builtin/clone.c        |  2 +-
 builtin/fetch.c        |  5 +++--
 builtin/receive-pack.c | 12 ++++++++----
 bundle.c               |  2 +-
 connected.c            | 22 +++++++++++-----------
 connected.h            |  4 +++-
 fetch-pack.c           |  3 ++-
 7 files changed, 29 insertions(+), 21 deletions(-)

index 44037dd428..1532361ea0 100644
@@ -572,7 +572,7 @@ static void update_remote_refs(const struct ref *refs,
 		opt.transport = transport;
 		opt.progress = transport->progress;
 
-		if (check_connected(iterate_ref_map, &rm, &opt))
+		if (check_connected(the_repository, iterate_ref_map, &rm, &opt))
 			die(_("remote did not send all necessary objects"));
 	}
 

index b2decc6cfd..0ef7fe92f1 100644
@@ -1250,7 +1250,8 @@ static int store_updated_refs(struct display_state *display_state,
 
 		opt.exclude_hidden_refs_section = "fetch";
 		rm = ref_map;
-		if (check_connected(iterate_ref_map, &rm, &opt)) {
+		if (check_connected(the_repository, iterate_ref_map,
+				    &rm, &opt)) {
 			rc = error(_("%s did not send all necessary objects"),
 				   display_state->url);
 			goto abort;

@@ -1422,7 +1423,7 @@ static int check_exist_and_connected(struct ref *ref_map)
 
 	opt.quiet = 1;
 	opt.exclude_hidden_refs_section = "fetch";
-	return check_connected(iterate_ref_map, &rm, &opt);
+	return check_connected(the_repository, iterate_ref_map, &rm, &opt);
 }
 
 static int fetch_and_consume_refs(struct display_state *display_state,

index 1d5b050beb..4f8f58950a 100644
@@ -1385,7 +1385,8 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 			oid_array_append(&extra, &si->shallow->oid[i]);
 
 	setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
-	if (check_connected(command_singleton_iterator, cmd, &opt)) {
+	if (check_connected(the_repository, command_singleton_iterator,
+			    cmd, &opt)) {
 		rollback_shallow_file(the_repository, &shallow_lock);
 		oid_array_clear(&extra);
 		return -1;

@@ -1827,8 +1828,9 @@ static void set_connectivity_errors(struct command *commands,
 		odb_transaction_env(transaction, &env);
 		opt.env = env.v;
 
-		if (!check_connected(command_singleton_iterator, &singleton,
-				     &opt))
+		if (!check_connected(the_repository,
+				     command_singleton_iterator,
+				     &singleton, &opt))
 			continue;
 
 		cmd->error_string = "missing necessary objects";

@@ -2090,7 +2092,9 @@ static void execute_commands(struct command *commands,
 		opt.env = env.v;
 		opt.exclude_hidden_refs_section = "receive";
 
-		if (check_connected(iterate_receive_command_list, &data, &opt))
+		if (check_connected(the_repository,
+				    iterate_receive_command_list,
+				    &data, &opt))
 			set_connectivity_errors(commands, si, transaction);
 
 		if (use_sideband)

index f55a521b2a..0a68e856f5 100644
@@ -259,7 +259,7 @@ int verify_bundle(struct repository *r,
 	if (ret)
 		goto cleanup;
 
-	if ((ret = check_connected(iterate_ref_map, &iter, &opts)))
+	if ((ret = check_connected(r, iterate_ref_map, &iter, &opts)))
 		error(_("some prerequisite commits exist in the object store, "
 			"but are not connected to the repository's history"));
 

index 929b9bd28d..34045f1032 100644
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "hex.h"

@@ -35,23 +33,24 @@ static int promised_object_cb(const struct object_id *oid UNUSED,
  * objects in non-promisor packs, in which case we'll have to fall back to the
  * rev-list-based connectivity checks. Returns a negative error code on error.
  */
-static int check_connected_promisor(oid_iterate_fn fn,
+static int check_connected_promisor(struct repository *repo,
+				    oid_iterate_fn fn,
 				    void *cb_data,
 				    const struct object_id **oid)
 {
 	struct odb_for_each_object_options opts = {
 		.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
-		.prefix_hex_len = the_repository->hash_algo->hexsz,
+		.prefix_hex_len = repo->hash_algo->hexsz,
 	};
 	int err;
 
-	odb_reprepare(the_repository->objects);
+	odb_reprepare(repo->objects);
 	do {
 		bool found = false;
 
 		opts.prefix = *oid;
 
-		err = odb_for_each_object_ext(the_repository->objects, NULL,
+		err = odb_for_each_object_ext(repo->objects, NULL,
 					      promised_object_cb, &found, &opts);
 		if (err < 0)
 			return err;

@@ -78,7 +77,8 @@ static int check_connected_promisor(oid_iterate_fn fn,
  *
  * Returns 0 if everything is connected, non-zero otherwise.
  */
-int check_connected(oid_iterate_fn fn, void *cb_data,
+int check_connected(struct repository *repo,
+		    oid_iterate_fn fn, void *cb_data,
 		    struct check_connected_options *opt)
 {
 	struct child_process rev_list = CHILD_PROCESS_INIT;

@@ -101,8 +101,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		return err;
 	}
 
-	if (repo_has_promisor_remote(the_repository)) {
-		err = check_connected_promisor(fn, cb_data, &oid);
+	if (repo_has_promisor_remote(repo)) {
+		err = check_connected_promisor(repo, fn, cb_data, &oid);
 		if (err) {
 			if (opt->err_fd)
 				close(opt->err_fd);

@@ -119,7 +119,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	strvec_push(&rev_list.args,"rev-list");
 	strvec_push(&rev_list.args, "--objects");
 	strvec_push(&rev_list.args, "--stdin");
-	if (repo_has_promisor_remote(the_repository))
+	if (repo_has_promisor_remote(repo))
 		strvec_push(&rev_list.args, "--exclude-promisor-objects");
 	if (!opt->is_deepening_fetch) {
 		strvec_push(&rev_list.args, "--not");

@@ -160,7 +160,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,
 			   base_len);
 		strbuf_addstr(&idx_file, ".idx");
-		new_pack = add_packed_git(the_repository, idx_file.buf,
+		new_pack = add_packed_git(repo, idx_file.buf,
 					  idx_file.len, 1);
 		strbuf_release(&idx_file);
 	}

index 16b2c84f2e..0624965baa 100644
@@ -2,6 +2,7 @@
 #define CONNECTED_H
 
 struct object_id;
+struct repository;
 struct transport;
 
 /*

@@ -66,7 +67,8 @@ struct check_connected_options {
  *
  * If "opt" is NULL, behaves as if CHECK_CONNECTED_INIT was passed.
  */
-int check_connected(oid_iterate_fn fn, void *cb_data,
+int check_connected(struct repository *repo,
+		    oid_iterate_fn fn, void *cb_data,
 		    struct check_connected_options *opt);
 
 #endif /* CONNECTED_H */

index 30a4dd6119..2cf8f8d653 100644
@@ -2186,7 +2186,8 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 		opt.shallow_file = alternate_shallow_file;
 		if (args->deepen)
 			opt.is_deepening_fetch = 1;
-		if (check_connected(iterate_ref_map, &iterator, &opt)) {
+		if (check_connected(the_repository, iterate_ref_map,
+				    &iterator, &opt)) {
 			error(_("remote did not send all necessary objects"));
 			free_refs(ref_cpy);
 			ref_cpy = NULL;

gitgitgadget

From: Kristofer Karlsson <[email protected]>

Add the transfer.connectivityCheck configuration option to choose the algorithm used for the connectivity check. For now the only accepted value is "full", which is the existing behavior and the default. Unknown values fall back to "full" with a warning, so that configuration written for a newer version of Git does not break fetches and pushes; "full" is always a correct check.

check_connected() now always tells its rev-list subprocess which algorithm to use with an internal --connectivity-check=<mode> option, which so far only accepts "full". Passing the mode explicitly means rev-list knows it is running a connectivity check, so later optimizations can be limited to that case.

This prepares for an incremental check added in the next commit.

---
 Documentation/config/transfer.adoc  | 16 ++++++++++++++++
 Documentation/rev-list-options.adoc |  5 +++++
 builtin/rev-list.c                  | 23 +++++++++++++++++++++++
 connected.c                         | 26 ++++++++++++++++++++++++++
 4 files changed, 70 insertions(+)

index f1ce50f4a6..811456f1bf 100644
@@ -1,3 +1,19 @@
+transfer.connectivityCheck::
+	Choose which algorithm to use for the connectivity check
+	performed during object transfer operations such as
+	linkgit:git-fetch[1] and linkgit:git-receive-pack[1].
+	The connectivity check verifies that all objects reachable
+	from the incoming tips are available locally or, in a partial
+	clone, promised by a promisor remote.
+	The variants are as follows:
++
+--
+`full` (default);;
+	Walk the full object closure of the boundary commits.
+--
++
+Unknown values are treated as `full`, with a warning.
+
 transfer.credentialsInUrl::
 	A configured URL can contain plaintext credentials in the form
 	`<protocol>://<user>:<password>@<domain>/<path>`. You may want

index bd9f345690..3979a142a5 100644
@@ -1102,6 +1102,11 @@ It is an error to use `--missing-only` without `--missing=print` or
 	stronger than `--missing=allow-promisor` because it limits the
 	traversal, rather than just silencing errors about missing
 	objects.
+
+`--connectivity-check=full`::
+	(For internal use only.)  Used by `check_connected()` to
+	select the connectivity check algorithm; see
+	`transfer.connectivityCheck` in linkgit:git-config[1].
 endif::git-rev-list[]
 
 `--no-walk[=(sorted|unsorted)]`::

index 0faa833fac..810eaa15e3 100644
@@ -29,6 +29,24 @@
 #include "quote.h"
 #include "strbuf.h"
 
+enum connectivity_check_mode {
+	CONNECTIVITY_CHECK_FULL,
+};
+
+struct connectivity_check {
+	enum connectivity_check_mode mode;
+};
+
+static void parse_connectivity_check(struct repository *repo UNUSED,
+				     const char *arg,
+				     struct connectivity_check *check)
+{
+	if (!strcmp(arg, "full"))
+		check->mode = CONNECTIVITY_CHECK_FULL;
+	else
+		die(_("unknown --connectivity-check mode '%s'"), arg);
+}
+
 struct rev_list_info {
 	struct rev_info *revs;
 	int flags;