Re: [PATCH 03/10] t-reftable-block: use reftable_record_equal() instead of check_str()

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 14, 2024 at 05:33:11PM +0530, Chandra Pratap wrote:
> In the current testing setup, operations like read and write for
> reftable blocks as defined by reftable/block.{c, h} are verified by
> comparing only the keys of input and output reftable records. This is
> not ideal because there can exist inequal reftable records with the
> same key. Use the dedicated function for record comparison,
> reftable_record_equal() instead of key-based comparison.

Nit: there should probably be a comma after the closing brace.

> diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
> index 31d179a50a..baeb9c8b07 100644
> --- a/t/unit-tests/t-reftable-block.c
> +++ b/t/unit-tests/t-reftable-block.c
> @@ -15,8 +15,8 @@ license that can be found in the LICENSE file or at
>  static void t_block_read_write(void)
>  {
>  	const int header_off = 21; /* random */
> -	char *names[30];
> -	const size_t N = ARRAY_SIZE(names);
> +	struct reftable_record recs[30];
> +	const size_t N = ARRAY_SIZE(recs);
>  	const size_t block_size = 1024;
>  	struct reftable_block block = { 0 };
>  	struct block_writer bw = {
> @@ -47,11 +47,11 @@ static void t_block_read_write(void)
>  		char name[100];
>  		snprintf(name, sizeof(name), "branch%02"PRIuMAX , (uintmax_t)i);
>  
> -		rec.u.ref.refname = name;
> +		rec.u.ref.refname = xstrdup(name);
>  		rec.u.ref.value_type = REFTABLE_REF_VAL1;
>  		memset(rec.u.ref.value.val1, i, GIT_SHA1_RAWSZ);
>  
> -		names[i] = xstrdup(name);
> +		recs[i] = rec;
>  		n = block_writer_add(&bw, &rec);
>  		rec.u.ref.refname = NULL;
>  		rec.u.ref.value_type = REFTABLE_REF_DELETION;
> @@ -72,7 +72,7 @@ static void t_block_read_write(void)
>  		check_int(r, >=, 0);
>  		if (r > 0)
>  			break;
> -		check_str(names[j], rec.u.ref.refname);
> +		check(reftable_record_equal(&recs[j], &rec, GIT_SHA1_RAWSZ));
>  		j++;
>  	}

Okay. Because we're not only checking for the refname anymore, we now
need to store the expected records as full records, which also requires
us to allocate the refname. Makes sense.

> @@ -90,7 +90,7 @@ static void t_block_read_write(void)
>  		n = block_iter_next(&it, &rec);
>  		check_int(n, ==, 0);
>  
> -		check_str(names[i], rec.u.ref.refname);
> +		check(reftable_record_equal(&recs[i], &rec, GIT_SHA1_RAWSZ));
>  
>  		want.len--;
>  		n = block_iter_seek_key(&it, &br, &want);

It would of course be great if we didn't only verify that SHA1 works as
expected, but that we can also read and write SHA256 records. But that
would be a new addition to the test suite that doesn't have to be part
of this patch series.

Patrick