Repository navigation
Conversation
spkrka
marked this pull request as ready for review
August 28, 2026 10:15
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
10 times, most recently
from
September 1, 2026 18:03
326b1e4 to
452d6b9
Compare
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
9 times, most recently
from
September 11, 2026 12:35
0d19d97 to
3f4473e
Compare
Author
|
/cc Patrick Steinhardt ps@pks.im |
Author
|
/cc Jeff King <peff@peff.net |
|
User |
Author
|
/cc Elijah Newren newren@gmail.com |
|
User |
|
User |
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
from
September 14, 2026 08:55
3f4473e to
ebe6c90
Compare
Author
|
/submit |
|
Submitted as pull.2211.git.1789379276.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This series adds an incremental mode for the connectivity check, gated
> behind transfer.connectivityCheck=incremental (no expected changes unless
> you opt in).
>
> The intent is to solve the problem of the connectivity check slowing down as
> the number of reachable objects from the boundary grows.
Exciting benchmarks. |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1,3 +1,23 @@ | |||
| transfer.connectivityCheck:: | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Sep 28, 2026 at 01:02:32PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> 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.
Okay. In the old world we basically mark evertyhing as uninteresting,
including trees and blobs, ...
> 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.
... 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.
> 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.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> 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
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.
> diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> index d20bff6af6..0a8f370546 100644
> --- a/Documentation/technical/connectivity-check.adoc
> +++ b/Documentation/technical/connectivity-check.adoc
> @@ -107,3 +107,137 @@ 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.
> +
> +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 parents.
Can we define "parents" here? Specifically, I wonder how you define
"parent" in the case where you perform a force push or when creating a
new reference. Is it the parent of the first new commit? Is it the old
state of the ref, if it even exists?
> +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.
Where the "already-connected side of the boundary" is anything reachable
via a reference.
> +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.
> +
> +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.
Right. The big question to me still is how you identify
already-connected trees without having to read all references.
> +Worked example
Worked?
[snip]
> diff --git a/tree-verify.c b/tree-verify.c
> new file mode 100644
> index 0000000000..5c11c2251a
> --- /dev/null
> +++ b/tree-verify.c
> @@ -0,0 +1,316 @@
[snip]
> +static void verify_commit_tree(struct repository *repo,
> + struct commit *commit,
> + struct verify_state *vs)
> +{
> + struct oid_array base_trees = OID_ARRAY_INIT;
> + struct commit_list *p;
> +
> + /*
> + * Parent trees are trusted: boundary parents are already
> + * connected, and earlier incoming parents were verified
> + * first due to the topological processing order.
> + */
I feel like I still miss where exactly you establish the trust boundary
between preexisting fully-connected commits and new commits.
> + 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)
> + 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);
> +}
> +
> +void 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;
> +
> + 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;
> +
> + /*
> + * Ancestors must be verified before descendants so that parent
> + * trees can be trusted without re-verification. Sort explicitly
> + * rather than relying on the caller's ordering.
> + *
> + * sort_in_topological_order() silently drops cycle members,
> + * so explicitly check if the size has changed.
> + */
> + 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"));
I don't think we should just die, should we? That may not interact well
with git-receive-pack(1) and others that expect a broken connectivity
check to bubble up errors so that they can properly report those to the
client and clean up their local state.
> + *commits = commit_list_reverse(*commits);
> +
> + for (iter = *commits; iter; iter = iter->next)
> + verify_commit_tree(repo, iter->item, &vs);
> + kh_destroy_oid_tree(vs.trees);
> + oidset_clear(&vs.trusted_blobs);
> + trace2_data_intmax("connectivity", repo,
> + "trees_loaded", vs.trees_loaded);
> + trace2_data_intmax("connectivity", repo,
> + "blobs_checked", vs.blobs_checked);
> +}
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Mon, 5 Oct 2026 at 10:00, Patrick Steinhardt <ps@pks.im> wrote:
>
> ... 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?
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).
>
> 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.
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).
> 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.
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.
> 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.
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.
> > +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 parents.
>
> Can we define "parents" here? Specifically, I wonder how you define
> "parent" in the case where you perform a force push or when creating a
> new reference. Is it the parent of the first new commit? Is it the old
> state of the ref, if it even exists?
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.
> > +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.
>
> Where the "already-connected side of the boundary" is anything reachable
> via a reference.
Yes, precisely.
> > +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.
> > +
> > +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.
>
> Right. The big question to me still is how you identify
> already-connected trees without having to read all references.
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).
> > +Worked example
>
> Worked?
Hm, I suppose I could just use the phrase "Example" here instead.
Will change.
>
> [snip]
> > diff --git a/tree-verify.c b/tree-verify.c
> > new file mode 100644
> > index 0000000000..5c11c2251a
> > --- /dev/null
> > +++ b/tree-verify.c
> > @@ -0,0 +1,316 @@
> [snip]
> > +static void verify_commit_tree(struct repository *repo,
> > + struct commit *commit,
> > + struct verify_state *vs)
> > +{
> > + struct oid_array base_trees = OID_ARRAY_INIT;
> > + struct commit_list *p;
> > +
> > + /*
> > + * Parent trees are trusted: boundary parents are already
> > + * connected, and earlier incoming parents were verified
> > + * first due to the topological processing order.
> > + */
>
> I feel like I still miss where exactly you establish the trust boundary
> between preexisting fully-connected commits and new commits.
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.
> > + if (commit_list_count(*commits) < nr_before)
> > + die(_("cycle detected in incoming commit graph"));
>
> I don't think we should just die, should we? That may not interact well
> with git-receive-pack(1) and others that expect a broken connectivity
> check to bubble up errors so that they can properly report those to the
> client and clean up their local state.
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,
KristoferThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Tue, Oct 06, 2026 at 12:37:40PM +0200, Kristofer Karlsson wrote:
> On Mon, 5 Oct 2026 at 10:00, Patrick Steinhardt <ps@pks.im> wrote:
[snip]
> > > +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 parents.
> >
> > Can we define "parents" here? Specifically, I wonder how you define
> > "parent" in the case where you perform a force push or when creating a
> > new reference. Is it the parent of the first new commit? Is it the old
> > state of the ref, if it even exists?
>
> 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).
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.
> > > +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.
> > > +
> > > +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.
> >
> > Right. The big question to me still is how you identify
> > already-connected trees without having to read all references.
>
> 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).
Yeah.
> > > diff --git a/tree-verify.c b/tree-verify.c
> > > new file mode 100644
> > > index 0000000000..5c11c2251a
> > > --- /dev/null
> > > +++ b/tree-verify.c
> > > @@ -0,0 +1,316 @@
> > [snip]
> > > +static void verify_commit_tree(struct repository *repo,
> > > + struct commit *commit,
> > > + struct verify_state *vs)
> > > +{
> > > + struct oid_array base_trees = OID_ARRAY_INIT;
> > > + struct commit_list *p;
> > > +
> > > + /*
> > > + * Parent trees are trusted: boundary parents are already
> > > + * connected, and earlier incoming parents were verified
> > > + * first due to the topological processing order.
> > > + */
> >
> > I feel like I still miss where exactly you establish the trust boundary
> > between preexisting fully-connected commits and new commits.
>
> 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.
>
> > > + if (commit_list_count(*commits) < nr_before)
> > > + die(_("cycle detected in incoming commit graph"));
> >
> > I don't think we should just die, should we? That may not interact well
> > with git-receive-pack(1) and others that expect a broken connectivity
> > check to bubble up errors so that they can properly report those to the
> > client and clean up their local state.
>
> 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.
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!
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Tue, 6 Oct 2026 at 14:08, Patrick Steinhardt <ps@pks.im> wrote:
> > 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).
>
> 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?
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.
>
> 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.
Yes, precisely.
> 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.
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
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
3 times, most recently
from
October 12, 2026 11:55
a7dfb84 to
efaada0
Compare
Author
|
/submit |
|
Submitted as pull.2211.v3.git.1791806877.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -1,3 +1,23 @@ | |||
| transfer.connectivityCheck:: | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/connected.c b/connected.c
> index 929b9bd28d..e3dcd8e2b0 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -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;
The file does declare USE_THE_REPOSITORY_VARIABLE but I wonder if we
can easily reduce the dependency to the_repository by adding "struct
repository *repo" pointer to the check_connected_options structure
and using it here and passing it to check_connected_promisor() which
is a file-scope static nearby.
> + 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)There was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Mon, 12 Oct 2026 at 20:49, Junio C Hamano <gitster@pobox.com> wrote:
>
> > +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;
>
> The file does declare USE_THE_REPOSITORY_VARIABLE but I wonder if we
> can easily reduce the dependency to the_repository by adding "struct
> repository *repo" pointer to the check_connected_options structure
> and using it here and passing it to check_connected_promisor() which
> is a file-scope static nearby.
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,
KristoferThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Kristofer Karlsson <krka@spotify.com> writes:
> 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.
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.
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
This is exciting.
> 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
> ...
> 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.
Hmm, ... OK.
> 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.
OK. |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Mon, 12 Oct 2026 at 20:55, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > 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.
>
> This is exciting.
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.
> > 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
> > ...
> > 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.
>
> Hmm, ... OK.
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 branch is now known as |
|
This patch series was integrated into seen via git@314a172. |
|
There was a status update in the "New Topics" section about the branch The connectivity check in 'git fetch' and 'git receive-pack' has been optimized with an alternative incremental mode, available via 'transfer.connectivityCheck=incremental'. Instead of walking the full object closure of the boundary commits, it compares the trees of incoming commits against their trusted parent trees, skipping unchanged subtrees. Expecting a reroll. cf. <CAL71e4MVf6QcFsAdUnW3XGw_g-OLdUZFyopkELVLsCqvM=Qinw@mail.gmail.com> source: <pull.2211.v3.git.1791806877.gitgitgadget@gmail.com> |
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
from
October 14, 2026 05:47
efaada0 to
3ecd788
Compare
| @@ -0,0 +1,110 @@ | |||
| Connectivity checking | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Oct 12, 2026 at 12:07:54PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> Add Documentation/technical/connectivity-check.adoc describing
> the connectivity invariant and the full connectivity check.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> .../technical/connectivity-check.adoc | 110 ++++++++++++++++++
> 1 file changed, 110 insertions(+)
> create mode 100644 Documentation/technical/connectivity-check.adoc
Shouldn't both the Makefile (`TECH_DOCS`) and Meson (`articles`) be
adapted to build this?
> diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> new file mode 100644
> index 0000000000..abef02debd
> --- /dev/null
> +++ b/Documentation/technical/connectivity-check.adoc
[snip]
> +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.
Would it make sense to document that fast path here, too?
> +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
Nit: "verified against these marks" reads a tiny bit awkward. How about
"Trees of incoming commits are then walked, stopping at objects already
marked as uninteresting".
> +covered. The cost of marking boundary trees uninteresting
> +dominates when the boundary commits have large trees, even when
> +the incoming changes are small.
Overall I think this part is a lot better now, thanks.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 15 Oct 2026 at 08:55, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 12, 2026 at 12:07:54PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <krka@spotify.com>
> >
> > Add Documentation/technical/connectivity-check.adoc describing
> > the connectivity invariant and the full connectivity check.
> >
> > Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> > ---
> > .../technical/connectivity-check.adoc | 110 ++++++++++++++++++
> > 1 file changed, 110 insertions(+)
> > create mode 100644 Documentation/technical/connectivity-check.adoc
>
> Shouldn't both the Makefile (`TECH_DOCS`) and Meson (`articles`) be
> adapted to build this?
Ah yes, will fix for next reroll.
>
> > diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> > new file mode 100644
> > index 0000000000..abef02debd
> > --- /dev/null
> > +++ b/Documentation/technical/connectivity-check.adoc
> [snip]
> > +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.
>
> Would it make sense to document that fast path here, too?
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.
> > +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
>
> Nit: "verified against these marks" reads a tiny bit awkward. How about
> "Trees of incoming commits are then walked, stopping at objects already
> marked as uninteresting".
Agreed, I think that would be easier to read. Will fix.
Thanks,
Kristofer
| @@ -0,0 +1,44 @@ | |||
| #!/usr/bin/perl | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Oct 12, 2026 at 12:07:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> diff --git a/t/perf/generate-repo-p5413-connectivity-check.perl b/t/perf/generate-repo-p5413-connectivity-check.perl
> new file mode 100644
> index 0000000000..9546996280
> --- /dev/null
> +++ b/t/perf/generate-repo-p5413-connectivity-check.perl
All of the benchmarks will need to be marked with the PERL prerequisite.
> @@ -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>]
These still say p5412, but it's p5413.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 15 Oct 2026 at 08:55, Patrick Steinhardt <ps@pks.im> wrote:
>
> All of the benchmarks will need to be marked with the PERL prerequisite.
Ah good catch, will add.
>
> > @@ -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>]
>
> These still say p5412, but it's p5413.
Oops, I fixed the file collision but forgot to go back and
clean up text references, will fix.
Thanks,
Kristofer
|
|
||
| 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 |
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Oct 12, 2026 at 12:07:57PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
[snip]
> 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.
Okay, so this is getting better the larger the trees.
> 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.
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.
> diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> index abef02debd..a1d2e9709c 100644
> --- a/Documentation/technical/connectivity-check.adoc
> +++ b/Documentation/technical/connectivity-check.adoc
> +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)
So based on this, `verify()` already knows which commits it will be
checking for connectivity. So wouldn't it be possible to fall back to
the original algortihm here in case we see more than N commits?
> diff --git a/tree-verify.c b/tree-verify.c
> new file mode 100644
> index 0000000000..b381a50102
> --- /dev/null
> +++ b/tree-verify.c
> @@ -0,0 +1,316 @@
> +#include "git-compat-util.h"
> +#include "commit.h"
> +#include "gettext.h"
> +#include "hex.h"
> +#include "khash.h"
> +#include "object.h"
> +#include "odb.h"
> +#include "oid-array.h"
> +#include "oidset.h"
> +#include "tree.h"
> +#include "tree-walk.h"
> +#include "tree-verify.h"
> +#include "packfile.h"
> +#include "trace2.h"
> +
> +enum tree_state {
> + TREE_UNTRUSTED = 0,
> + TREE_TRUSTED = 1,
> + TREE_EXPANDED = 2,
> +};
> +
> +KHASH_INIT(oid_tree, struct object_id, unsigned char, 1,
The values are `enum tree_state`s, right? Why don't we use that type
here?
> + oidhash_by_value, oideq_by_value)
> +
> +static enum tree_state tree_map_get(kh_oid_tree_t *m,
> + const struct object_id *oid)
> +{
> + khint_t pos = kh_get_oid_tree(m, *oid);
> + if (pos == kh_end(m))
> + return TREE_UNTRUSTED;
> + return kh_val(m, pos);
> +}
> +
> +static void tree_map_add(kh_oid_tree_t *m, const struct object_id *oid,
> + enum tree_state state)
> +{
> + int added;
> + khint_t pos = kh_put_oid_tree(m, *oid, &added);
> + if (added)
> + kh_val(m, pos) = state;
> + else if (state > kh_val(m, pos))
> + kh_val(m, pos) = state;
> +}
> +
> +struct work_item {
> + struct name_entry entry;
> + struct oid_array parent_trees;
> +};
> +
> +struct verify_state {
> + kh_oid_tree_t *trees;
> + struct oidset trusted_blobs;
> + int trees_loaded;
> + int blobs_checked;
These probably should be `size_t`.
> + int exclude_promisor_objects;
> +};
> +
> +/*
> + * Merge-walk the work list against one base tree entry, recording
> + * 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)
> +{
> + 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);
> + 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);
> + return wi + 1;
> + }
> + return wi;
> +}
> +
> +static void verify_blob(struct repository *repo,
> + const struct object_id *oid,
> + struct verify_state *vs)
> +{
> + int type;
> +
> + if (oidset_contains(&vs->trusted_blobs, oid))
> + return;
> +
> + vs->blobs_checked++;
> + type = odb_read_object_info(repo->objects, oid, NULL);
> + if (type == OBJ_BLOB) {
> + oidset_insert(&vs->trusted_blobs, oid);
> + return;
> + }
> + if (type >= 0)
> + die(_("object %s is a %s, not a %s"),
> + oid_to_hex(oid), type_name(type), "blob");
I'd love not having all these calls to `die()`, as this logic is
essentially part of a library, and dying inside a library is a bad
smell. We do this everywhere else in Git, too, but we're slowly getting
rid of this as part of our libification efforts.
> + if (vs->exclude_promisor_objects &&
> + is_promisor_object(repo, oid))
> + return;
> + die(_("missing blob object '%s'"), oid_to_hex(oid));
Maybe the object is missing, but it may just as well be corrupt. So this
should probably say "unable to read".
> +}
> +
> +static void *read_tree_object(struct object_database *odb,
> + const struct object_id *oid,
> + 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");
> + }
> + return buf;
> +}
> +
> +static void verify_tree(struct repository *repo,
> + const struct object_id *new_tree_oid,
> + const struct oid_array *base_trees,
I think `base_trees` is a bit of a confusing term, as I think those
always are the parent's trees, right? Should we maybe name it
accordingly?
> + struct verify_state *vs, int depth)
> +{
> + 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;
Nit: this should be a `bool`.
> + 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);
> +
> + while (tree_entry(&desc, &entry)) {
> + if (S_ISGITLINK(entry.mode))
> + continue;
> + if (S_ISDIR(entry.mode)) {
> + if (tree_map_get(vs->trees, &entry.oid) >= TREE_TRUSTED)
> + continue;
> + need_subtree_bases = 1;
> + } else {
> + 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++;
> + }
Okay, so first we gather all the tree entries that need verification
becaues we don't trust them yet.
> +
> + if (!nr_work) {
> + free(tree_buf);
> + goto done;
> + }
> +
> + 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;
> +
> + if (expanded && !need_subtree_bases)
> + 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));
> + }
> +
> + vs->trees_loaded++;
> + init_tree_desc(&base_desc, base_oid, base_buf, base_size);
> +
> + while (tree_entry(&base_desc, &scan_entry)) {
> + if (S_ISGITLINK(scan_entry.mode))
> + continue;
> +
> + 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);
> + }
> + }
> +
> + if (!expanded)
> + tree_map_add(vs->trees, base_oid, TREE_EXPANDED);
> +
> + free(base_buf);
> + }
Then we expand all of our parent trees to exand the trust boundary. I
think this part is rather non-obvious though, and could really use some
code comments to explain what's even going on here.
> + 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);
And here we then recurse. The recursion isn't unbounded, but we may go
up to 2048 layers deep here. We already do this recursively in other
places, but I do wonder whether we can maybe avoid the recursion and
instead use a work queue so that we don't bust the stack.
> + else
> + verify_blob(repo, &work[i].entry.oid, vs);
> + }
> +
> + free(tree_buf);
> +
> +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);
> +}
> +
> +static void verify_commit_tree(struct repository *repo,
> + struct commit *commit,
> + struct verify_state *vs)
> +{
> + struct oid_array base_trees = OID_ARRAY_INIT;
> + const struct object_id *tree_oid;
> + struct commit_list *p;
> +
> + /*
> + * Parent trees are trusted: boundary parents are already
> + * connected, and earlier incoming parents were verified
> + * first due to the topological processing order.
> + */
Okay. Parents are trusted because we go from oldest to newest in
topological order.
> + for (p = commit->parents; p; p = p->next) {
> + parse_commit_or_die(p->item);
This part here may die in case the repository is a partial clone with
missing commits because we enforce that `fetch_if_missing` is disabled.
> + 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);
And we only mark those trees as TRUSTED, not EXPANDED, as we didn't read
the trees themselves.
> + }
> +
> + 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);
And this function here recursively verifies the root tree itself.
> + oid_array_clear(&base_trees);
> +}
> +
> +void 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;
> +
> + 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;
> +
> + /*
> + * Ancestors must be verified before descendants so that parent
> + * trees can be trusted without re-verification. Sort explicitly
> + * rather than relying on the caller's ordering.
> + *
> + * sort_in_topological_order() silently drops cycle members,
> + * so explicitly check if the size has changed.
> + */
> + 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"));
> +
> + *commits = commit_list_reverse(*commits);
> +
> + for (iter = *commits; iter; iter = iter->next)
> + verify_commit_tree(repo, iter->item, &vs);
> +
> + kh_destroy_oid_tree(vs.trees);
> + oidset_clear(&vs.trusted_blobs);
> + trace2_data_intmax("connectivity", repo,
> + "trees_loaded", vs.trees_loaded);
> + trace2_data_intmax("connectivity", repo,
> + "blobs_checked", vs.blobs_checked);
> +}
Thanks!
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 15 Oct 2026 at 08:55, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 12, 2026 at 12:07:57PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <krka@spotify.com>
> [snip]
> > 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.
>
> Okay, so this is getting better the larger the trees.
>
> > 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.
>
> 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.
>
> > diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> > index abef02debd..a1d2e9709c 100644
> > --- a/Documentation/technical/connectivity-check.adoc
> > +++ b/Documentation/technical/connectivity-check.adoc
> > +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)
>
> So based on this, `verify()` already knows which commits it will be
> checking for connectivity. So wouldn't it be possible to fall back to
> the original algortihm here in case we see more than N commits?
Yes, but which N? I think it would depend on the shape of the
repository too -- size of object tree, merge-heavy vs linear.
> > +enum tree_state {
> > + TREE_UNTRUSTED = 0,
> > + TREE_TRUSTED = 1,
> > + TREE_EXPANDED = 2,
> > +};
> > +
> > +KHASH_INIT(oid_tree, struct object_id, unsigned char, 1,
>
> The values are `enum tree_state`s, right? Why don't we use that type
> here?
I tried that first, but the natural size of enum is larger than unsigned char,
so this reduces the memory usage.
> > +struct verify_state {
> > + kh_oid_tree_t *trees;
> > + struct oidset trusted_blobs;
> > + int trees_loaded;
> > + int blobs_checked;
>
> These probably should be `size_t`.
Sure, I can change that. Or intmax_t since we'll need to cast
to that later anyway for the trace2 output?
> > + if (type >= 0)
> > + die(_("object %s is a %s, not a %s"),
> > + oid_to_hex(oid), type_name(type), "blob");
>
> I'd love not having all these calls to `die()`, as this logic is
> essentially part of a library, and dying inside a library is a bad
> smell. We do this everywhere else in Git, too, but we're slowly getting
> rid of this as part of our libification efforts.
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.
>
> > + if (vs->exclude_promisor_objects &&
> > + is_promisor_object(repo, oid))
> > + return;
> > + die(_("missing blob object '%s'"), oid_to_hex(oid));
>
> Maybe the object is missing, but it may just as well be corrupt. So this
> should probably say "unable to read".
Hmm yes, perhaps. I will need to dig into that more.
>
> > +}
> > +
> > +static void *read_tree_object(struct object_database *odb,
> > + const struct object_id *oid,
> > + 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");
> > + }
> > + return buf;
> > +}
> > +
> > +static void verify_tree(struct repository *repo,
> > + const struct object_id *new_tree_oid,
> > + const struct oid_array *base_trees,
>
> I think `base_trees` is a bit of a confusing term, as I think those
> always are the parent's trees, right? Should we maybe name it
> accordingly?
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.
> > + int need_subtree_bases = 0;
>
> Nit: this should be a `bool`.
True, will fix. I think I got confused because the code base
use both styles in different places.
> > +
> > + if (!nr_work) {
> > + free(tree_buf);
> > + goto done;
> > + }
> > +
> > + 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;
> > +
> > + if (expanded && !need_subtree_bases)
> > + 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));
> > + }
> > +
> > + vs->trees_loaded++;
> > + init_tree_desc(&base_desc, base_oid, base_buf, base_size);
> > +
> > + while (tree_entry(&base_desc, &scan_entry)) {
> > + if (S_ISGITLINK(scan_entry.mode))
> > + continue;
> > +
> > + 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);
> > + }
> > + }
> > +
> > + if (!expanded)
> > + tree_map_add(vs->trees, base_oid, TREE_EXPANDED);
> > +
> > + free(base_buf);
> > + }
>
> Then we expand all of our parent trees to exand the trust boundary. I
> think this part is rather non-obvious though, and could really use some
> code comments to explain what's even going on here.
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.
>
> > + 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);
>
> And here we then recurse. The recursion isn't unbounded, but we may go
> up to 2048 layers deep here. We already do this recursively in other
> places, but I do wonder whether we can maybe avoid the recursion and
> instead use a work queue so that we don't bust the stack.
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.
> > + for (p = commit->parents; p; p = p->next) {
> > + parse_commit_or_die(p->item);
>
> This part here may die in case the repository is a partial clone with
> missing commits because we enforce that `fetch_if_missing` is disabled.
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!
KristoferThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Oct 15, 2026 at 11:51:46AM +0200, Kristofer Karlsson wrote:
> On Thu, 15 Oct 2026 at 08:55, Patrick Steinhardt <ps@pks.im> wrote:
> > On Mon, Oct 12, 2026 at 12:07:57PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > > From: Kristofer Karlsson <krka@spotify.com>
[snip]
> > > 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.
> >
> > 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.
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?
> > > diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> > > index abef02debd..a1d2e9709c 100644
> > > --- a/Documentation/technical/connectivity-check.adoc
> > > +++ b/Documentation/technical/connectivity-check.adoc
[snip]
> > > +struct verify_state {
> > > + kh_oid_tree_t *trees;
> > > + struct oidset trusted_blobs;
> > > + int trees_loaded;
> > > + int blobs_checked;
> >
> > These probably should be `size_t`.
>
> Sure, I can change that. Or intmax_t since we'll need to cast
> to that later anyway for the trace2 output?
Hm, okay. Not a fan of using signed integers as counters, but so be it.
> > > + if (type >= 0)
> > > + die(_("object %s is a %s, not a %s"),
> > > + oid_to_hex(oid), type_name(type), "blob");
> >
> > I'd love not having all these calls to `die()`, as this logic is
> > essentially part of a library, and dying inside a library is a bad
> > smell. We do this everywhere else in Git, too, but we're slowly getting
> > rid of this as part of our libification efforts.
>
> 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.
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.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 15 Oct 2026 at 13:50, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > 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.
>
> 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?
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.
> > > These probably should be `size_t`.
> >
> > Sure, I can change that. Or intmax_t since we'll need to cast
> > to that later anyway for the trace2 output?
>
> Hm, okay. Not a fan of using signed integers as counters, but so be it.
No worries, I will switch to size_t instead.
>
> > > > + if (type >= 0)
> > > > + die(_("object %s is a %s, not a %s"),
> > > > + oid_to_hex(oid), type_name(type), "blob");
> > >
> > > I'd love not having all these calls to `die()`, as this logic is
> > > essentially part of a library, and dying inside a library is a bad
> > > smell. We do this everywhere else in Git, too, but we're slowly getting
> > > rid of this as part of our libification efforts.
> >
> > 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.
>
> 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.
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,
KristoferThere was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Fri, Oct 16, 2026 at 01:50:21PM +0200, Kristofer Karlsson wrote:
> On Thu, 15 Oct 2026 at 13:50, Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > 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.
> >
> > 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?
>
> 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.
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.
> 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.
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]
> > > > > + if (type >= 0)
> > > > > + die(_("object %s is a %s, not a %s"),
> > > > > + oid_to_hex(oid), type_name(type), "blob");
> > > >
> > > > I'd love not having all these calls to `die()`, as this logic is
> > > > essentially part of a library, and dying inside a library is a bad
> > > > smell. We do this everywhere else in Git, too, but we're slowly getting
> > > > rid of this as part of our libification efforts.
> > >
> > > 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.
> >
> > 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.
>
> 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, I appreciate it when folks take their time :)
Patrick
| @@ -1,3 +1,23 @@ | |||
| transfer.connectivityCheck:: | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Mon, Oct 12, 2026 at 12:07:56PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> 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.
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.
PatrickThere was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Thu, 15 Oct 2026 at 08:55, Patrick Steinhardt <ps@pks.im> wrote:
>
> > The rev-list flag is accepted but has no effect yet; the incremental
> > verification will be implemented in the next commit.
>
> 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.
Hm yes, I can try to do that.
Thanks,
Kristofer
|
There was a status update in the "Cooking" section about the branch The connectivity check in 'git fetch' and 'git receive-pack' has been optimized with an alternative incremental mode, available via 'transfer.connectivityCheck=incremental'. Instead of walking the full object closure of the boundary commits, it compares the trees of incoming commits against their trusted parent trees, skipping unchanged subtrees. Expecting a reroll. cf. <CAL71e4NA3SH8V_haxJFeYCBmKk8+96j7ckxLDZA7+XMNcVfdKg@mail.gmail.com> source: <pull.2211.v3.git.1791806877.gitgitgadget@gmail.com> |
Add Documentation/technical/connectivity-check.adoc describing the connectivity invariant and the full connectivity check. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Add benchmarks for the rev-list connectivity check. Each fixture varies one dimension: total tree size, incoming commit count, or files changed per commit. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
|
There was a status update in the "Cooking" section about the branch The connectivity check in 'git fetch' and 'git receive-pack' has been optimized with an alternative incremental mode, available via 'transfer.connectivityCheck=incremental'. Instead of walking the full object closure of the boundary commits, it compares the trees of incoming commits against their trusted parent trees, skipping unchanged subtrees. Expecting a reroll. cf. <CAL71e4NA3SH8V_haxJFeYCBmKk8+96j7ckxLDZA7+XMNcVfdKg@mail.gmail.com> source: <pull.2211.v3.git.1791806877.gitgitgadget@gmail.com> |
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`. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
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. Suggested-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
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 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.
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.
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 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.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
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.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
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.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
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:
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 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: 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 <krka@spotify.com>
The incremental connectivity check pays a per-commit cost. For
every incoming commit it reads the new trees along the changed
paths, and the trees at the same paths in its parents to compare
against. A parent tree is often read again even though an earlier
commit already read it: on git.git, about a third of the tree reads
for 2000 incoming commits are such re-reads. The full check
instead pays a mostly fixed cost for walking the boundary trees,
so with many incoming commits the incremental check becomes the
slower one (see the previous commit). Users who mostly
receive small pushes but occasionally a large one should not have
to choose between the two.
Add an "auto" value for transfer.connectivityCheck that picks the
mode based on the incoming history. When the revision walk is
limited, which is the case whenever the receiving repository has
references, the incoming commit list is already computed by
limit_list(), so counting it is cheap. "auto" then uses the
incremental check for up to 1000 incoming commits, and the full
check above that.
The cutoff is a fixed constant based on the p5413 fixture in the
previous commit, where breakeven is between 500 and 3000 commits.
It is conservative for repositories with large trees, where the
fixed cost of the full check is higher. Measured on real
repositories (rev-list against a single boundary commit, mean
of at least 10 runs):
repository files commits full incr. full/incr
internal A 556K 1000 1.71s 0.61s 2.8x
internal A 556K 4000 2.78s 2.83s 1.0x
internal B 288K 1000 1.78s 0.88s 2.0x
linux 95K 598 0.33s 0.12s 2.7x
linux 95K 1728 0.56s 0.35s 1.6x
git 4.9K 984 0.22s 0.27s 0.8x
With small trees incremental can be slower within the cutoff,
but both modes are fast enough there (0.22s vs 0.27s for git
above) that the difference does not matter in practice.
No single cutoff is optimal for every repository, so it can be set
with "auto=<cutoff>"; plain "auto" uses 1000. A malformed cutoff
is treated like any other unknown value: it falls back to "full"
with a warning.
Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
tree-diff-connectivity-v1-clean
branch
from
October 21, 2026 08:54
3ecd788 to
f603196
Compare
Author
|
/submit |
|
Submitted as pull.2211.v4.git.1792581744.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This series adds an incremental 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:
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 boundaryN,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 HEAD
isolating the tree verification cost from the boundary-finding
cost:
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:
mismatches, malformed trees (unparseable, mid-tree
corruption), and corrupt blobs (incremental only, since the
full check does not read blobs)
subtrees are skipped, subtree moves, merge parent boundaries
trees, missing promised parent commits, verification of
local commits
repository use the full check, "auto" and its cutoff, and
unknown config values
(t6700)
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_repositoryinconnected.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
be20128 (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!).
baseline, config plumbing + rev-list flag, then the
incremental implementation.
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.
calling get_commit_tree_oid(commit) twice.
(t5412 was taken by another topic). Renumbered test and perf
files to t5413/p5413.
Changes since v1
verify_commit_tree(), dying with a clear message instead of
a NULL dereference. (Thanks Junio for catching this)
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)
during the incremental check.
to avoid adding new strings for localization.
Thanks,
Kristofer
cc: Kristofer Karlsson krka@spotify.com
cc: Patrick Steinhardt ps@pks.im