diff mbox series

[v2,02/10] reftable: stop using `strbuf_addf()`

Message ID 6a7333b275e9f7eab81568a8de939011d292a31a.1728910727.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: stop using `struct strbuf` | expand

Commit Message

Patrick Steinhardt Oct. 14, 2024, 1:02 p.m. UTC
We're about to introduce our own `reftable_buf` type to replace
`strbuf`. Get rid of the seldomly-used `strbuf_addf()` function such
that we have to reimplement one less function.

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>
---
 reftable/stack.c                    | 18 ++++++++-----
 t/unit-tests/t-reftable-block.c     |  7 +++--
 t/unit-tests/t-reftable-readwrite.c | 20 +++++++-------
 t/unit-tests/t-reftable-stack.c     | 42 ++++++++++++++++-------------
 4 files changed, 50 insertions(+), 37 deletions(-)

Comments

Taylor Blau Oct. 14, 2024, 10:32 p.m. UTC | #1
On Mon, Oct 14, 2024 at 03:02:21PM +0200, Patrick Steinhardt wrote:
> We're about to introduce our own `reftable_buf` type to replace
> `strbuf`. Get rid of the seldomly-used `strbuf_addf()` function such
> that we have to reimplement one less function.

Hmm. I count twelve calls to strbuf_addf() here in this patch that were
rewritten in terms of snprintf()ing to a temporary buffer. So I am not
sure that I agree that it is "seldomly-used".

Sure, implementing fewer functions is nice, but I am not sure that
forcing the caller to use snprintf() directly is necessarily a
worthwhile trade-off.

Part of me wishes that we didn't have to write our own `reftable_buf` in
the first place. Could we use `strbuf` as-is and expose it through a
generic reftable-specific interface that users of reftable fill in with
a vtable or something?

Thanks,
Taylor
Patrick Steinhardt Oct. 15, 2024, 4:37 a.m. UTC | #2
On Mon, Oct 14, 2024 at 06:32:07PM -0400, Taylor Blau wrote:
> On Mon, Oct 14, 2024 at 03:02:21PM +0200, Patrick Steinhardt wrote:
> > We're about to introduce our own `reftable_buf` type to replace
> > `strbuf`. Get rid of the seldomly-used `strbuf_addf()` function such
> > that we have to reimplement one less function.
> 
> Hmm. I count twelve calls to strbuf_addf() here in this patch that were
> rewritten in terms of snprintf()ing to a temporary buffer. So I am not
> sure that I agree that it is "seldomly-used".
> 
> Sure, implementing fewer functions is nice, but I am not sure that
> forcing the caller to use snprintf() directly is necessarily a
> worthwhile trade-off.

Another problem here is that snprintf() isn't exactly the most portable
interface. Some systems don't have it at all, others have broken return
code handling. So avoiding it completely makes that issue go away
entirely.

I can add that to the commit message if this needs a reroll.

> Part of me wishes that we didn't have to write our own `reftable_buf` in
> the first place. Could we use `strbuf` as-is and expose it through a
> generic reftable-specific interface that users of reftable fill in with
> a vtable or something?

I tried that, and it felt way worse. The amount of code you have to
write is roughly in the same ballpark, you don't have pluggable
allocators, you don't have allocation error handling and every consumer
would have to implement their own type.

So overall it's only losses from my point of view.

Patrick
Taylor Blau Oct. 15, 2024, 7:26 p.m. UTC | #3
On Tue, Oct 15, 2024 at 06:37:48AM +0200, Patrick Steinhardt wrote:
> > Part of me wishes that we didn't have to write our own `reftable_buf` in
> > the first place. Could we use `strbuf` as-is and expose it through a
> > generic reftable-specific interface that users of reftable fill in with
> > a vtable or something?
>
> I tried that, and it felt way worse. The amount of code you have to
> write is roughly in the same ballpark, you don't have pluggable
> allocators, you don't have allocation error handling and every consumer
> would have to implement their own type.
>
> So overall it's only losses from my point of view.

Makes sense, although the end result is somewhat unsatisfying.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/reftable/stack.c b/reftable/stack.c
index 7e617c25914..d7bc1187dfb 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1387,12 +1387,18 @@  static int stack_compact_range(struct reftable_stack *st,
 	 * have just written. In case the compacted table became empty we
 	 * simply skip writing it.
 	 */
-	for (i = 0; i < first_to_replace; i++)
-		strbuf_addf(&tables_list_buf, "%s\n", names[i]);
-	if (!is_empty_table)
-		strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf);
-	for (i = last_to_replace + 1; names[i]; i++)
-		strbuf_addf(&tables_list_buf, "%s\n", names[i]);
+	for (i = 0; i < first_to_replace; i++) {
+		strbuf_addstr(&tables_list_buf, names[i]);
+		strbuf_addstr(&tables_list_buf, "\n");
+	}
+	if (!is_empty_table) {
+		strbuf_addstr(&tables_list_buf, new_table_name.buf);
+		strbuf_addstr(&tables_list_buf, "\n");
+	}
+	for (i = last_to_replace + 1; names[i]; i++) {
+		strbuf_addstr(&tables_list_buf, names[i]);
+		strbuf_addstr(&tables_list_buf, "\n");
+	}
 
 	err = write_in_full(get_lock_file_fd(&tables_list_lock),
 			    tables_list_buf.buf, tables_list_buf.len);
