| Message ID | 8061b9d2fcb3e8c3d1fd641e705b9a8879e452f4.1702047081.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: small set of fixes | expand |
On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote: > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > index 0644c8ad2e..c979d177c2 100644 > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -850,6 +850,52 @@ 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; > + 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", > + }; > + char name[100]; > + > + /* > + * 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; > + > + snprintf(name, sizeof(name), "branch%04d", i); > + ref.refname = name; Is there a reason that we have to use snprintf() here and not a strbuf? I would have expected to see something like: struct strbuf buf = STRBUF_INIT; /* ... */ strbuf_addf(&buf, "branch%04d", i); ref.refname = strbuf_detach(&buf, NULL); I guess it doesn't matter too much, but I think if we can avoid using snprintf(), it's worth doing. If we must use snprintf() here, we should probably use Git's xsnprintf() instead. > + 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); You could shorten this to EXPECT(st->merged->stack_len == (i == n ? 1 : i + 1); But I like the version that you wrote here better, because it clearly indicates when we should and should not perform compaction. Thanks, Taylor
On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote: > On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote: > > +static void test_reftable_stack_add_performs_auto_compaction(void) > > +{ > > + char name[100]; > > + snprintf(name, sizeof(name), "branch%04d", i); > > + ref.refname = name; > > Is there a reason that we have to use snprintf() here and not a strbuf? > > I would have expected to see something like: > > struct strbuf buf = STRBUF_INIT; > /* ... */ > strbuf_addf(&buf, "branch%04d", i); > ref.refname = strbuf_detach(&buf, NULL); If I'm reading the code correctly, this use of strbuf would leak each time through the loop. > I guess it doesn't matter too much, but I think if we can avoid using > snprintf(), it's worth doing. If we must use snprintf() here, we should > probably use Git's xsnprintf() instead. xstrfmt() from strbuf.h would be even simpler if the intention is to allocate a new string which will be freed later. In this case, though, assuming I understand the intent, I think the more common and safe idiom in this codebase is something like this: struct strbuf name = STRBUF_INIT; strbuf_addstr(&name, "branch"); size_t len = name.len; for (...) { strbuf_setlen(&name, len); strbuf_addf(&name, "%04d", i); ref.refname = name.buf; ... } strbuf_release(&name);
On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote: > On Fri, Dec 8, 2023 at 4:35 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, Dec 08, 2023 at 03:53:10PM +0100, Patrick Steinhardt wrote: > > > +static void test_reftable_stack_add_performs_auto_compaction(void) > > > +{ > > > + char name[100]; > > > + snprintf(name, sizeof(name), "branch%04d", i); > > > + ref.refname = name; > > > > Is there a reason that we have to use snprintf() here and not a strbuf? > > > > I would have expected to see something like: > > > > struct strbuf buf = STRBUF_INIT; > > /* ... */ > > strbuf_addf(&buf, "branch%04d", i); > > ref.refname = strbuf_detach(&buf, NULL); > > If I'm reading the code correctly, this use of strbuf would leak each > time through the loop. > > > I guess it doesn't matter too much, but I think if we can avoid using > > snprintf(), it's worth doing. If we must use snprintf() here, we should > > probably use Git's xsnprintf() instead. > > xstrfmt() from strbuf.h would be even simpler if the intention is to > allocate a new string which will be freed later. > > In this case, though, assuming I understand the intent, I think the > more common and safe idiom in this codebase is something like this: > > struct strbuf name = STRBUF_INIT; > strbuf_addstr(&name, "branch"); > size_t len = name.len; > for (...) { > strbuf_setlen(&name, len); > strbuf_addf(&name, "%04d", i); > ref.refname = name.buf; > ... > } > strbuf_release(&name); Yeah, I'll convert this to use a `struct strbuf` instead. But instead of tracking the length I'll just use a `strbuf_reset()` followed by `strbuf_addf("branch-%04d")`. It's simpler to read and we don't need to squeeze every last drop of performance out of this loop anyway. Patrick
On Mon, Dec 11, 2023 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote: > On Fri, Dec 08, 2023 at 06:46:33PM -0500, Eric Sunshine wrote: > > In this case, though, assuming I understand the intent, I think the > > more common and safe idiom in this codebase is something like this: > > > > struct strbuf name = STRBUF_INIT; > > strbuf_addstr(&name, "branch"); > > size_t len = name.len; > > for (...) { > > strbuf_setlen(&name, len); > > strbuf_addf(&name, "%04d", i); > > ref.refname = name.buf; > > ... > > } > > strbuf_release(&name); > > Yeah, I'll convert this to use a `struct strbuf` instead. But instead of > tracking the length I'll just use a `strbuf_reset()` followed by > `strbuf_addf("branch-%04d")`. It's simpler to read and we don't need to > squeeze every last drop of performance out of this loop anyway. Sounds perfectly reasonable to me, and I agree with the reasoning, especially the lower cognitive load with strbuf_reset(). Thanks.
diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 0644c8ad2e..c979d177c2 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -850,6 +850,52 @@ 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; + 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", + }; + char name[100]; + + /* + * 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; + + snprintf(name, sizeof(name), "branch%04d", i); + ref.refname = name; + + 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); + clear_dir(dir); +} + static void test_reftable_stack_compaction_concurrent(void) { struct reftable_write_options cfg = { 0 }; @@ -960,6 +1006,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 | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)