From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 03 Nov 2026 10:17:16 GMT
Subject: Re: [PATCH] rev-list: ignore replacements in the connectivity check
Message-ID: <aum0rJ6T-OFWrMqW@pks.im>
In-Reply-To: <CAL71e4M7yFnTzYD0eVVGRmzkzSXbU0KZ-JScQOyB4-dB5xkAHA@mail.gmail.com>

On Tue, Nov 03, 2026 at 09:35:07AM +0100, Kristofer Karlsson wrote:
> On Mon, 2 Nov 2026 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> > > Teach the connectivity check to ignore replacements as well, so
> > > that it verifies the same objects that the commands relying on its
> > > result will read.
> >
> > Sounds like absolutely the right thing to do.  We might want to say
> > that we check both the original and the replacement, i.e., instead
> > of ignoring replace refs, we ensure that the commits, trees, and
> > blobs reachable from the replacement objects all exist by treating
> > those objects as if they were also at the tips of the connectivity
> > graph.
> >
> > [snip]
> > We can address the "shouldn't we make sure that replacement objects
> > used when the resulting history is in use are backed by objects
> > reachable from them?" issue separately.

Yeah, I was wondering whether we shouldn't verify both original and
replacement object. Previously we checked the latter, now we check the
former. But is either of those variants the correct thing to do?

> I have not fully thought this through yet, but I'm happy
> to follow up with further strengthening if there's a gap here.
> 
> My preliminary thinking is that since replacements are
> implemented as regular refs (refs/replace/OLD -> NEW),
> their reachable objects should already be covered by
> ordinary connectivity checks when those refs are fetched
> or pushed.

That's fair, I guess. The question though is whether we can rely on that
property when the replace ref was already written locally. But I'm not
sure that we even _should_ care about that. It's arguably the
responsibility of the user that wrote the replace ref to make sure that
the new object is valid. And in the context of the connectivity check
done as part of a push we always assume that existing refs are fully
connected anyway.

So overall I'm aligned with this change.

Patrick