From: Kristofer Karlsson <krka@spotify.com>
Date: Tue, 06 Oct 2026 10:12:40 GMT
Subject: Re: [PATCH v2 1/2] Documentation: describe connectivity checking
Message-ID: <CAL71e4Pd3ziR-7bbBDUei-pcF69mEpt33fE51WC-H=7YgXq=NQ@mail.gmail.com>
In-Reply-To: <asNY7SfEohsOSf0J@pks.im>

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,
Kristofer