From: Justin Tobler <jltobler@gmail.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: Han-Wen Nienhuys <hanwenn@gmail.com>,
Justin Tobler via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Josh Steadmon <steadmon@google.com>
Subject: Re: [PATCH v2 0/3] reftable/stack: use geometric table compaction
Date: Wed, 3 Apr 2024 10:14:43 -0500 [thread overview]
Message-ID: <b24jjzw72rlcpctteokr5yfbuwuy2cc3qvibzhuju4gbj63lfa@gbtsmvufyuhd> (raw)
In-Reply-To: <Zg0tEa7DDEuub09Q@tanuki>
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@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.
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
next prev parent reply other threads:[~2024-04-03 15:15 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-05 20:03 [PATCH] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-03-06 12:30 ` Patrick Steinhardt
2024-03-06 12:37 ` Patrick Steinhardt
2024-03-21 22:48 ` Justin Tobler
2024-03-21 22:40 ` [PATCH v2 0/3] " Justin Tobler via GitGitGadget
2024-03-21 22:40 ` [PATCH v2 1/3] reftable/stack: add env to disable autocompaction Justin Tobler via GitGitGadget
2024-03-22 1:25 ` Patrick Steinhardt
2024-03-21 22:40 ` [PATCH v2 2/3] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-03-22 1:25 ` Patrick Steinhardt
2024-03-27 13:24 ` Karthik Nayak
2024-03-21 22:40 ` [PATCH v2 3/3] reftable/segment: make segment end inclusive Justin Tobler via GitGitGadget
2024-03-22 1:25 ` [PATCH v2 0/3] reftable/stack: use geometric table compaction Patrick Steinhardt
2024-04-03 10:13 ` Han-Wen Nienhuys
2024-04-03 10:18 ` Patrick Steinhardt
2024-04-03 15:14 ` Justin Tobler [this message]
2024-04-03 16:40 ` Junio C Hamano
2024-03-29 4:16 ` [PATCH v3 " Justin Tobler via GitGitGadget
2024-03-29 4:16 ` [PATCH v3 1/3] reftable/stack: add env to disable autocompaction Justin Tobler via GitGitGadget
2024-03-29 18:25 ` Junio C Hamano
2024-03-29 21:56 ` Junio C Hamano
2024-04-02 7:23 ` Patrick Steinhardt
2024-04-02 17:23 ` Junio C Hamano
2024-03-29 4:16 ` [PATCH v3 2/3] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-04-02 7:23 ` Patrick Steinhardt
2024-03-29 4:16 ` [PATCH v3 3/3] reftable/stack: make segment end inclusive Justin Tobler via GitGitGadget
2024-03-29 18:36 ` Junio C Hamano
2024-04-02 7:23 ` Patrick Steinhardt
2024-04-03 0:20 ` [PATCH v4 0/2] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-04-03 0:20 ` [PATCH v4 1/2] reftable/stack: add env to disable autocompaction Justin Tobler via GitGitGadget
2024-04-03 0:20 ` [PATCH v4 2/2] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-04-03 4:47 ` [PATCH v4 0/2] " Patrick Steinhardt
2024-04-03 11:12 ` Karthik Nayak
2024-04-03 16:56 ` Junio C Hamano
2024-04-04 18:29 ` [PATCH v5 0/3] " Justin Tobler via GitGitGadget
2024-04-04 18:29 ` [PATCH v5 1/3] reftable/stack: allow disabling of auto-compaction Justin Tobler via GitGitGadget
2024-04-08 6:12 ` Patrick Steinhardt
2024-04-04 18:29 ` [PATCH v5 2/3] reftable/stack: add env to disable autocompaction Justin Tobler via GitGitGadget
2024-04-08 6:12 ` Patrick Steinhardt
2024-04-08 16:18 ` Junio C Hamano
2024-04-04 18:29 ` [PATCH v5 3/3] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-04-08 6:12 ` [PATCH v5 0/3] " Patrick Steinhardt
2024-04-08 16:17 ` Justin Tobler
2024-04-08 16:16 ` [PATCH v6 " Justin Tobler via GitGitGadget
2024-04-08 16:16 ` [PATCH v6 1/3] reftable/stack: expose option to disable auto-compaction Justin Tobler via GitGitGadget
2024-04-08 16:16 ` [PATCH v6 2/3] reftable/stack: add env to disable autocompaction Justin Tobler via GitGitGadget
2024-04-08 16:16 ` [PATCH v6 3/3] reftable/stack: use geometric table compaction Justin Tobler via GitGitGadget
2024-04-08 16:20 ` [PATCH v6 0/3] " Patrick Steinhardt
2024-04-08 19:12 ` Junio C Hamano
2024-04-03 19:12 ` [PATCH v2 " Junio C Hamano
2024-04-03 19:30 ` Patrick Steinhardt
2024-04-04 5:34 ` Patrick Steinhardt
2024-04-04 18:28 ` Justin Tobler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b24jjzw72rlcpctteokr5yfbuwuy2cc3qvibzhuju4gbj63lfa@gbtsmvufyuhd \
--to=jltobler@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=hanwenn@gmail.com \
--cc=ps@pks.im \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).