From: Kristofer Karlsson <krka@spotify.com>
Date: Tue, 03 Nov 2026 10:38:38 GMT
Subject: Re: [PATCH] rev-list: ignore replacements in the connectivity check
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:
>
> 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.

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