Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 3, 2022, 17:39 UTC
Message-ID: <220203.867dab6dmp.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_NSCvRbj1bxirxhqSWD+LadzCa8VNOsxGCmFCNT3GUU0g@mail.gmail.com>

On Thu, Feb 03 2022, Han-Wen Nienhuys wrote:

Show 34 quoted lines

> On Tue, Feb 1, 2022 at 11:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>> > We could surely add magic record types, but how would such a dance be
>> > performed while keeping compatibility with existing JGit clients?
>>
>> Yes.  It is exactly the point of the question I asked.  If it is
>> simple and easy to add such a new type that is ignored/skipped by
>> existing clients, then we can go that route.  If it is simple and
>> easy to add a new bit per ref that existing clients would not barf,
>> we can use that as an alternative implementation strategy.
>
> I'm not sure that there are any JGit clients: I committed reftable
> support at the end of 2019. Before that time, we were running it
> internally at Google, but only ref storage, and without the posix
> part. Reflogs were never stored in refable, and I actually found a
> couple of bugs in Shawn's Java code.
>
> Gerrit has increasingly started using Git as a database, and the
> packed/loose system is just not a very good database, so that
> motivates the work reftable in general. But the folks who run Gerrit
> on a POSIX filesystem want to be sure that isn't a fringe feature, so
> they only want to start using it once Git itself supports it. So there
> is a chicken & egg problem.
>
> It's sad that we have to introduce an existence bit to make things
> work, but overall it is probably easier for me to do than trying to
> make sense of sequencer.c and how it uses refs/stash@{0}.
>
> Technically, the only obstacle I see is that we'd need to treat an
> existence entry especially for the purpose of compaction/gc: we can
> discard older entries, but we shouldn't discard the existence bit, no
> matter how old it is.

Ah, that's very informative. I had been assuming (or misremembered) that reftable was already seeing production use at Google. Perhaps I remembering the now-dead Google Code (or whatever it was called). Maybe not.

In any case, not being locked into the format as specified is very nice. So is it basically seeing no (production) use anywhere as far as you know? Whether that's in production at Google, or some third parties via JGit-something (maybe as editor libraries?).

Taking a bit of a step back.

I do think that generally speaking parts of this series are putting the cart before the horse in seemingly trying to get the test suite clean before we have the integration in-tree.

Not everything you have here, but some of it.

I know I'm the one who started encouraging you to work towards getting the test mode passing, but I think that while it's good to mark some obviously file-only tests beforehand, anything where we have different behavior under reftable should really come after.

Because then we can positively assert what we do differently, not just skip an existing test.

And yes, for many tests that will require rewriting their setup, because they conflate things that are backend-independent, such as the general question of "can we ask about reflog existence?" with the implementation detail of the test setup, which oftentimes is file-backend specific.

Of course that will mean we'll have some interim period where our test suite is a dumpster fire under GIT_TEST_REFTABLE=true, and I think that's fine, as long as we work towards getting it passing, and as long as the non-stability of the nascent backend is very prominently advertised in the interim.

I.e. I think *the* issue with the original series you had in this regard was that git-init.txt (or whatever it was) basically just discussed enabling reftable matter-of-factly, when we were still failing tens/hundreds of tests, which is just setting up a big bear trap for users to step into.

But if we just changed those docs a bit to note "!!WARNING WARNING!! EXPERIMENTAL AND UNSTABLE !!WARNING WARNING!!" or whatever we could merge the API integration parts sooner than later, even with a lot of known-broken tests.

We could then whitelist the broken parts, and work on narrowing that set down. Similar what the SANITIZE=leak mode is currently doing for memory leaks.

I think that would make things a lot easier when reviewing submissions like these, in that we have reftable/* in-tree already, but with the "real" integration we could check how files/reftable backends behave, add the diverging behavior to tests etc.

What do you think?