Re: [PATCH 2/8] reftable/stack: test compaction with already-locked tables

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On Fri, Aug 02, 2024 at 02:05:43PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]