On Wed, May 26 2021, Han-Wen Nienhuys wrote:
> On Thu, May 20, 2021 at 6:35 PM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx/
2. http://lore.kernel.org/git/87pmxlpgb1.fsf@xxxxxxxxxxxxxxxxxxx
