Repository navigation
connected: add incremental connectivity check #2211
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,33 @@ | ||
| 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,
KristoferThere 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.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 |
||
| Choose which algorithm to use for the connectivity check | ||
| performed during object transfer operations such as | ||
| linkgit:git-fetch[1] and linkgit:git-receive-pack[1]. | ||
| The connectivity check verifies that all objects reachable | ||
| from the incoming tips are available locally or, in a partial | ||
| clone, promised by a promisor remote. | ||
| The variants are as follows: | ||
| + | ||
| -- | ||
|
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:
> -static void parse_connectivity_check(struct repository *repo UNUSED,
> +static void parse_connectivity_check(struct repository *repo,
> const char *arg,
> struct connectivity_check *check)
> {
> if (!strcmp(arg, "full"))
> check->mode = CONNECTIVITY_CHECK_FULL;
> + else if (!strcmp(arg, "incremental"))
> + check->mode = CONNECTIVITY_CHECK_INCREMENTAL;
> else
> die(_("unknown --connectivity-check mode '%s'"), arg);
> +
> + if (check->mode != CONNECTIVITY_CHECK_FULL)
> + repo->fetch_if_missing = 0;
> }
This assumes that everybody that is different from full want to
avoid lazy fetching. It may be true right now as you have only full
and incremental, but it somehow feels like a trap waiting to bite
future developers.
> @@ -992,6 +999,29 @@ int cmd_rev_list(int argc,
>
> prepare_maximal_independent(&revs);
>
> + /*
> + * Without a boundary of negative revisions, as in a clone or a
> + * push into an empty repository, every incoming commit is new.
> + * The incremental check would still work, but it would verify
> + * the whole object graph like the full check, with extra
> + * per-commit overhead on top, so use the full check.
> + */
> + if (!revs.limited)
> + connectivity_check.mode = CONNECTIVITY_CHECK_FULL;
> +
> + if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) {
> + struct commit *commit;
> + struct commit_list *new_commits = NULL;
> +
> + while ((commit = get_revision(&revs)) != NULL)
> + commit_list_insert(commit, &new_commits);
> +
> + if (verify_commits_incremental(repo, &new_commits,
> + revs.exclude_promisor_objects))
> + die(_("connectivity check failed"));
> + commit_list_free(new_commits);
> + }
> +
We only call verify_commits_incremental() when check.mode says INCREMENTAL,
which makes sense.
> +static int verify_blob(struct repository *repo,
> + const struct object_id *oid,
> + struct verify_state *vs)
> +{
> + int type;
> +
> + if (oidset_contains(&vs->trusted_blobs, oid))
> + return 0;
> +
> + vs->blobs_checked++;
> + type = odb_read_object_info(repo->objects, oid, NULL);
I wonder if this needs to be _extended() kind that passes a flag bit
to avoid fetching from promisors. repo->fetch_if_missing that is
set does prevent lazy fetching, but I wonder if it is better to be
more explicit.
> +int verify_commits_incremental(struct repository *repo,
> + struct commit_list **commits,
> + int exclude_promisor_objects)
> +{
> + struct verify_state vs = { 0 };
> + struct commit_list *iter;
> + unsigned nr_before;
> + int ret = 0;
> +
> + if (repo->fetch_if_missing)
> + BUG("verify_commits_incremental must not be called "
> + "with fetch_if_missing set");
And we make sure that the caller flipped the bit before they decide
to call us, which makes sense.
There was a problem hiding this comment. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 21 Oct 2026 at 22:18, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > -static void parse_connectivity_check(struct repository *repo UNUSED,
> > +static void parse_connectivity_check(struct repository *repo,
> > const char *arg,
> > struct connectivity_check *check)
> > {
> > if (!strcmp(arg, "full"))
> > check->mode = CONNECTIVITY_CHECK_FULL;
> > + else if (!strcmp(arg, "incremental"))
> > + check->mode = CONNECTIVITY_CHECK_INCREMENTAL;
> > else
> > die(_("unknown --connectivity-check mode '%s'"), arg);
> > +
> > + if (check->mode != CONNECTIVITY_CHECK_FULL)
> > + repo->fetch_if_missing = 0;
> > }
>
> This assumes that everybody that is different from full want to
> avoid lazy fetching. It may be true right now as you have only full
> and incremental, but it somehow feels like a trap waiting to bite
> future developers.
Good point, and I think I could simplify this to always set
fetch_if_missing = 0 for any type of connectivity check, since
no connectivity check should be allowed to trigger remote
calls.
I think the reason this was not in place before was that
rev-list did not know that the intent was a connectivity
check but now by passing this new --connectivity-check
it always does.
> > +static int verify_blob(struct repository *repo,
> > + const struct object_id *oid,
> > + struct verify_state *vs)
> > +{
> > + int type;
> > +
> > + if (oidset_contains(&vs->trusted_blobs, oid))
> > + return 0;
> > +
> > + vs->blobs_checked++;
> > + type = odb_read_object_info(repo->objects, oid, NULL);
>
> I wonder if this needs to be _extended() kind that passes a flag bit
> to avoid fetching from promisors. repo->fetch_if_missing that is
> set does prevent lazy fetching, but I wonder if it is better to be
> more explicit.
Yes, and an earlier internal prototype (that I never sent) did
this. Blob and tree reads are easy to switch to
odb_read_object_info_extended() with OBJECT_INFO_SKIP_FETCH_OBJECT,
but parsing the parent commits goes through
repo_parse_commit_gently(), which has no such flag. That would
need a no-fetch flag passed down through commit parsing, which I
think is not too bad on its own.
But even then, the revision walk around the verifier and the full
check would still depend on fetch_if_missing, so the verifier alone
would not let us drop it. I think it is better to refactor that all
at once in a separate follow-up, since this series is already big
enough. However, if you would prefer to have it now already, I
could try to add a new commit on top for the next reroll.
(My earlier prototype was more complex because it ran in-process
rather than via rev-list, so it also had to avoid die(), which
triggered even more low level function changes. That does not
apply here though, fortunately.)
Thanks for the review,
KristoferThere was a problem hiding this comment. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Thu, 22 Oct 2026 at 09:00, Kristofer Karlsson <krka@spotify.com> wrote:
>
> On Wed, 21 Oct 2026 at 22:18, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > I wonder if this needs to be _extended() kind that passes a flag bit
> > to avoid fetching from promisors. repo->fetch_if_missing that is
> > set does prevent lazy fetching, but I wonder if it is better to be
> > more explicit.
>
> Yes, and an earlier internal prototype (that I never sent) did
> this. Blob and tree reads are easy to switch to
> odb_read_object_info_extended() with OBJECT_INFO_SKIP_FETCH_OBJECT,
> but parsing the parent commits goes through
> repo_parse_commit_gently(), which has no such flag. That would
> need a no-fetch flag passed down through commit parsing, which I
> think is not too bad on its own.
Sorry, I think I was wrong about this part -- commit parsing
never fetches. I think this means my initial problems were
partly to die(). I will re-explore this and see if eliminating
fetch_if_missing = 0 is less work than I first thought. |
||
| `full` (default);; | ||
| Walk the full object closure of the boundary commits. | ||
| `incremental`;; | ||
| Verify incoming commits by diffing their trees against parent | ||
| trees, recursively descending only into entries that differ. | ||
| The largest benefits occur when incoming commits change a | ||
| small fraction of a large tree closure, and it can be slower | ||
| than `full` when there are many incoming commits. | ||
| Falls back to `full` for clones, pushes into an empty | ||
| repository and deepening fetches. | ||
| `auto`;; | ||
| `auto=<cutoff>`;; | ||
| Choose between `full` and `incremental` with a heuristic. | ||
| Currently `incremental` is used when there are at most | ||
| _<cutoff>_ incoming commits (1000 by default), and `full` | ||
| otherwise. The heuristic may change in future versions. | ||
| -- | ||
| + | ||
| Unknown values are treated as `full`, with a warning. | ||
|
|
||
| transfer.credentialsInUrl:: | ||
| A configured URL can contain plaintext credentials in the form | ||
| `<protocol>://<user>:<password>@<domain>/<path>`. You may want | ||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,259 @@ | ||
| 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, Sep 28, 2026 at 01:02:31PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/Documentation/technical/connectivity-check.adoc b/Documentation/technical/connectivity-check.adoc
> new file mode 100644
> index 0000000000..d20bff6af6
> --- /dev/null
> +++ b/Documentation/technical/connectivity-check.adoc
> @@ -0,0 +1,109 @@
> +Connectivity checking
> +=====================
> +
> +After receiving new objects via fetch, push (receive-pack), clone,
> +or bundle, Git verifies that the new reference tips do not leave
> +the repository in a state where reachable objects are missing.
> +This verification is called the connectivity check.
> +
> +Connectivity invariant
> +----------------------
> +
> +A repository is connected when every object reachable from its
> +references is available locally (with exceptions noted below).
Right. I think it would also be important to spell out the reverse of
this, which is that nothing can be assumed about objects that aren't
reachable by any reference. So even if an object already exists in the
object database, it is not safe to assume that it is fully connected
unless it is referenced.
> +The connectivity check maintains this invariant when references
> +are updated. It trusts the existing connected state and verifies
Nit: it's basically already implicit, but I'd clarify that "existing
connected state" is again just the connected state of objects reachable
from reference tips. So maybe "It trusts that all objects reachable from
references are already fully connected and verifies..."
> +that the new reference tips do not introduce references to
> +unavailable objects. Verification is permitted to stop when it
> +reaches objects already reachable from trusted existing
> +references, since their closure is already connected. These
> +trusted references include local references and references from
> +alternate object stores.
> +
> +Without this check, a truncated or corrupted transfer could leave
> +a repository in a state where later history walks encounter
> +missing objects.
> +
> +Exceptions
> +~~~~~~~~~~
> +
> +Gitlink entries (submodule references) are excluded from
> +connectivity checking. Their target objects belong to a separate
> +repository.
> +
> +In partial clones, objects promised by a promisor remote are
> +accepted as connected without requiring local existence. The
> +check excludes promisor objects from traversal so that it does
> +not trigger on-demand fetches for them.
> +
> +Full connectivity check
> +-----------------------
> +
> +`check_connected()` (see `connected.c`) normally performs the
I'm always a bit hesitant to directly refer to code in our docs. We
should either make this documentation part of "connected.c" directly, or
we should not refer to code. Otherwise, chances that this documentation
grows stale is very high.
> +connectivity check using a `rev-list` subprocess, feeding the
> +new reference tips via stdin. A normal invocation is roughly:
> +
> + git rev-list --objects --stdin --not --all --quiet
> + --alternate-refs [--exclude-promisor-objects]
> +
> +When promisor remotes are configured, `check_connected()` first
> +attempts a fast path based on promisor packfiles. If it falls
> +back to the `rev-list` check, `--exclude-promisor-objects` is
> +added so that the traversal does not trigger on-demand fetches.
> +
> +Consider the following graph after a fetch, where all reference
> +tips point directly to commits. For simplicity, only local
> +references appear on the already-connected side; alternate refs
> +play the same role. N3 is a merge commit:
> +
> + /-------------L2
> + /
> + C1---B1---C2---B2-----L1
> + \ \
> + N1 N3---T2
> + \ /
> + N2-----------T1
> +
> + L1, L2: local refs
> + T1, T2: incoming tips (new refs)
> + N1, N2, N3: incoming commits (N3 is a merge)
> + B1, B2: boundary commits (already connected)
> + C1, C2: already connected (but not boundary)
> +
> +The incoming set is the commits reachable from the incoming
> +tips but not from the already-connected side. Boundary commits
> +are the already-connected commits at the edge of that set. Here
> +B1 is an ancestor of B2, which happens when incoming branches
> +fork at different depths in the existing history.
> +
> +The check proceeds in three phases:
> +
> +1. Walk from the incoming tips (T1, T2) against the trusted
> + refs (L1, L2) to find the incoming set ({N1, N2, N3, T1, T2}).
> +
> +2. Walk the trees of the boundary commits (B1, B2) and mark
> + those objects uninteresting. These trees are already trusted
> + because their commits are on the already-connected side.
> +
> +3. Walk the trees of each incoming commit and verify that every
> + referenced object is connected, stopping at objects already
> + marked uninteresting in phase 2.
I feel like these phases here basically just explain how revision walks
work without adding any more details that are specifically relevant to
the connectivity check.
> +Deepening fetches
> +~~~~~~~~~~~~~~~~~
> +
> +For deepening fetches (where the shallow boundary moves), the
> +full check omits `--not --all`. There is no existing-reference
> +boundary at which the walk can stop. Instead, traversal follows
> +the effective shallow boundary supplied for the deepened
> +repository. The new content may be below the old shallow
> +boundary even when the tips themselves have not changed.
> +
> +Non-commit tips
> +~~~~~~~~~~~~~~~
> +
> +When a new reference points to a non-commit object, such as a
> +tag, tree, or blob, that object is not part of the commit walk.
> +These non-commit tips are handled by the subsequent object
> +traversal.
Huh, what subsequent object traversal? This part puzzles me a bit.
PatrickThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Patrick Steinhardt <ps@pks.im> writes:
>> +Full connectivity check
>> +-----------------------
>> +
>> +`check_connected()` (see `connected.c`) normally performs the
>
> I'm always a bit hesitant to directly refer to code in our docs. We
> should either make this documentation part of "connected.c" directly, or
> we should not refer to code. Otherwise, chances that this documentation
> grows stale is very high.
This is totally outside the topic of documentation updates, but it
makes me wonder if we should pay attention to connectivity roots
other than refs (like index entries) that we use when we run fsck.There was a problem hiding this comment. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Mon, Oct 05, 2026 at 12:17:27PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> +Full connectivity check
> >> +-----------------------
> >> +
> >> +`check_connected()` (see `connected.c`) normally performs the
> >
> > I'm always a bit hesitant to directly refer to code in our docs. We
> > should either make this documentation part of "connected.c" directly, or
> > we should not refer to code. Otherwise, chances that this documentation
> > grows stale is very high.
>
> This is totally outside the topic of documentation updates, but it
> makes me wonder if we should pay attention to connectivity roots
> other than refs (like index entries) that we use when we run fsck.
Hmm, I'm not sure. I guess performance of the connectivity check is
typically an issue on the server side only, much less so on the client
side. And the server would of course typically not even have an index
entry at all. Same for reflogs, at least in many setups.
I also wonder whether that'd really speed things up if we add more data
sources. At GitLab we typically have the problem that we have too many
connectivity roots with refs alone, and that is making the whole check
painfully slow in some repositories. So adding more connectivity roots
to it would probably be counterproductive.
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 07:59, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 05, 2026 at 12:17:27PM -0700, Junio C Hamano wrote:
> >
> > This is totally outside the topic of documentation updates, but it
> > makes me wonder if we should pay attention to connectivity roots
> > other than refs (like index entries) that we use when we run fsck.
>
> Hmm, I'm not sure. I guess performance of the connectivity check is
> typically an issue on the server side only, much less so on the client
> side. And the server would of course typically not even have an index
> entry at all. Same for reflogs, at least in many setups.
>
> I also wonder whether that'd really speed things up if we add more data
> sources. At GitLab we typically have the problem that we have too many
> connectivity roots with refs alone, and that is making the whole check
> painfully slow in some repositories. So adding more connectivity roots
> to it would probably be counterproductive.
I have some followup work in this area (but I want to keep focus)
and it's indeed true that number of refs used as seeds is also an
important part of the problem.
I see connectivity and reachability as two separate things,
though the current implementation has an invariant based on
reachability implying (e.g. requiring) connectivity.
I think we could build up a stronger connectivity invariant
for the common case, where we can cheaply check if an object
is connectivity-closed regardless of its reachability state.
One idea is to utilize the fact that MIDX maps out a very
specific set of packs, so we could compute which set of packs
form a connectivity closure (and cheaply update that information
as the MIDX changes).
Another idea would be to dynamically load refs as seeds
in parallel with walking the graph. This creates a tradeoff
between how many objects we need to check and how many refs
we need to load (if we load fewer refs we just need to walk
further to reach common ground).
Sorry, this is a bit hand-wave:y but my point is that I think
regardless of how this is followed up to solve the bigger
problems, I think optimizing the object-tree traversal is a
useful first step.
Thanks,
KristoferThere was a problem hiding this comment. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): Thanks, it's clear I'll need to do some more wordsmithing to
improve the clarity here. I'll reroll/rewrite parts of it in
some way (and aligned with the answers below).
On Mon, 5 Oct 2026 at 09:59, Patrick Steinhardt <ps@pks.im> wrote:
>
> > +A repository is connected when every object reachable from its
> > +references is available locally (with exceptions noted below).
>
> Right. I think it would also be important to spell out the reverse of
> this, which is that nothing can be assumed about objects that aren't
> reachable by any reference. So even if an object already exists in the
> object database, it is not safe to assume that it is fully connected
> unless it is referenced.
Fair, I can make that more explicit in the text.
>
> > +The connectivity check maintains this invariant when references
> > +are updated. It trusts the existing connected state and verifies
>
> Nit: it's basically already implicit, but I'd clarify that "existing
> connected state" is again just the connected state of objects reachable
> from reference tips. So maybe "It trusts that all objects reachable from
> references are already fully connected and verifies..."
Agreed, I will make that more clear.
> > +Full connectivity check
> > +-----------------------
> > +
> > +`check_connected()` (see `connected.c`) normally performs the
>
> I'm always a bit hesitant to directly refer to code in our docs. We
> should either make this documentation part of "connected.c" directly, or
> we should not refer to code. Otherwise, chances that this documentation
> grows stale is very high.
Good point, I will remove it and make the logic more self contained.
> > +The check proceeds in three phases:
> > +
> > +1. Walk from the incoming tips (T1, T2) against the trusted
> > + refs (L1, L2) to find the incoming set ({N1, N2, N3, T1, T2}).
> > +
> > +2. Walk the trees of the boundary commits (B1, B2) and mark
> > + those objects uninteresting. These trees are already trusted
> > + because their commits are on the already-connected side.
> > +
> > +3. Walk the trees of each incoming commit and verify that every
> > + referenced object is connected, stopping at objects already
> > + marked uninteresting in phase 2.
>
> I feel like these phases here basically just explain how revision walks
> work without adding any more details that are specifically relevant to
> the connectivity check.
Yes, though I think this is important for understanding how the
connectivity-check is implemented to see the relation to the
rev-list walk. I have some hope that this dependency can go
away in the future, since the general rev-list function is not
necessarily the most optimal way to reason about connectivity.
That said, I am happy to remove this part of it's not deemed
useful.
> > +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.
>
> Huh, what subsequent object traversal? This part puzzles me a bit.
Good point, this is meant to reference back to the rev-list
implementation but that's not very clear. And like
the previous feedback, perhaps this should also go away
(or be reworded in a more self contained way and less tied
to rev-list)
Thanks,
KristoferThere 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 |
||
| ===================== | ||
|
|
||
| After receiving new objects via fetch, push (receive-pack), clone, | ||
| or bundle, Git verifies that the new reference tips do not leave | ||
| the repository in a state where reachable objects are missing. | ||
| This verification is called the connectivity check. | ||
|
|
||
| Connectivity invariant | ||
| ---------------------- | ||
|
|
||
| A repository is connected when every object reachable from its | ||
| references is available locally (with exceptions noted below). | ||
| The reverse does not hold: objects that exist in the object | ||
| database but are not reachable from any reference may be | ||
| incomplete or lack a connected closure. | ||
|
|
||
| The connectivity check maintains this invariant when references | ||
| are updated. It trusts that all objects reachable from existing | ||
| reference tips are already fully connected and verifies that the | ||
| new reference tips do not introduce references to unavailable | ||
| objects. Verification is permitted to stop when it reaches | ||
| objects already reachable from existing references, since their | ||
| closure is already connected. These trusted references include | ||
| local references and references from alternate object stores. | ||
|
|
||
| Without this check, a truncated or corrupted transfer could leave | ||
| a repository in a state where later history walks encounter | ||
| missing objects. | ||
|
|
||
| Exceptions | ||
| ~~~~~~~~~~ | ||
|
|
||
| Gitlink entries (submodule references) are excluded from | ||
| connectivity checking. Their target objects belong to a separate | ||
| repository. | ||
|
|
||
| In partial clones, objects promised by a promisor remote are | ||
| accepted as connected without requiring local existence. The | ||
| check excludes promisor objects from traversal so that it does | ||
| not trigger on-demand fetches for them. | ||
|
|
||
| Full connectivity check | ||
| ----------------------- | ||
|
|
||
| The full connectivity check runs a `rev-list` subprocess, feeding | ||
| the new reference tips via stdin. A normal invocation is roughly: | ||
|
|
||
| git rev-list --objects --stdin --not --all --quiet | ||
| --alternate-refs [--exclude-promisor-objects] | ||
|
|
||
| In a partial clone, objects received from a promisor remote do | ||
| not need to be checked: anything they reference is promised by | ||
| that remote and can be fetched on demand. So if every new tip is | ||
| found in a promisor packfile, `rev-list` is not run at all. | ||
| Otherwise it runs with `--exclude-promisor-objects`, so that the | ||
| traversal stops at promisor objects instead of fetching them. | ||
|
|
||
| Consider the following graph after a fetch, where all reference | ||
| tips point directly to commits. For simplicity, only local | ||
| 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 then walked, stopping at objects already marked as | ||
| uninteresting. The cost of marking boundary trees uninteresting | ||
| dominates when the boundary commits have large trees, even when | ||
| the incoming changes are small. | ||
|
|
||
| Deepening fetches | ||
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| For deepening fetches (where the shallow boundary moves), the | ||
| full check omits `--not --all`. There is no existing-reference | ||
| boundary at which the walk can stop. Instead, traversal follows | ||
| the effective shallow boundary supplied for the deepened | ||
| repository. The new content may be below the old shallow | ||
| boundary even when the tips themselves have not changed. | ||
|
|
||
| Non-commit tips | ||
| ~~~~~~~~~~~~~~~ | ||
|
|
||
| When a new reference points to a non-commit object, such as a | ||
| tag, tree, or blob, that object is not part of the commit walk. | ||
| These non-commit tips are verified during the tree and blob | ||
|
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 |
||
| traversal that follows the commit walk within the same rev-list | ||
| invocation. | ||
|
|
||
| Incremental connectivity check | ||
| ------------------------------ | ||
|
|
||
| The incremental mode, selected by | ||
| `transfer.connectivityCheck=incremental`, avoids traversing the | ||
| full tree walk of the boundary commits. Instead, it verifies | ||
| each incoming commit's tree against the already-trusted trees of | ||
| its parent commits. | ||
|
|
||
| The same `--not --all` boundary as the full check identifies the | ||
| incoming commits. The incremental mode reuses this boundary but | ||
| replaces what happens afterwards: rather than walking boundary | ||
| trees to mark objects uninteresting, it compares each incoming | ||
| commit's tree against its parents' trees, processing commits in | ||
| topological order (ancestors first). This all happens inside | ||
| the same `rev-list` invocation, and the promisor fast path above | ||
| still runs first. | ||
|
|
||
| Trust model | ||
| ~~~~~~~~~~~ | ||
|
|
||
| A tree is trusted when its transitive object closure is known to | ||
| be connected. | ||
|
|
||
| Every parent of an incoming commit is either on the | ||
| already-connected side of the `--not --all` boundary (a boundary | ||
| parent whose tree is trusted by definition) or is an earlier | ||
| incoming commit that was already verified due to the topological | ||
| processing order. In both cases the parent's tree is trusted, so | ||
| all parent trees can be unconditionally used as comparison bases. | ||
|
|
||
| Once an incoming commit's tree has been verified, it is trusted | ||
| and can serve as a comparison base for later descendants. This | ||
| gives an inductive correctness argument: the base case is the | ||
| trust boundary established by `--not --all`, and the inductive | ||
| step is that verifying a commit extends the trusted set. | ||
|
|
||
| Tree states | ||
| ~~~~~~~~~~~ | ||
|
|
||
| The verifier tracks tree OIDs in three states: | ||
|
|
||
| untrusted:: | ||
| The tree has not yet been established as connected. This | ||
| is the implicit state of an OID not present in the state | ||
| map. | ||
|
|
||
| trusted:: | ||
| The tree is known to have a connected transitive closure, | ||
| but its direct entries have not yet been published into the | ||
| verifier's trusted object sets. | ||
|
|
||
| expanded:: | ||
| The tree is trusted and its direct non-gitlink entries | ||
| have also been published into the trusted object sets. | ||
|
|
||
| State transitions are monotonic: a tree may move from untrusted | ||
| to trusted to expanded, but never backwards. An untrusted tree | ||
| that is successfully verified goes directly to expanded. | ||
|
|
||
| Blobs require only trusted/untrusted state: a blob becomes | ||
| trusted when it is found in a trusted tree or when its existence | ||
| and type have been verified directly. | ||
|
|
||
| The trusted/expanded distinction is an optimization. An expanded | ||
| parent does not need to be reread merely to publish entries that | ||
| are already trusted, though it may still be read when same-path | ||
| subtree bases are needed for recursive comparison. | ||
|
|
||
| Algorithm | ||
| ~~~~~~~~~ | ||
|
|
||
| At a high level: | ||
|
|
||
| verify(commits): | ||
| sort topologically (ancestors first) | ||
| for each commit: | ||
| mark parent root trees as trusted | ||
| verify_tree(commit.tree, parent root trees) | ||
|
|
||
| verify_tree(tree, base_trees): | ||
| if tree already trusted: return | ||
| read tree, collect entries not already trusted | ||
| for each available base tree: | ||
| publish its entries as trusted | ||
| record same-path subtrees as bases | ||
| for each collected entry: | ||
| if now trusted: skip | ||
| if blob: verify blob connectivity and type | ||
| if tree: verify_tree(entry, its recorded bases) | ||
| mark tree as expanded | ||
|
|
||
| The important ordering within `verify_tree` is that entries from | ||
| the new tree are collected before the base trees are scanned, but | ||
| are processed only afterwards. Trust learned from any base can | ||
| therefore eliminate work before recursive verification begins. | ||
|
|
||
| Same-path parent subtrees are passed down as comparison bases | ||
| during recursive descent. If no comparison base is available, | ||
| the new subtree is verified from scratch. | ||
|
|
||
| Example | ||
| ~~~~~~~ | ||
|
|
||
| Consider a commit that changes one file under `lib/` and moves an | ||
| unchanged subtree from `src/` to `dev/`: | ||
|
|
||
| Parent tree New tree | ||
| +-- src/ (aaa) +-- dev/ (aaa) | ||
| +-- lib/ (bbb) +-- lib/ (ccc) | ||
| +-- foo.c (ddd) +-- foo.c (ddd) | ||
| +-- bar.c (eee) +-- bar.c (fff) | ||
|
|
||
| Scanning the parent makes `aaa` trusted even though it moved from | ||
| `src/` to `dev/`, so that subtree is skipped. The changed `ccc` | ||
| subtree is compared against its same-path parent `bbb`; scanning | ||
| `bbb` makes `ddd` and `eee` trusted, leaving only the new `fff` | ||
| blob to be checked. | ||
|
|
||
| This illustrates two properties: | ||
|
|
||
| * Trust is OID-based rather than path-based. An unchanged subtree | ||
| is recognized after a move. | ||
|
|
||
| * Same-path parent subtrees provide recursive comparison bases. | ||
| These bases improve pruning efficiency but are not required for | ||
| correctness; a new subtree can always be verified from scratch. | ||
|
|
||
| Multiple parents | ||
| ~~~~~~~~~~~~~~~~ | ||
|
|
||
| For a merge commit, root trees from all parents are comparison | ||
| bases. When several parents contain a same-path subtree, each | ||
| matching subtree is collected as a recursive comparison base. | ||
|
|
||
| Entries published from any trusted parent become globally trusted, | ||
| so a matching tree or blob entry present in any parent can be | ||
| skipped while verifying the merge tree. | ||
|
|
||
| Missing comparison bases | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| A promised base tree may not be locally available for comparison. | ||
| In that case the verifier skips that base and verifies the new | ||
| subtree without it. The missing comparison can reduce pruning but | ||
| does not affect correctness. | ||
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):