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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Apr 26, 2021, 16:17 UTC
Message-ID: <CAFQ2z_Pb62Oe352UwdsPWLxjXd7P97pyuggTMK2URJd2p=OTnA@mail.gmail.com>
In-Reply-To: <87lf953yto.fsf@evledraar.gmail.com>

On Mon, Apr 26, 2021 at 4:28 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines

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

Show 5 quoted lines

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

Show 8 quoted lines

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

Show 5 quoted lines

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

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