diff --git a/t/unit-tests/t-reftable-block.c b/t/unit-tests/t-reftable-block.c
index d470060e8be..8077bbc5e7a 100644
--- a/t/unit-tests/t-reftable-block.c
+++ b/t/unit-tests/t-reftable-block.c
@@ -308,10 +308,13 @@  static void t_index_block_read_write(void)
 	check(!ret);
 
 	for (i = 0; i < N; i++) {
-		strbuf_init(&recs[i].u.idx.last_key, 9);
+		char buf[128];
+
+		snprintf(buf, sizeof(buf), "branch%02"PRIuMAX, (uintmax_t)i);
 
+		strbuf_init(&recs[i].u.idx.last_key, 9);
 		recs[i].type = BLOCK_TYPE_INDEX;
-		strbuf_addf(&recs[i].u.idx.last_key, "branch%02"PRIuMAX, (uintmax_t)i);
+		strbuf_addstr(&recs[i].u.idx.last_key, buf);
 		recs[i].u.idx.offset = i;
 
 		ret = block_writer_add(&bw, &recs[i]);
diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c
index 27ce84445e8..5f59b0ad6ad 100644
--- a/t/unit-tests/t-reftable-readwrite.c
+++ b/t/unit-tests/t-reftable-readwrite.c
@@ -753,12 +753,13 @@  static void t_write_multiple_indices(void)
 	struct reftable_write_options opts = {
 		.block_size = 100,
 	};
-	struct strbuf writer_buf = STRBUF_INIT, buf = STRBUF_INIT;
+	struct strbuf writer_buf = STRBUF_INIT;
 	struct reftable_block_source source = { 0 };
 	struct reftable_iterator it = { 0 };
 	const struct reftable_stats *stats;
 	struct reftable_writer *writer;
 	struct reftable_reader *reader;
+	char buf[128];
 	int err, i;
 
 	writer = t_reftable_strbuf_writer(&writer_buf, &opts);
@@ -770,9 +771,8 @@  static void t_write_multiple_indices(void)
 			.value.val1 = {i},
 		};
 
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "refs/heads/%04d", i);
-		ref.refname = buf.buf,
+		snprintf(buf, sizeof(buf), "refs/heads/%04d", i);
+		ref.refname = buf;
 
 		err = reftable_writer_add_ref(writer, &ref);
 		check(!err);
@@ -788,9 +788,8 @@  static void t_write_multiple_indices(void)
 			},
 		};
 
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "refs/heads/%04d", i);
-		log.refname = buf.buf,
+		snprintf(buf, sizeof(buf), "refs/heads/%04d", i);
+		log.refname = buf;
 
 		err = reftable_writer_add_log(writer, &log);
 		check(!err);
@@ -824,7 +823,6 @@  static void t_write_multiple_indices(void)
 	reftable_writer_free(writer);
 	reftable_reader_decref(reader);
 	strbuf_release(&writer_buf);
-	strbuf_release(&buf);
 }
 
 static void t_write_multi_level_index(void)
@@ -848,10 +846,10 @@  static void t_write_multi_level_index(void)
 			.value_type = REFTABLE_REF_VAL1,
 			.value.val1 = {i},
 		};
+		char buf[128];
 
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "refs/heads/%03" PRIuMAX, (uintmax_t)i);
-		ref.refname = buf.buf,
+		snprintf(buf, sizeof(buf), "refs/heads/%03" PRIuMAX, (uintmax_t)i);
+		ref.refname = buf;
 
 		err = reftable_writer_add_ref(writer, &ref);
 		check(!err);
diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c
index 874095b9ee2..b56ea774312 100644
--- a/t/unit-tests/t-reftable-stack.c
+++ b/t/unit-tests/t-reftable-stack.c
@@ -105,7 +105,6 @@  static int write_test_ref(struct reftable_writer *wr, void *arg)
 static void write_n_ref_tables(struct reftable_stack *st,
 			       size_t n)
 {
-	struct strbuf buf = STRBUF_INIT;
 	int disable_auto_compact;
 	int err;
 
@@ -117,10 +116,10 @@  static void write_n_ref_tables(struct reftable_stack *st,
 			.update_index = reftable_stack_next_update_index(st),
 			.value_type = REFTABLE_REF_VAL1,
 		};
+		char buf[128];
 
-		strbuf_reset(&buf);
-		strbuf_addf(&buf, "refs/heads/branch-%04"PRIuMAX, (uintmax_t)i);
-		ref.refname = buf.buf;
+		snprintf(buf, sizeof(buf), "refs/heads/branch-%04"PRIuMAX, (uintmax_t)i);
+		ref.refname = buf;
 		t_reftable_set_hash(ref.value.val1, i, GIT_SHA1_FORMAT_ID);
 
 		err = reftable_stack_add(st, &write_test_ref, &ref);
@@ -128,7 +127,6 @@  static void write_n_ref_tables(struct reftable_stack *st,
 	}
 
 	st->opts.disable_auto_compact = disable_auto_compact;
-	strbuf_release(&buf);
 }
 
 struct write_log_arg {
@@ -434,7 +432,10 @@  static void t_reftable_stack_auto_compaction_fails_gracefully(void)
 	 * Adding a new table to the stack should not be impacted by this, even
 	 * though auto-compaction will now fail.
 	 */
-	strbuf_addf(&table_path, "%s/%s.lock", dir, st->readers[0]->name);
+	strbuf_addstr(&table_path, dir);
+	strbuf_addstr(&table_path, "/");
+	strbuf_addstr(&table_path, st->readers[0]->name);
+	strbuf_addstr(&table_path, ".lock");
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
@@ -1077,8 +1078,10 @@  static void t_reftable_stack_auto_compaction_with_locked_tables(void)
 	 * 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);
+	strbuf_addstr(&buf, dir);
+	strbuf_addstr(&buf, "/");
+	strbuf_addstr(&buf, st->readers[2]->name);
+	strbuf_addstr(&buf, ".lock");
 	write_file_buf(buf.buf, "", 0);
 
 	/*
@@ -1101,7 +1104,6 @@  static void t_reftable_stack_add_performs_auto_compaction(void)
 {
 	struct reftable_write_options opts = { 0 };
 	struct reftable_stack *st = NULL;
-	struct strbuf refname = STRBUF_INIT;
 	char *dir = get_tmp_dir(__LINE__);
 	int err;
 	size_t i, n = 20;
@@ -1115,6 +1117,7 @@  static void t_reftable_stack_add_performs_auto_compaction(void)
 			.value_type = REFTABLE_REF_SYMREF,
 			.value.symref = (char *) "master",
 		};
+		char buf[128];
 
 		/*
 		 * 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)
 		 */
 		st->opts.disable_auto_compact = i != n;
 
-		strbuf_reset(&refname);
-		strbuf_addf(&refname, "branch-%04"PRIuMAX, (uintmax_t)i);
-		ref.refname = refname.buf;
+		snprintf(buf, sizeof(buf), "branch-%04"PRIuMAX, (uintmax_t)i);
+		ref.refname = buf;
 
 		err = reftable_stack_add(st, write_test_ref, &ref);
 		check(!err);
@@ -1142,7 +1144,6 @@  static void t_reftable_stack_add_performs_auto_compaction(void)
 	}
 
 	reftable_stack_destroy(st);
-	strbuf_release(&refname);
 	clear_dir(dir);
 }
 
@@ -1163,8 +1164,10 @@  static void t_reftable_stack_compaction_with_locked_tables(void)
 	check_int(st->merged->readers_len, ==, 3);
 
 	/* Lock one of the tables that we're about to compact. */
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "%s/%s.lock", dir, st->readers[1]->name);
+	strbuf_addstr(&buf, dir);
+	strbuf_addstr(&buf, "/");
+	strbuf_addstr(&buf, st->readers[1]->name);
+	strbuf_addstr(&buf, ".lock");
 	write_file_buf(buf.buf, "", 0);
 
 	/*
@@ -1321,10 +1324,13 @@  static void t_reftable_stack_reload_with_missing_table(void)
 	 * our old readers. This should trigger a partial reload of the stack,
 	 * where we try to reuse our old readers.
 	*/
-	strbuf_addf(&content, "%s\n", st->readers[0]->name);
-	strbuf_addf(&content, "%s\n", st->readers[1]->name);
+	strbuf_addstr(&content, st->readers[0]->name);
+	strbuf_addstr(&content, "\n");
+	strbuf_addstr(&content, st->readers[1]->name);
+	strbuf_addstr(&content, "\n");
 	strbuf_addstr(&content, "garbage\n");
-	strbuf_addf(&table_path, "%s.lock", st->list_file);
+	strbuf_addstr(&table_path, st->list_file);
+	strbuf_addstr(&table_path, ".lock");
 	write_file_buf(table_path.buf, content.buf, content.len);
 	err = rename(table_path.buf, st->list_file);
 	check(!err);