On Fri, Aug 02, 2024 at 02:05:43PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
> > + ref.refname = buf.buf;
> > +
> > + err = reftable_stack_add(st, &write_test_ref, &ref);
> > + EXPECT_ERR(err);
> > + }
> > + EXPECT(st->merged->stack_len == 5);
> > +
> > + /*
> > + * Given that all tables we have written should be roughly the same
> > + * size, we expect that auto-compaction will want to compact all of the
> > + * tables. Locking any of the tables will keep it from doing so.
> > + */
> > + strbuf_reset(&buf);
> > + strbuf_addf(&buf, "%s/%s.lock", dir, st->readers[2]->name);
> > + write_file_buf(buf.buf, "", 0);
>
> OK. [2] is just a random number pulled out of 0..5?
Not quite as random. It is picked such that we can demonstrate in a
follow-up patch that auto-compaction knows to pack tables 4 and 5, while
leaking tables 1 to 3 intact. This only becomes important in a follow up
patch where we change the backing logic.
> > +static void test_reftable_stack_compaction_with_locked_tables(void)
> > +{
> > + struct reftable_write_options opts = {
> > + .disable_auto_compact = 1,
> > + };
> > + struct reftable_stack *st = NULL;
> > + struct strbuf buf = STRBUF_INIT;
> > + char *dir = get_tmp_dir(__LINE__);
> > + int err;
> > +
> > + err = reftable_new_stack(&st, dir, &opts);
> > + EXPECT_ERR(err);
> > +
> > + for (size_t i = 0; i < 3; i++) {
> > +...
> > + }
> > + EXPECT(st->merged->stack_len == 3);
>
> Hmph, this somehow looks familiar. The only difference is how many
> tables are compacted with which one locked, and whether it is
> compact_all() or auto_compact() that triggers the compaction
> behaviour, right?
>
> I wonder if we want to factor out the commonality into a shared
> function, or it is too much trouble only for two duplicates and we
> can worry about it when we were about to add the third one?
I was also briefly thinking the same, but then didn't follow through
with that thought. In fact, there's multiple places in this file where
we populate a stack with N tables. I think it should be easy enough to
pull this into a function indeed.
Patrick
Attachment:
signature.asc
Description: PGP signature
