Subject: Re: [PATCH v2 02/30] oid-array: teach oid-array to handle multiple kinds of oids

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 15 Feb 2024 12:21:51 +0100

On Sun, Oct 01, 2023 at 09:40:06PM -0500, Eric W. Biederman wrote:
> From: "Eric W. Biederman" <[email-masked]>
> 
> While looking at how to handle input of both SHA-1 and SHA-256 oids in
> get_oid_with_context, I realized that the oid_array in
> repo_for_each_abbrev might have more than one kind of oid stored in it
> simultaneously.
> 
> Update to oid_array_append to ensure that oids added to an oid array
> always have an algorithm set.
> 
> Update void_hashcmp to first verify two oids use the same hash algorithm
> before comparing them to each other.
> 
> With that oid-array should be safe to use with different kinds of
> oids simultaneously.
> 
> Signed-off-by: "Eric W. Biederman" <[email-masked]>
> ---
>  oid-array.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/oid-array.c b/oid-array.c
> index 8e4717746c31..1f36651754ed 100644
> --- a/oid-array.c
> +++ b/oid-array.c
> @@ -6,12 +6,20 @@ void oid_array_append(struct oid_array *array, const struct object_id *oid)
>  {
>  	ALLOC_GROW(array->oid, array->nr + 1, array->alloc);
>  	oidcpy(&array->oid[array->nr++], oid);
> +	if (!oid->algo)
> +		oid_set_algo(&array->oid[array->nr - 1], the_hash_algo);

I feel like it's a design wart that `oid_array_append()` now started to
depend on repository discovery, adding an external dependency to it that
may cause very confusing behaviour. Are there for example ever cases
where we populate such an OID array before we have discovered the repo?
Can it happen that we use OID arrays in the context of a submodule that
has a different object ID than the main repository?

>  	array->sorted = 0;
>  }
>  
> -static int void_hashcmp(const void *a, const void *b)
> +static int void_hashcmp(const void *va, const void *vb)
>  {
> -	return oidcmp(a, b);
> +	const struct object_id *a = va, *b = vb;
> +	int ret;
> +	if (a->algo == b->algo)
> +		ret = oidcmp(a, b);
> +	else
> +		ret = a->algo > b->algo ? 1 : -1;

Okay, so we basically end up sorting first by the algorithm, and then we
sort all object IDs of a specific algorithm relative to each other.

Patrick

> +	return ret;
>  }
>  
>  void oid_array_sort(struct oid_array *array)
> -- 
> 2.41.0
>