| Message ID | 5e27d0a5566d90969734e92984cfafe6048924f4.1702285387.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 15f98b602f36ce5e5cc1f466850eaf2b37022e3b |
| Headers | show |
| Series | reftable: small set of fixes | expand |
On Mon, Dec 11, 2023 at 10:07:42AM +0100, Patrick Steinhardt wrote: > While we have several tests that check whether we correctly perform > auto-compaction when manually calling `reftable_stack_auto_compact()`, > we don't have any tests that verify whether `reftable_stack_add()` does > call it automatically. Add one. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > index 0644c8ad2e..52b4dc3b14 100644 > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void) > clear_dir(dir); > } > > +static void test_reftable_stack_add_performs_auto_compaction(void) > +{ > + struct reftable_write_options cfg = { 0 }; > + struct reftable_stack *st = NULL; > + struct strbuf refname = STRBUF_INIT; > + char *dir = get_tmp_dir(__LINE__); > + int err, i, n = 20; > + > + err = reftable_new_stack(&st, dir, cfg); > + EXPECT_ERR(err); > + > + for (i = 0; i <= n; i++) { > + struct reftable_ref_record ref = { > + .update_index = reftable_stack_next_update_index(st), > + .value_type = REFTABLE_REF_SYMREF, > + .value.symref = "master", > + }; > + > + /* > + * Disable auto-compaction for all but the last runs. Like this > + * we can ensure that we indeed honor this setting and have > + * better control over when exactly auto compaction runs. > + */ > + st->disable_auto_compact = i != n; > + > + strbuf_reset(&refname); > + strbuf_addf(&refname, "branch-%04d", i); > + ref.refname = refname.buf; Does the reftable backend take ownership of the "refname" field? If so, then I think we'd want to use strbuf_detach() here to avoid a double-free() when you call strbuf_release() below. Thanks, Taylor
On Mon, Dec 11, 2023 at 03:15:19PM -0500, Taylor Blau wrote: > On Mon, Dec 11, 2023 at 10:07:42AM +0100, Patrick Steinhardt wrote: > > While we have several tests that check whether we correctly perform > > auto-compaction when manually calling `reftable_stack_auto_compact()`, > > we don't have any tests that verify whether `reftable_stack_add()` does > > call it automatically. Add one. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > > index 0644c8ad2e..52b4dc3b14 100644 > > --- a/reftable/stack_test.c > > +++ b/reftable/stack_test.c > > @@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void) > > clear_dir(dir); > > } > > > > +static void test_reftable_stack_add_performs_auto_compaction(void) > > +{ > > + struct reftable_write_options cfg = { 0 }; > > + struct reftable_stack *st = NULL; > > + struct strbuf refname = STRBUF_INIT; > > + char *dir = get_tmp_dir(__LINE__); > > + int err, i, n = 20; > > + > > + err = reftable_new_stack(&st, dir, cfg); > > + EXPECT_ERR(err); > > + > > + for (i = 0; i <= n; i++) { > > + struct reftable_ref_record ref = { > > + .update_index = reftable_stack_next_update_index(st), > > + .value_type = REFTABLE_REF_SYMREF, > > + .value.symref = "master", > > + }; > > + > > + /* > > + * Disable auto-compaction for all but the last runs. Like this > > + * we can ensure that we indeed honor this setting and have > > + * better control over when exactly auto compaction runs. > > + */ > > + st->disable_auto_compact = i != n; > > + > > + strbuf_reset(&refname); > > + strbuf_addf(&refname, "branch-%04d", i); > > + ref.refname = refname.buf; > > Does the reftable backend take ownership of the "refname" field? If so, > then I think we'd want to use strbuf_detach() here to avoid a > double-free() when you call strbuf_release() below. No it doesn't. `reftable_stack_add()` will lock the stack and commit the new table immediately, so there's no need to transfer ownership of memory here. Patrick
diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 0644c8ad2e..52b4dc3b14 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -850,6 +850,54 @@ static void test_reftable_stack_auto_compaction(void) clear_dir(dir); } +static void test_reftable_stack_add_performs_auto_compaction(void) +{ + struct reftable_write_options cfg = { 0 }; + struct reftable_stack *st = NULL; + struct strbuf refname = STRBUF_INIT; + char *dir = get_tmp_dir(__LINE__); + int err, i, n = 20; + + err = reftable_new_stack(&st, dir, cfg); + EXPECT_ERR(err); + + for (i = 0; i <= n; i++) { + struct reftable_ref_record ref = { + .update_index = reftable_stack_next_update_index(st), + .value_type = REFTABLE_REF_SYMREF, + .value.symref = "master", + }; + + /* + * Disable auto-compaction for all but the last runs. Like this + * we can ensure that we indeed honor this setting and have + * better control over when exactly auto compaction runs. + */ + st->disable_auto_compact = i != n; + + strbuf_reset(&refname); + strbuf_addf(&refname, "branch-%04d", i); + ref.refname = refname.buf; + + err = reftable_stack_add(st, &write_test_ref, &ref); + EXPECT_ERR(err); + + /* + * The stack length should grow continuously for all runs where + * auto compaction is disabled. When enabled, we should merge + * all tables in the stack. + */ + if (i != n) + EXPECT(st->merged->stack_len == i + 1); + else + EXPECT(st->merged->stack_len == 1); + } + + reftable_stack_destroy(st); + strbuf_release(&refname); + clear_dir(dir); +} + static void test_reftable_stack_compaction_concurrent(void) { struct reftable_write_options cfg = { 0 }; @@ -960,6 +1008,7 @@ int stack_test_main(int argc, const char *argv[]) RUN_TEST(test_reftable_stack_add); RUN_TEST(test_reftable_stack_add_one); RUN_TEST(test_reftable_stack_auto_compaction); + RUN_TEST(test_reftable_stack_add_performs_auto_compaction); RUN_TEST(test_reftable_stack_compaction_concurrent); RUN_TEST(test_reftable_stack_compaction_concurrent_clean); RUN_TEST(test_reftable_stack_hash_id);
While we have several tests that check whether we correctly perform auto-compaction when manually calling `reftable_stack_auto_compact()`, we don't have any tests that verify whether `reftable_stack_add()` does call it automatically. Add one. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)