Re: [PATCH v4 13/15] Reftable support for git-core

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Han-Wen Nienhuys <hanwen@google.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Ramsay Jones <ramsay@ramsayjones.plus.com>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jonathan Tan <jonathantanmy@google.com>,
	Josh Steadmon <steadmon@google.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Patrick Steinhardt <ps@pks.im>,
	Felipe Contreras <felipe.contreras@gmail.com>,
	Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH v4 13/15] Reftable support for git-core
Date: Wed, 28 Apr 2021 18:32:31 +0200	[thread overview]
Message-ID: <87h7jqz7k5.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <CAFQ2z_Pb62Oe352UwdsPWLxjXd7P97pyuggTMK2URJd2p=OTnA@mail.gmail.com>

On Mon, Apr 26 2021, Han-Wen Nienhuys wrote:

> On Mon, Apr 26, 2021 at 4:28 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I agree that whether or not we should have any tests at all is a matter
>> of opinion and circumstance. I don't think in general that someone using
>> some random internal API needs to be checking what is and isn't tested
>> in that API.
>>
>> I do think in this case that it's worth digging a bit further. The APIs
>> in question are using EINVAL and EISDIR to pass up errors that are had a
>> 1:1 mapping into the FS historically.
>>
>> Are we really better off faking those up, or is some of that perhaps not
>> used at all? Maybe we'd find that this level of abstraction isn't what
>> we need, and it could actually be much simpler.
>
> I think none of these error functions are being used at all, and I've
> made a start at removing them in https://github.com/git/git/pull/1012
> (see also my discussion with peff.)
>
>> It seems to me that a good way to get there is to seek some systematic
>> ways of focusing review onto various edge cases of this series. I.e. to
>> begin with having GIT_TEST_REFTABLE pass as noted elsewhere, and in this
>> case calling attention to some of the underlying assumptions behind this
>> series.
>
>> One of the hardest things I've found about trying to review this has
>> been closing the gap between things that exist in your mind and commit
>> messages / code.
>
> thanks, that is valuable feedback.
>
>> something like this in the commit message (and other applicable commits
>> etc.) would be *very* valuable:
>>
>>     In functions such as git_reftable_read_raw_ref() (and ????, are
>>     there more?) we're diligently emulating the documented behavior and
>>     return values of the refs file backend. According to "make gcov" we
>>     can see we don't have coverage for this, in particular the behavior
>>     of EINVAL etc.
>
> I haven't done this, because a lot of these considerations are
> transient, and I'd rather not spend a lot of time documenting what I
> don't know.
>
>> I.e. per [1] once if and when we have GIT_TEST_REFTABLE passing surely
>> one of the best way to garner feedback on the rest is to discover those
>> parts (using "make gcov", after running with/without
>> GIT_TEST_REFTABLE=[true|false]) where we still have outstanding blind
>> spots between the tests and code.
>
> Getting GIT_TEST_REFTABLE=1 passing is the hard part, because it means
> having  to understand exactly how the current code is supposed to
> work.  Once I get to that point (with knowledge being complete and
> tests passing), it will be easy to document what is happening and why.
>
> I was hoping that by posting these series with known test failures,
> and questions marked "XXX" in reftable-backend.c, I would get feedback
> from the other people who know exactly how this part of the code
> works.  But from your mail, I get the sense that nobody understands
> how the whole picture fits together?

Almost definitely not. I don't know about you but when I'm looking at
code I wrote 6 months ago handling some special case it takes me a while
to get up to speed on just knowing what I knew then, and when we're
talking about something like refs.c ...

On the topic of the way forward: I for one would very much be for a plan
where step 0 is to just a series import the reftable code you have
as-is. I.e. we'd include it as an imported external library, maybe have
some light test-tool integration and compile it / run its own tests, but
not have/advertise the "git init" etc. integration yet.

I.e. my opinion on GIT_TEST_REFTABLE=1 needing to pass is implicit (but
I now realize I haven't explicitly said this) on that happening before
the tree is in a state where we'd code/doc-wise be in a state of
shipping this to users.

Whereas just importing the library != that, and I think we'd be in much
better shape if we had it in-tree and would incrementally work on
integration from that point, v.s. having more re-rolls of
mostly-the-same big codebase being re-submitted.

I'm sure there's some things that'll need to change as we start the
test/integration work, e.g. the reflog topic that's been discussed. But
that's surely better done as some patches on top of the already-landed
library import at this point v.s. trying to get the library perfect
before getting it in-tree.

Maybe Junio disagrees, just my 0.02....