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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2025, 21:46 UTC
Message-ID: <xmqq34dsarhv.fsf@gitster.g>
In-Reply-To: <20250425-pks-object-store-cleanups-v2-7-63f1695b7700@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 16 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 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?

Show 19 quoted lines

> 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();
>  	}