@matheusbsbs / matheus-bagliano / commits / 7fa7e14ebe

reftable: stop using `strbuf_addf()`

We're about to introduce our own `reftable_buf` type to replace `strbuf`. One function we'll have to convert is `strbuf_addf()`, which is used in a handful of places. This function uses `snprintf()` internally, which makes porting it a bit more involved: - It is not available on all platforms. - Some platforms like Windows have broken implementations. So by using `snprintf()` we'd also push the burden on downstream users of the reftable library to make available a properly working version of it. Most callsites of `strbuf_addf()` are trivial to convert to not using it. We do end up using `snprintf()` in our unit tests, but that isn't much of a problem for downstream users of the reftable library. While at it, remove a useless call to `strbuf_reset()` in `t_reftable_stack_auto_compaction_with_locked_tables()`. We don't write to the buffer before this and initialize it with `STRBUF_INIT`, so there is no need to reset anything. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Patrick Steinhardt committed Oct 17, 2024 at 06:53 UTC 7fa7e14ebee9d82545bb4a8dcc1cac22ef51cfed

4 files changed +50 -37

reftable/stack.c

+12 -6

@@ -1387,12 +1387,18 @@ static int stack_compact_range(struct reftable_stack *st,

1387 * have just written. In case the compacted table became empty we

1388 * simply skip writing it.

1389 */

1390 - for (i = 0; i < first_to_replace; i++)

1391 - strbuf_addf(&tables_list_buf, "%s\n", names[i]);

1392 - if (!is_empty_table)

1393 - strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf);

1394 - for (i = last_to_replace + 1; names[i]; i++)

1395 - strbuf_addf(&tables_list_buf, "%s\n", names[i]);

1390 + for (i = 0; i < first_to_replace; i++) {

1391 + strbuf_addstr(&tables_list_buf, names[i]);

1392 + strbuf_addstr(&tables_list_buf, "\n");

1393 + }

1394 + if (!is_empty_table) {

1395 + strbuf_addstr(&tables_list_buf, new_table_name.buf);

1396 + strbuf_addstr(&tables_list_buf, "\n");

1397 + }

1398 + for (i = last_to_replace + 1; names[i]; i++) {

1399 + strbuf_addstr(&tables_list_buf, names[i]);

1400 + strbuf_addstr(&tables_list_buf, "\n");

1401 + }

1402

1403 err = write_in_full(get_lock_file_fd(&tables_list_lock),

1404 tables_list_buf.buf, tables_list_buf.len);

t/unit-tests/t-reftable-block.c

+5 -2

@@ -308,10 +308,13 @@ static void t_index_block_read_write(void)

308 check(!ret);

309

310 for (i = 0; i < N; i++) {

311 - strbuf_init(&recs[i].u.idx.last_key, 9);

311 + char buf[128];

312 +

313 + snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i);

314

315 + strbuf_init(&recs[i].u.idx.last_key, 9);

316 recs[i].type = BLOCK_TYPE_INDEX;

314 - strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);

317 + strbuf_addstr(&recs[i].u.idx.last_key, buf);

318 recs[i].u.idx.offset = i;

319

320 ret = block_writer_add(&bw, &recs[i]);

t/unit-tests/t-reftable-readwrite.c

+9 -11

@@ -753,12 +753,13 @@ static void t_write_multiple_indices(void)

753 struct reftable_write_options opts = {

754 .block_size = 100,

755 };

756 - struct strbuf writer_buf = STRBUF_INIT, buf = STRBUF_INIT;

756 + struct strbuf writer_buf = STRBUF_INIT;

757 struct reftable_block_source source = { 0 };

758 struct reftable_iterator it = { 0 };

759 const struct reftable_stats *stats;

760 struct reftable_writer *writer;

761 struct reftable_reader *reader;

762 + char buf[128];

763 int err, i;

764

765 writer = t_reftable_strbuf_writer(&writer_buf, &opts);

@@ -770,9 +771,8 @@ static void t_write_multiple_indices(void)

