On Wed, Apr 03, 2024 at 12:13:42PM +0200, Han-Wen Nienhuys wrote: > On Fri, Mar 22, 2024 at 10:51 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > > Thanks for taking a look! > > > > Cc'ing Han-Wen and Josh for additional input. From my point of view the > > new algorithm is simpler to understand and less fragile, but I do wonder > > whether there is anything that we're missing. > > Good spotting. I hadn't thought about alternating tables. > > I have one minor criticism: > > Environment variables are untyped global variables without any form of > data protection, so I find them unsavoury, and have tried to avoid > them throughout. (The whole reftable library only looks at $TMPDIR in > tests). They're also accessible to end users, so it can become a > feature that can inadvertently become a maintenance burden. > > For testing, there is a stack->disable_auto_compact. > > If you want to keep that style, I would elevate disable_auto_compact > into reftable_write_options to make it API surface. This will let you > use it in tests written in C, which can be unittests and therefore > more precise and fine-grained. They also run more quickly, and are > easier to instrument with asan/valgrind/etc. The test for tables with > alternating sizes can be easily written in C. > > If you really need it, you could initialize disable_auto_compact from > the environment, but I would suggest avoiding it if possible. That's actually a good point. I think keeping this as an environment variable isn't too bad as a stop-gap measure for now, and it should be obvious to users that it's not for general use due to the `GIT_TEST` prefix. But I'm definitely supportive of lifting it out of the reftable library and into the reftable backend so that it is specific to Git, not to the reftable library. Patrick
Attachment:
signature.asc
Description: PGP signature
