Re: [PATCH v7 00/28] reftable library

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Apr 21, 2021, 09:52 UTC
Message-ID: <CAFQ2z_MF3SbKf8yXBvkOcLGg03dVxJW3F=c6ZDUtstEbZUUMCQ@mail.gmail.com>
In-Reply-To: <878s5c3wti.fsf@evledraar.gmail.com>

On Wed, Apr 21, 2021 at 9:45 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 7 quoted lines

> > The commits up to "hash.h: provide constants for the hash IDs" should be
> > good to merge to 'next'.
>
> With how Junio queues things up perhaps submitting this as another
> "prep" series would be good, to reduce future reviewer fatigue,
> i.e. anything we can make land on master makes re-rolls that much
> smaller.

will do.

Show 9 quoted lines

> > There are several test fixups, but I've put them in another series because
> > GGG enforces max 30 commits.
>
> I left a bunch of comments on the test prep series now. Probably good to
> have it split up regardless of GGG limits.
>
> Re the comments I left on the test series. I'm very happy to see the
> start of addressing the "it must be tested" concerns I had in
> https://lore.kernel.org/git/87wnt2th1v.fsf@evledraar.gmail.com/

It may look like the start, but I've been improving the number of tests that pass continuously since I posted the first version of this code over a year ago.

Show 10 quoted lines

> 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. If you would like this effort to move forward faster, I am most in need of review for the part that glues the library together with the git code itself (ie. the commit introducing refs/reftable-backend.c).

Show 5 quoted lines

> > Due to using uncompress2, this build fails on the Linux32 builder; what is
> > the magic incantation to run autoconf?
>
> Doesn't this just need a NO_UNCOMPRESS2 for Linux32 in ci/lib.sh? See
> the lines just below that for e.g. NO_REGEX for another CI target.

Thanks!

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