771 .value.val1 = {i},

772 };

773

773 - strbuf_reset(&buf);

774 - strbuf_addf(&buf, "refs/heads/%04d", i);

775 - ref.refname = buf.buf,

774 + snprintf(buf, sizeof(buf), "refs/heads/%04d", i);

775 + ref.refname = buf;

776

777 err = reftable_writer_add_ref(writer, &ref);

778 check(!err);

@@ -788,9 +788,8 @@ static void t_write_multiple_indices(void)

788 },

789 };

790

791 - strbuf_reset(&buf);

792 - strbuf_addf(&buf, "refs/heads/%04d", i);

793 - log.refname = buf.buf,

791 + snprintf(buf, sizeof(buf), "refs/heads/%04d", i);

792 + log.refname = buf;

793

794 err = reftable_writer_add_log(writer, &log);

795 check(!err);

@@ -824,7 +823,6 @@ static void t_write_multiple_indices(void)

823 reftable_writer_free(writer);

824 reftable_reader_decref(reader);

825 strbuf_release(&writer_buf);

827 - strbuf_release(&buf);

826 }

827

828 static void t_write_multi_level_index(void)

@@ -848,10 +846,10 @@ static void t_write_multi_level_index(void)

846 .value_type = REFTABLE_REF_VAL1,

847 .value.val1 = {i},

848 };

849 + char buf[128];

850

852 - strbuf_reset(&buf);

853 - strbuf_addf(&buf, "refs/heads/%03" PRIuMAX, (uintmax_t)i);

854 - ref.refname = buf.buf,

851 + snprintf(buf, sizeof(buf), "refs/heads/%03" PRIuMAX, (uintmax_t)i);

852 + ref.refname = buf;

853

854 err = reftable_writer_add_ref(writer, &ref);

855 check(!err);

t/unit-tests/t-reftable-stack.c

+24 -18

@@ -105,7 +105,6 @@ static int write_test_ref(struct reftable_writer *wr, void *arg)

105 static void write_n_ref_tables(struct reftable_stack *st,

106 size_t n)

107 {

108 - struct strbuf buf = STRBUF_INIT;

108 int disable_auto_compact;

109 int err;

110

@@ -117,10 +116,10 @@ static void write_n_ref_tables(struct reftable_stack *st,

116 .update_index = reftable_stack_next_update_index(st),

117 .value_type = REFTABLE_REF_VAL1,

118 };

119 + char buf[128];

120

121 - strbuf_reset(&buf);

122 - strbuf_addf(&buf, "refs/heads/branch-%04"PRIuMAX, (uintmax_t)i);

123 - ref.refname = buf.buf;

121 + snprintf(buf, sizeof(buf), "refs/heads/branch-%04"PRIuMAX, (uintmax_t)i);

122 + ref.refname = buf;

123 t_reftable_set_hash(ref.value.val1, i, GIT_SHA1_FORMAT_ID);

124

125 err = reftable_stack_add(st, &write_test_ref, &ref);

@@ -128,7 +127,6 @@ static void write_n_ref_tables(struct reftable_stack *st,

127 }

128

129 st->opts.disable_auto_compact = disable_auto_compact;

131 - strbuf_release(&buf);

130 }

131

