On Wed, 7 Aug 2024 at 14:11, Patrick Steinhardt <ps@xxxxxx> wrote: > > On Tue, Aug 06, 2024 at 07:43:40PM +0530, Chandra Pratap wrote: > > diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c > > index 14909b127e..0c15e654e8 100644 > > --- a/t/unit-tests/t-reftable-stack.c > > +++ b/t/unit-tests/t-reftable-stack.c > > @@ -145,7 +145,7 @@ static void t_reftable_stack_add_one(void) > > > > err = reftable_stack_read_ref(st, ref.refname, &dest); > > check(!err); > > - check_str("master", dest.value.symref); > > + check(reftable_ref_record_equal(&ref, &dest, GIT_SHA1_RAWSZ)); > > check_int(st->readers_len, >, 0); > > I think the change itself is sensible as long as we have tests that > verify that `reftable_ref_record_equal()` itself behaves as expected. I > don't think we have such tests anywhere though, uncovering a test gap. We _do_ test reftable_record_equal (which is a wrapper for reftable_ref_record_equal in the case of ref records) in the recently ported t-reftable-record test. Here is the test exercising this function in unit-tests/t-reftable-record.c: static void t_reftable_ref_record_comparison(void) { struct reftable_record in[3] = { { .type = BLOCK_TYPE_REF, .u.ref.refname = (char *) "refs/heads/master", .u.ref.value_type = REFTABLE_REF_VAL1, }, { .type = BLOCK_TYPE_REF, .u.ref.refname = (char *) "refs/heads/master", .u.ref.value_type = REFTABLE_REF_DELETION, }, { .type = BLOCK_TYPE_REF, .u.ref.refname = (char *) "HEAD", .u.ref.value_type = REFTABLE_REF_SYMREF, .u.ref.value.symref = (char *) "refs/heads/master", }, }; check(!reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); check(!reftable_record_cmp(&in[0], &in[1])); check(!reftable_record_equal(&in[1], &in[2], GIT_SHA1_RAWSZ)); check_int(reftable_record_cmp(&in[1], &in[2]), >, 0); in[1].u.ref.value_type = in[0].u.ref.value_type; check(reftable_record_equal(&in[0], &in[1], GIT_SHA1_RAWSZ)); check(!reftable_record_cmp(&in[0], &in[1])); }