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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Apr 28, 2025, 19:48 UTC
Message-ID: <CAOLa=ZQy=OrWkqUTS+t_fAYj2w--1Fy=n7=gdJTHFhebNAZCnA@mail.gmail.com>
In-Reply-To: <aAs0F58Hlpn6WI6b@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 46 quoted lines

> On Wed, Apr 23, 2025 at 10:08:05AM -0700, Karthik Nayak wrote:
>> 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 performed via
>> > `repo_has_object_file_with_flags(..., OBJECT_INFO_QUICK)`. \
>> >
>>
>> Nit: this was a little confusing at first, until I saw the code. So what
>> this means is that the collision check is only performed, iff
>> `repo_has_object_file_with_flags(...)` returns true.
>>
>> I think the confusing part was 'is performed via', perhaps:
>>
>>   The collision check is only performed, if
>>   repo_has_object_file_with_flags(..., OBJECT_INFO_QUICK) returns a
>>   truthy value.
>>
>> But it is okay as is too!
>
> Will rephrase.
>
>> > But unless
>> > explicitly told otherwise via `OBJECT_INFO_SKIP_FETCH_OBJECT`, this
>> > function will also cause us to fetch the object ID 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.
>> >
>>
>> So us fetching the object is pointless, since we only care about the
>> 'does it exist' part and not really what it contains. In that case,
>> shouldn't this be s/same content/same oid/?
>
> No, it really checks for the same content. It basically verifies that
> any pair of objects that:
>
>   - Exist in the packfile that we're currently indexing.
>   - And preexists in the local repository.
>
> Actually have the same content.
>

Okay this makes sense, if they do have the same content, this is not a collision. It is simply a duplicate.

Show 8 quoted lines

> The weird part is that we also do this for objects that don't yet exist
> in the repository, but which are promised to us. This causes us to fetch
> them first only to verify that the fetched promised object has the same
> content as the packfile. And given that git-index-pack(1) would usually
> run after a fetch, we end up verifying that the fetched object obtained
> from the promisor is the same as the fetched object obtained from the
> packfile. Which ultimately seems rather dubious to me.
>

To clarify, the flow currently (simplified) is:

1. We check if a collision test is required, by checking if the new OID
already exists in the repository.
2. If collision test is required.
   a. Fetch and check the object type.
   b. Read the old object data.
   c. Compare the new object data and the old object data.
   d. Collision detected if there is a mismatch.

Currently, we fetch for promisor objects in #1, which is unnecessary because we simply want to know if the object exists in the repository. The actual check in #2.b would still fetch the promisor object (if that flow is taken).

> Patrick