Re: [PATCH 06/11] t-reftable-record: add ref tests for reftable_record_is_deletion()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 25, 2024, 17:31 UTC
Message-ID: <CAPig+cQFnKXeHJwKxSShXQOwP3fbjfSMX+C04_5urQyL9+uWJA@mail.gmail.com>
In-Reply-To: <CAOLa=ZSKcpOVv8wcDpe8mgtu53aWrCKG+scMqNix_BfBrT=KkA@mail.gmail.com>

On Tue, Jun 25, 2024 at 5:15 AM Karthik Nayak <karthik.188@gmail.com> wrote:

Show 27 quoted lines

> Chandra Pratap <chandrapratap3519@gmail.com> writes:
> > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> > @@ -121,15 +122,19 @@ static void test_reftable_ref_record_roundtrip(void)
> >               switch (i) {
> >               case REFTABLE_REF_DELETION:
> > +                     check(reftable_record_is_deletion(&in));
> >                       break;
> >               case REFTABLE_REF_VAL1:
> > +                     check(!reftable_record_is_deletion(&in));
> >                       set_hash(in.u.ref.value.val1, 1);
> >                       break;
>
> I think it might be easier to follow if we just move this outside the
> switch, perhaps something like:
>
> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c
> @@ -139,19 +139,15 @@ static void test_reftable_ref_record_roundtrip(void)
>                 switch (i) {
>                 case REFTABLE_REF_DELETION:
> -                       check(reftable_record_is_deletion(&in));
>                         break;
>                 case REFTABLE_REF_VAL1:
> -                       check(!reftable_record_is_deletion(&in));
>                         set_hash(in.u.ref.value.val1, 1);
>                         break;
> @@ -159,6 +155,7 @@ static void test_reftable_ref_record_roundtrip(void)
> +               check_int(reftable_record_is_deletion(&in), ==, i == REFTABLE_REF_DELETION);

It's subjective, of course, but for what it's worth, I find Chandra's code easier to reason about than this proposed rewrite for at least two reasons:

(1) The intention of the original code is obvious at a glance, whereas the proposed rewrite requires careful reading and thinking to understand what is being tested.

(2) In the original, because the check is being done within each `case` arm, it is easy to see how it relates to the case in question, whereas in the proposed rewrite, the test is far enough removed from from the `switch` that it is more difficult to relate to each possible case.