Subject: Re: [PATCH v6 04/20] reftable: utility functions

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 13 Apr 2021 10:02:28 +0200

On Mon, Apr 12 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> +int strbuf_add_void(void *b, const void *data, size_t sz)
> +{
> +	strbuf_add((struct strbuf *)b, data, sz);
> +	return sz;
> +}

Is that cast needed on your compiler? This compiles without warnings for
me without that.

Also, maybe this is the sort of thing that makes sense to split into
general "APIs needed for reftable" patches. E.g. something like the
below (just the strbuf.h change):

diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 0c301cecced..e49029eed34 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -43,7 +43,7 @@ static void write_test_table(struct strbuf *buf,
 		}
 	}
 
-	w = reftable_new_writer(&strbuf_add_void, buf, &opts);
+	w = reftable_new_writer(&strbuf_add_write, buf, &opts);
 	reftable_writer_set_limits(w, min, max);
 
 	for (i = 0; i < n; i++) {
@@ -241,7 +241,7 @@ static void test_default_write_opts(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 
 	struct reftable_ref_record rec = {
 		.refname = "master",
diff --git a/reftable/refname_test.c b/reftable/refname_test.c
index 5e005d6af31..e8ecba1fad9 100644
--- a/reftable/refname_test.c
+++ b/reftable/refname_test.c
@@ -31,7 +31,7 @@ static void test_conflict(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 	struct reftable_ref_record rec = {
 		.refname = "a/b",
 		.value_type = REFTABLE_REF_SYMREF,
diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 9d2f8d60555..bdef4813f4c 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -83,7 +83,7 @@ struct reftable_stats {
 
 /* reftable_new_writer creates a new writer */
 struct reftable_writer *
-reftable_new_writer(int (*writer_func)(void *, const void *, size_t),
+reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		    void *writer_arg, struct reftable_write_options *opts);
 
 /* Set the range of update indices for the records we will add. When writing a
diff --git a/reftable/reftable_test.c b/reftable/reftable_test.c
index 69dbfb09fff..1685b9a07bc 100644
--- a/reftable/reftable_test.c
+++ b/reftable/reftable_test.c
@@ -52,7 +52,7 @@ static void write_table(char ***names, struct strbuf *buf, int N,
 		.hash_id = hash_id,
 	};
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, buf, &opts);
+		reftable_new_writer(&strbuf_add_write, buf, &opts);
 	struct reftable_ref_record ref = { NULL };
 	int i = 0, n;
 	struct reftable_log_record log = { NULL };
@@ -131,7 +131,7 @@ static void test_log_buffer_size(void)
 						   .message = "commit: 9\n",
 					   } };
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 
 	/* This tests buffer extension for log compression. Must use a random
 	   hash, to ensure that the compressed part is larger than the original.
@@ -169,7 +169,7 @@ static void test_log_write_read(void)
 	struct reftable_block_source source = { NULL };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 	const struct reftable_stats *stats = NULL;
 	reftable_writer_set_limits(w, 0, N);
 	for (i = 0; i < N; i++) {
@@ -437,7 +437,7 @@ static void test_table_refs_for(int indexed)
 
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 
 	struct reftable_iterator it = { NULL };
 	int j;
@@ -534,7 +534,7 @@ static void test_table_empty(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &buf, &opts);
+		reftable_new_writer(&strbuf_add_write, &buf, &opts);
 	struct reftable_block_source source = { NULL };
 	struct reftable_reader *rd = NULL;
 	struct reftable_ref_record rec = { NULL };
diff --git a/reftable/stack.c b/reftable/stack.c
index 3cdb6e8ed33..2e3fd8db1dd 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -39,7 +39,7 @@ static void stack_filename(struct strbuf *dest, struct reftable_stack *st,
 	strbuf_addstr(dest, name);
 }
 
-static int reftable_fd_write(void *arg, const void *data, size_t sz)
+static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
 {
 	int *fdp = (int *)arg;
 	return write(*fdp, data, sz);
diff --git a/reftable/test_framework.c b/reftable/test_framework.c
index a5ff4e2a2d2..e5e39fe8b13 100644
--- a/reftable/test_framework.c
+++ b/reftable/test_framework.c
@@ -15,9 +15,3 @@ void set_test_hash(uint8_t *p, int i)
 {
 	memset(p, (uint8_t)i, hash_size(SHA1_ID));
 }
-
-int strbuf_add_void(void *b, const void *data, size_t sz)
-{
-	strbuf_add((struct strbuf *)b, data, sz);
-	return sz;
-}
diff --git a/reftable/test_framework.h b/reftable/test_framework.h
index 5fdc9519a5a..c04925ea11d 100644
--- a/reftable/test_framework.h
+++ b/reftable/test_framework.h
@@ -46,8 +46,4 @@ license that can be found in the LICENSE file or at
 
 void set_test_hash(uint8_t *p, int i);
 
-/* Like strbuf_add, but suitable for passing to reftable_new_writer
- */
-int strbuf_add_void(void *b, const void *data, size_t sz);
-
 #endif
diff --git a/reftable/writer.c b/reftable/writer.c
index d42ca8afac1..ca3b127f83d 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -118,7 +118,7 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
 static struct strbuf reftable_empty_strbuf = STRBUF_INIT;
 
 struct reftable_writer *
-reftable_new_writer(int (*writer_func)(void *, const void *, size_t),
+reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		    void *writer_arg, struct reftable_write_options *opts)
 {
 	struct reftable_writer *wp =
diff --git a/reftable/writer.h b/reftable/writer.h
index 4921c249d06..09b88673d97 100644
--- a/reftable/writer.h
+++ b/reftable/writer.h
@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at
 #include "reftable-writer.h"
 
 struct reftable_writer {
-	int (*write)(void *, const void *, size_t);
+	ssize_t (*write)(void *, const void *, size_t);
 	void *write_arg;
 	int pending_padding;
 	struct strbuf last_key;
diff --git a/strbuf.h b/strbuf.h
index 223ee2094af..e3ae5b8a9d3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -290,6 +290,18 @@ void strbuf_add_commented_lines(struct strbuf *out,
  */
 void strbuf_add(struct strbuf *sb, const void *data, size_t len);
 
+/**
+ * Like strbuf_add() but emulates write() for APIs that need
+ * it. Returns the passed-in `len` as-is. The `void *` is really a
+ * `struct strbuf *`.
+ */
+static inline ssize_t strbuf_add_write(void *sb, const void *data,
+				       size_t len)
+{
+	strbuf_add(sb, data, len);
+	return len;
+}
+
 /**
  * Add a NUL-terminated string to the buffer.
  *