Re: [PATCH v2 00/21] Prepare tests for reftable backend

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 26, 2021, 09:52 UTC
Message-ID: <87pmxdn6zo.fsf@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_NDKn4vTM_WBrDMumrcTkzS2nOT-cd-TQ=LpTHG+m8Fug@mail.gmail.com>

On Wed, May 26 2021, Han-Wen Nienhuys wrote:

Show 25 quoted lines

> On Thu, May 20, 2021 at 6:35 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>
>>
>> I looked this all over and left some nits, suspect/probably/definite bug
>> comments on specific patches, the ones with no comments from me LGTM.
>
> Awesome. Thanks for reviewing. I'll try to go over your comments this week.
>
>> I intentionally didn't review my own earlier feedback on v1 to look at
>> this with fresh eyes, I'd forgetten what points I raised, aside from the
>> general "let's not skip tests but test the new behavior" mantra I think
>> I either mentioned there or in related discussions.
>
>
> "test new behavior" is troublesome, because it requires merging the
> reftable support first, which in itself is a tough job, and predicated
> on getting reviews for that code. This is why I split this series off,
> because it can be merged early without impacting coverage of the
> existing loose/packed backend.
>
> How about I document more clearly, for each test marked REFFILES, what
> is going on, and what should be done for coverage with reftable? We
> can go over it as part of the reftable series. "REFFILES" will be an
> easy term to grep for.

Indeed, as I pointed out in some (particularly later patches) v2 commentary there's that chicken & egg problem.

Having re-skimmed my v2 reviews I think these would help:

 A. Just leaning on the side of splitting up tests, e.g in my comment on
    20/21[1], there's a test with file backend specific logic there, but
    also a general assertion. Should just be two tests.

 B. My suggestion in 15/21[2] to split up maybe make these two
    prerequisites, i.e. REFFILES and REFTABLE.

    One one hand it's a bit silly to have two prerequisites required for
    the one reftable mode we don't have yet, and as long as we just have
    two backends it'll be an informed guess at best.

    But I'm leaning on the side of it being a good idea, to clearly
    document those cases that are really files-backend.c specific, such
    as low-level test to check that a .git/HEAD exists along with a "git
    rev-parse HEAD" for that backend.

    And then REFTABLE (and !REFTABLE) to test/skip things that are
    specific to the behavior of reftable, but not in a way that has to
    do with the "ref files" v.s. "a ref database" distinction. E.g. a
    maintenance command calling pack-refs is skipped under REFTABLE, but
    one could imagine us e.g. having a PostgreSQL backend where that
    would be mapped to "VACUUM refs" or whatever. I.e. it's
    testing/skipping based on reftable-specific features.

    Even if strictly speaking we'd make no use of the distinction now in
    that you could losslessly map REFFILES = !REFTABLE and REFTABLE =
    !REFFILES.

    Or maybe it's useless and we'd want to eventually do a second pass
    over both REFFILES and REFTABLE to see if we need to change them to
    positively assert the behavior of the "other" backend in both
    cases. Just a thought...

1. https://lore.kernel.org/git/87eee1pfzk.fsf@evledraar.gmail.com/
2. http://lore.kernel.org/git/87pmxlpgb1.fsf@evledraar.gmail.com