Re: [PATCH v2 07/13] builtin/index-pack: don't fetch promised objects for collision check

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 29, 2025, 06:15 UTC
Message-ID: <aBBumhDhWoR9LEb3@pks.im>
In-Reply-To: <xmqq34dsarhv.fsf@gitster.g>

On Mon, Apr 28, 2025 at 02:46:52PM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Any packed objects indexed via git-index-pack(1) are subject to a
> > collision check. This collision check has the intent to determine
> > whether we already have an object with the same object ID, but different
> > contents in the repository.
> >
> > The check whether the collision check is really needed is only performed
> > in case `repo_has_object_file_with_flags(..., OBJECT_INFO_QUICK)` tells
> > us that the object exists. But unless explicitly told otherwise by
> > passing `OBJECT_INFO_SKIP_FETCH_OBJECT`, this function will also cause
> > us to fetch the object in case it is part of a promisor pack. As such,
> > we may end up fetching the object only to check whether the fetched
> > object and the object that we're indexing have the same content.
> >
> > This behaviour is highly dubious and more likely than not unintended.
> > Fix it by converting to `has_object()`, which knows to neither reload
> > packfiles nor to fetch promisor objects by default.
> 
> It is unclear why you thing it is highly dubious from reading the
> above paragraph three times, though.
> 
> Is it that if we are suspicious of the incoming pack data we are
> indexing, we should also not be too trusting of the object that our
> promisor remote would be giving us?  To put it in reverse, our
> attitude being "we trust the first copy of object we saw", which
> translates to "we trust where we explicitly clone and fetch from" in
> the traditional world without lazy fetching, if somebody else we are
> explicitly fetching from offers us an object that the promisor
> remote would give us, we just do not bother if they are the same
> because it is not like we trust our promisor more than we trust the
> current counterpart we are fetching from?

Yes, exactly. When we don't have an object locally we don't have a trust anchor for verifying that contents of the object look as expected. So there are only two ways to do this:

  - Use a trust-on-first-use model. We trust the object we obtain
    initially and from thereon we start to treat it as the "correct"
    object and verify incoming objects with the same ID against it.

  - We only trust what everyone agrees one. In that case though we
    really should be cross-verifying with _all_ remotes, not only with
    the promisor remote.

Right now we do neither, but we end up treating the promisor as "more trusted" than any of the other remotes.

I think it's completely unintentional that we end up fetching the object from the promisor to perform a collision check against the packfile we are about to index. It is highly likely that the promisor remote and the remote that we're fetching from are the same anyway, so all this does is to waste resources.

Anyway, I'll evict these patches from this series. I think a couple of the sites are broken, but for now I care more about the bigger picture.

Patrick