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

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

On Mon, Apr 26, 2021 at 4:28 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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.)

quoted

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.

quoted

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.

quoted

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.

quoted

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....