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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Apr 23, 2025, 17:08 UTC
Message-ID: <CAOLa=ZQh+agLJYEGWU2O2=iRnJy94y9Cud3XRVaGVHC4YdbBZw@mail.gmail.com>
In-Reply-To: <20250423-pks-object-store-cleanups-v1-7-81f8411a5d08@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 8 quoted lines

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

Show 7 quoted lines

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

Show 27 quoted lines

> 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.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/index-pack.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index f49431d626b..805b7aa1e28 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -892,9 +892,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>
>  	if (startup_info->have_repository) {
>  		read_lock();
> -		collision_test_needed =
> -			repo_has_object_file_with_flags(the_repository, oid,
> -							OBJECT_INFO_QUICK);
> +		collision_test_needed = has_object(the_repository, oid, 0);
>  		read_unlock();
>  	}
>
>
> --
> 2.49.0.901.g37484f566f.dirty