Re: [PATCH 1/2] packfile: split promisor objects oidset into two

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

Calvin Wan <calvinwan@google.com> writes:

> From: Han Young <hanyang.tony@bytedance.com>
>
> split promisor objects oidset into two, one is objects in promisor packfile,
> and other set is objects referenced in promisor packfile. This enable us to
> check if an object is in promisor packfile.

OK, so the idea is that we can discard the objects that are _in_ a
promisor packfile and assume that we can fetch them back?

Objects that are referenced by objects in the promisor packfile may
or may not be in the same packfile, and we obviously cannot expect
that we can refetch those that are not in the promisor packfile from
the promisor.  So what is the other list for?

What I am wondering is what good the existing helper function
is_promisor_object() is for.  It will say "yes" for objects that we
may have obtained from the promisor remote (hence we can lazily
fetch them again even if we lost them) and in promisor packs, but it
may also say "yes" for any object that an object that is in a
promisor pack (e.g., a tree object that represents a subdirectory
that was not modified by a commit in a promisor pack, a parent
commit of a commit in a promisor pack, etc.).  In other words, are
the callers getting any useful answer to their question to the
helper function, or are they all buggy for not asking "is this
object in a promisor pack" and allowing the helper to say "yes" for
objects that are merely referenced by an object in promisor packs?

Thanks.


> -int is_promisor_object(const struct object_id *oid)
> +int is_in_promisor_pack(const struct object_id *oid, int referenced)
>  {
> -	static struct oidset promisor_objects;
> +	static struct promisor_objects promisor_objects;
>  	static int promisor_objects_prepared;
>  
>  	if (!promisor_objects_prepared) {
> @@ -2303,5 +2308,6 @@ int is_promisor_object(const struct object_id *oid)
>  		}
>  		promisor_objects_prepared = 1;
>  	}
> -	return oidset_contains(&promisor_objects, oid);
> +	return oidset_contains(&promisor_objects.promisor_pack_objects, oid) ||
> +		(referenced && oidset_contains(&promisor_objects.promisor_pack_referenced_objects, oid));
>  }
> diff --git a/packfile.h b/packfile.h
> index 0f78658229..13a349e223 100644
> --- a/packfile.h
> +++ b/packfile.h
> @@ -195,11 +195,16 @@ int has_object_kept_pack(const struct object_id *oid, unsigned flags);
>  
>  int has_pack_index(const unsigned char *sha1);
>  
> +int is_in_promisor_pack(const struct object_id *oid, int referenced);
> +
>  /*
>   * Return 1 if an object in a promisor packfile is or refers to the given
>   * object, 0 otherwise.
>   */
> -int is_promisor_object(const struct object_id *oid);
> +static inline int is_promisor_object(const struct object_id *oid)
> +{
> +	return is_in_promisor_pack(oid, 1);
> +}
>  
>  /*
>   * Expose a function for fuzz testing.