Subject: Re: [PATCH v6 12/15] reftable: implement record equality generically

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 21 Jan 2022 12:52:09 +0100

On Thu, Jan 20 2022, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <[email-masked]>
>
> This simplifies unittests a little, and provides further coverage for
> reftable_record_copy().
>
> Signed-off-by: Han-Wen Nienhuys <[email-masked]>
> ---
>  reftable/record.c      | 57 +++++++++++++++++++++++++++++++++++++++++-
>  reftable/record.h      |  5 +++-
>  reftable/record_test.c | 23 +++--------------
>  3 files changed, 63 insertions(+), 22 deletions(-)
>
> diff --git a/reftable/record.c b/reftable/record.c
> index f7c77c51539..2a9e41a992e 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -430,6 +430,15 @@ static int reftable_ref_record_is_deletion_void(const void *p)
>  		(const struct reftable_ref_record *)p);
>  }
>  
> +

stray extra newline being added here.

> +static int reftable_ref_record_equal_void(const void *a,
> +					  const void *b, int hash_size)
> +{
> +	struct reftable_ref_record *ra = (struct reftable_ref_record *) a;
> +	struct reftable_ref_record *rb = (struct reftable_ref_record *) b;
> +	return reftable_ref_record_equal(ra, rb, hash_size);
> +}
> +
>  static struct reftable_record_vtable reftable_ref_record_vtable = {
>  	.key = &reftable_ref_record_key,
>  	.type = BLOCK_TYPE_REF,
> @@ -439,6 +448,7 @@ static struct reftable_record_vtable reftable_ref_record_vtable = {
>  	.decode = &reftable_ref_record_decode,
>  	.release = &reftable_ref_record_release_void,
>  	.is_deletion = &reftable_ref_record_is_deletion_void,
> +	.equal = &reftable_ref_record_equal_void,
>  };
>  
>  static void reftable_obj_record_key(const void *r, struct strbuf *dest)
> @@ -572,6 +582,25 @@ static int not_a_deletion(const void *p)
>  	return 0;
>  }
>  
> +static int reftable_obj_record_equal_void(const void *a, const void *b, int hash_size)
> +{
> +	struct reftable_obj_record *ra = (struct reftable_obj_record *) a;
> +	struct reftable_obj_record *rb = (struct reftable_obj_record *) b;
> +
> +	if (ra->hash_prefix_len != rb->hash_prefix_len
> +	    || ra->offset_len != rb->offset_len)
> +		return 0;
> +
> +	if (ra->hash_prefix_len &&
> +	    memcmp(ra->hash_prefix, rb->hash_prefix, ra->hash_prefix_len))
> +		return 0;

Similar to the memcpy() paranoia isn't this memcmp() paranoia?
I.e. memcmp() returns 0 on a n=0, so we can lose the
"ra->hash_prefix_len &&" here, no?

> +	if (ra->offset_len &&
> +	    memcmp(ra->offsets, rb->offsets, ra->offset_len * sizeof(uint64_t)))
> +		return 0;

...and here, since 0 * sizeof() will just get us zero.