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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 23 2021, Han-Wen Nienhuys wrote:

> On Thu, Jan 21, 2021 at 4:55 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I've been chasing down some edge cases in refs.c as part of another WIP
>> series I have and found that for this particular "errno" stuff we don't
>> have any test coverage. And with master & hanwen/reftable if I apply:
>>
>>     -                       errno = EINVAL;
> ..
>> All tests pass on both.
> ..
>> That I can remove error checking/handling from this place means existing
>> general logic was faithfully copied without checking that we have a test
>> for it, or adding one.
>
> I think that is an unfair characterization. The API documentation of
> read_raw_ref_fn says that implementations should return EINVAL for
> certain cases, so that's what I did. The point of having a documented
> internal API is that one doesn't have to double check what is behind
> the API.

I'm only meaning to point out that the behavior being relied on here
doesn't have any tests.

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.

My main goal here is not to keep creating some infinite amount of work
for you, but to try to help this along into some shape that's better
reviewable & land-able.

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.

E.g. in this case sure, you can be of the opinion that since the point
of an internal API is to provide an interface we can simply assume it's
OK and working as intended.

I don't think given the history and use of this code that that's an
assumption I'd be as comfortable making, but anyway. Getting back on
point, I do think whatever opinion anyone is of on that subject having
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.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.

1. https://lore.kernel.org/git/87lf9b3mth.fsf@evledraar.gmail.com/