Re: [PATCH] rev-list: ignore replacements in the connectivity check

From: Kristofer Karlsson <krka@spotify.com>
Date: Nov 3, 2026, 10:38 UTC
Message-ID: <CAL71e4M4qBJ6aHu-vr9Ex53LQsrpxjn9GRtkgNa8KSOSYWo4Lw@mail.gmail.com>
In-Reply-To: <aum0sTUAqc27DU_W@pks.im>

On Tue, 3 Nov 2026 at 11:17, Patrick Steinhardt <ps@pks.im> wrote:

Show 10 quoted lines

>
> On Mon, Nov 02, 2026 at 03:47:09PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> [snip]
> > The t5413 test that expected the check to accept a commit whose
> > original tree is incomplete because a replacement is complete now
> > expects a rejection, and the test that disabled replacements
> > explicitly to get one is redundant.
>
> This here makes me pause a bit. Do we know whether this specific test
> was introduced intentionally to make a specific edge case work?

Both the t5413 test and tree-verify.c were introduced as part of my (recent) incremental connectivity check series. I noticed the replacement "oddity" back then, but deliberately kept the existing behavior -- changing the semantics should be separate and intentional.

Show 25 quoted lines

>
> > diff --git a/tree-verify.c b/tree-verify.c
> > index 25db76d30b..c228235150 100644
> > --- a/tree-verify.c
> > +++ b/tree-verify.c
> > @@ -133,7 +133,6 @@ static int verify_blob(struct repository *repo,
> >       vs->blobs_checked++;
> >       oi.typep = &type;
> >       if (odb_read_object_info_extended(repo->objects, oid, &oi,
> > -                                       OBJECT_INFO_LOOKUP_REPLACE |
> >                                         OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> >               if (vs->exclude_promisor_objects &&
> >                   is_promisor_object(repo, oid))
> > @@ -163,7 +162,6 @@ static int read_tree_object(struct object_database *odb,
> >       oi.sizep = sizep;
> >       oi.contentp = bufp;
> >       if (odb_read_object_info_extended(odb, oid, &oi,
> > -                                       OBJECT_INFO_LOOKUP_REPLACE |
> >                                         OBJECT_INFO_SKIP_FETCH_OBJECT))
> >               *bufp = NULL;
> >       if (*bufp && type != OBJ_TREE) {
>
> Nit: this might have warranted a separate commit as it isn't even
> related to the connectivity check, is it? Did you also vet all the other
> sites that that verify objects?

tree-verify.c is only used by rev-list --connectivity-check=incremental, so the flag removal is part of the same change: without it, the incremental check would still nominally ask for replacements while the process has them disabled. The behavior would be the same, but it would look confusing (why ask for replacements if they are always globally disabled when this path is reached?)

This patch is intentionally limited to the connectivity check. I did review the other users of OBJECT_INFO_LOOKUP_REPLACE, but none of them seem related to it. Longer term it would be nice if each caller stated whether it wants replacements, rather than relying on the global switch, but that is a much larger change.

One of them does look like a separate issue, though: mktree checks that each entry exists, following replacements, but writes the original oid into the tree, so a missing object can pass the check without --missing if it has a replace ref. That could be looked at separately as a bugfix maybe? I have not looked into it more though.

Thanks, Kristofer