On 24/04/03 12:18PM, Patrick Steinhardt wrote: > 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. Moving the env out of the reftable library seems reasonable to me. I'll make this change as part of the next version of this series. -Justin
