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

[Junio C Hamano <gitster@pobox.com>]

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?

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