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

On Wed, May 26, 2021 at 12:07 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

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'm sending out the series shortly, with a Reviewed-By footer added to
the commits.

Jun, is this OK to schedule for 'next'  ?

quoted

quoted

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.

I split a few tests.

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

Until reftable is actually merged that will introduce a lot of dead
code in the tests. Besides requiring time for me to write that code,
it also introduces extra reviewer effort. I think that is best done as
part of the reftable series, when it is close to being merged.

    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

Let's first see the actual PostgreSQL backend materialize before
making investments to support it.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado