Re: [PATCH v7 00/28] reftable library

On Wed, Apr 21, 2021 at 1:21 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

I don't see the point of having re-rolls of this topic while the test
changes topic it's based on hasn't finished
marking/splitting/refactoring the various tests that do and don't depend
on the file backend.

At least when I review it I'm just left with going in circles digging
into one of those failing test, figuring out if it's really
refs/files-backend.c specific or not etc., and as long as we can't turn
on GIT_TEST_REFTABLE=true in CI as part of this series I don't see a
path to making it advance to next->master.

The point of posting updates is to garner feedback, especially from
people familiar with the Git code itself.

So you agree that we should make the tests pass first, then shouldn't
these me marked as RFC/PATCH?

I added RFC to the patch introducing reftable-backend.c

I consider the code under reftable/*.c to be of production quality,
even if it's stylistically different from the git code.

IOW a large part of the feedback you're looking for is already part of
the codebase. Nobody can keep all of it in their head, but we've encoded
all the tricky edge cases we could think of in the tests.

That's disappointing. I know that I can just fix test failures until
there are none left, but it's not a terribly efficient method if there
is someone who knows how this works.  Is there no other way to solicit
feedback?

I.e. reaching some consensus on things like whether
GIT_TEST_REFTABLE=true passing under CI being a hard-prereq for this
series or not is surely one of the first things to sketch out before
figuring out how to move this forward.

I've had conflicting feedback about this, but it's ultimately not my
call to make. Jonathan Nieder suggested that this was asking too much,
and it would be OK to have a credible starting point where the effort
to address remaining test failures could be shared across multiple
people. Jun also expressed he didn't want to put all the work on my
shoulders, but so far I've not received much help.

In v6 I noted that t5510-fetch.sh had a segfault[4], you said you'd
check it out, and reading your cover letter nothing stood out about
that, so I assumed it was sorted out somehow.

I fixed a number of segfaults, so it's quite likely (I did see the
freed transaction BUG).

One of the problems of working with a large pending series like this,
is that it is a project in itself, and keeping track of which change
fixes which bug is something I really need version control for, but
the frequent rebasing/squashing erases this kind of information.

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