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@pks.im> 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