132 struct write_log_arg {

@@ -434,7 +432,10 @@ static void t_reftable_stack_auto_compaction_fails_gracefully(void)

432 * Adding a new table to the stack should not be impacted by this, even

433 * though auto-compaction will now fail.

434 */

437 - strbuf_addf(&table_path, "%s/%s.lock", dir, st->readers[0]->name);

435 + strbuf_addstr(&table_path, dir);

436 + strbuf_addstr(&table_path, "/");

437 + strbuf_addstr(&table_path, st->readers[0]->name);

438 + strbuf_addstr(&table_path, ".lock");

439 write_file_buf(table_path.buf, "", 0);

440

441 ref.update_index = 2;

@@ -1077,8 +1078,10 @@ static void t_reftable_stack_auto_compaction_with_locked_tables(void)

1078 * size, we expect that auto-compaction will want to compact all of the

1079 * tables. Locking any of the tables will keep it from doing so.

1080 */

1080 - strbuf_reset(&buf);

1081 - strbuf_addf(&buf, "%s/%s.lock", dir, st->readers[2]->name);

1081 + strbuf_addstr(&buf, dir);

1082 + strbuf_addstr(&buf, "/");

1083 + strbuf_addstr(&buf, st->readers[2]->name);

1084 + strbuf_addstr(&buf, ".lock");

1085 write_file_buf(buf.buf, "", 0);

1086

1087 /*

@@ -1101,7 +1104,6 @@ static void t_reftable_stack_add_performs_auto_compaction(void)

1104 {

1105 struct reftable_write_options opts = { 0 };

1106 struct reftable_stack *st = NULL;

1104 - struct strbuf refname = STRBUF_INIT;

1107 char *dir = get_tmp_dir(__LINE__);

1108 int err;

1109 size_t i, n = 20;

@@ -1115,6 +1117,7 @@ static void t_reftable_stack_add_performs_auto_compaction(void)

1117 .value_type = REFTABLE_REF_SYMREF,

1118 .value.symref = (char *) "master",

1119 };

1120 + char buf[128];

1121

1122 /*

1123 * Disable auto-compaction for all but the last runs. Like this

@@ -1123,9 +1126,8 @@ static void t_reftable_stack_add_performs_auto_compaction(void)

1126 */

1127 st->opts.disable_auto_compact = i != n;

1128

1126 - strbuf_reset(&refname);

1127 - strbuf_addf(&refname, "branch-%04"PRIuMAX, (uintmax_t)i);

1128 - ref.refname = refname.buf;

1129 + snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);

1130 + ref.refname = buf;

1131

1132 err = reftable_stack_add(st, write_test_ref, &ref);

1133 check(!err);

@@ -1142,7 +1144,6 @@ static void t_reftable_stack_add_performs_auto_compaction(void)

1144 }

1145

1146 reftable_stack_destroy(st);

1145 - strbuf_release(&refname);

1147 clear_dir(dir);

1148 }

1149

@@ -1163,8 +1164,10 @@ static void t_reftable_stack_compaction_with_locked_tables(void)

1164 check_int(st->merged->readers_len, ==, 3);

1165

1166 /* Lock one of the tables that we're about to compact. */

1166 - strbuf_reset(&buf);

1167 - strbuf_addf(&buf, "%s/%s.lock", dir, st->readers[1]->name);

1167 + strbuf_addstr(&buf, dir);

1168 + strbuf_addstr(&buf, "/");

1169 + strbuf_addstr(&buf, st->readers[1]->name);

1170 + strbuf_addstr(&buf, ".lock");

1171 write_file_buf(buf.buf, "", 0);

1172

1173 /*

@@ -1321,10 +1324,13 @@ static void t_reftable_stack_reload_with_missing_table(void)

1324 * our old readers. This should trigger a partial reload of the stack,

1325 * where we try to reuse our old readers.

1326 */

1324 - strbuf_addf(&content, "%s\n", st->readers[0]->name);

1325 - strbuf_addf(&content, "%s\n", st->readers[1]->name);

1327 + strbuf_addstr(&content, st->readers[0]->name);

1328 + strbuf_addstr(&content, "\n");

1329 + strbuf_addstr(&content, st->readers[1]->name);

1330 + strbuf_addstr(&content, "\n");

1331 strbuf_addstr(&content, "garbage\n");

1327 - strbuf_addf(&table_path, "%s.lock", st->list_file);

1332 + strbuf_addstr(&table_path, st->list_file);

1333 + strbuf_addstr(&table_path, ".lock");

1334 write_file_buf(table_path.buf, content.buf, content.len);

1335 err = rename(table_path.buf, st->list_file);

1336 check(!err);