diff mbox series

[v2,02/11] reftable: consistently pass write opts as value

Message ID e6f8fc09c282e620a331a737c30a190107c76996.1715336798.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: expose write options as config | expand

Commit Message

Patrick Steinhardt May 10, 2024, 10:29 a.m. UTC 
We sometimes pass the refatble write options as value and sometimes as a
pointer. This is quite confusing and makes the reader wonder whether the
options get modified sometimes.

In fact, `reftable_new_writer()` does cause the caller-provided options
to get updated when some values aren't set up. This is quite unexpected,
but didn't cause any harm until now.

Refactor the code to consistently pass the options as a value so that
they are local to the subsystem they are being passed into so that we
can avoid weirdness like this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/merged_test.c     |  6 +++---
 reftable/readwrite_test.c  | 26 +++++++++++++-------------
 reftable/refname_test.c    |  2 +-
 reftable/reftable-writer.h |  2 +-
 reftable/stack.c           |  4 ++--
 reftable/writer.c          | 12 +++++++-----
 6 files changed, 27 insertions(+), 25 deletions(-)

Comments

Junio C Hamano May 10, 2024, 9:11 p.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

> We sometimes pass the refatble write options as value and sometimes as a
> pointer. This is quite confusing and makes the reader wonder whether the
> options get modified sometimes.
>
> In fact, `reftable_new_writer()` does cause the caller-provided options
> to get updated when some values aren't set up. This is quite unexpected,
> but didn't cause any harm until now.
>
> Refactor the code to consistently pass the options as a value so that
> they are local to the subsystem they are being passed into so that we
> can avoid weirdness like this.

Turning pass-by-reference to pass-by-value of a large structure is a
rather huge hammer to ensure that the structure is not modified
(qualifying the pointer with "const" is).  Consistency is good, but
I am not sure offhand if this is making things consistent in a good
way.
Patrick Steinhardt May 13, 2024, 7:53 a.m. UTC | #2 
On Fri, May 10, 2024 at 02:11:55PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We sometimes pass the refatble write options as value and sometimes as a
> > pointer. This is quite confusing and makes the reader wonder whether the
> > options get modified sometimes.
> >
> > In fact, `reftable_new_writer()` does cause the caller-provided options
> > to get updated when some values aren't set up. This is quite unexpected,
> > but didn't cause any harm until now.
> >
> > Refactor the code to consistently pass the options as a value so that
> > they are local to the subsystem they are being passed into so that we
> > can avoid weirdness like this.
> 
> Turning pass-by-reference to pass-by-value of a large structure is a
> rather huge hammer to ensure that the structure is not modified
> (qualifying the pointer with "const" is).  Consistency is good, but
> I am not sure offhand if this is making things consistent in a good
> way.

Yeah, I've also been a bit "meh" about this. I'm happy to convert this
to all be const pointers instead.

Patrick
diff mbox series

Patch

diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 530fc82d1c..4ac81de8d4 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -42,7 +42,7 @@  static void write_test_table(struct strbuf *buf,
 		}
 	}
 
-	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
+	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, opts);
 	reftable_writer_set_limits(w, min, max);
 
 	for (i = 0; i < n; i++) {
@@ -70,7 +70,7 @@  static void write_test_log_table(struct strbuf *buf,
 		.exact_log_message = 1,
 	};
 	struct reftable_writer *w = NULL;
-	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, &opts);
+	w = reftable_new_writer(&strbuf_add_void, &noop_flush, buf, opts);
 	reftable_writer_set_limits(w, update_index, update_index);
 
 	for (i = 0; i < n; i++) {
@@ -403,7 +403,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, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 
 	struct reftable_ref_record rec = {
 		.refname = "master",
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index a6dbd214c5..27631a041b 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -51,7 +51,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, &noop_flush, buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, buf, opts);
 	struct reftable_ref_record ref = { NULL };
 	int i = 0, n;
 	struct reftable_log_record log = { NULL };
@@ -129,7 +129,7 @@  static void test_log_buffer_size(void)
 					   .message = "commit: 9\n",
 				   } } };
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &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.
@@ -172,7 +172,7 @@  static void test_log_overflow(void)
 		},
 	};
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 
 	memset(msg, 'x', sizeof(msg) - 1);
 	reftable_writer_set_limits(w, update_index, update_index);
@@ -199,7 +199,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, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	const struct reftable_stats *stats = NULL;
 	reftable_writer_set_limits(w, 0, N);
 	for (i = 0; i < N; i++) {
@@ -288,7 +288,7 @@  static void test_log_zlib_corruption(void)
 	struct reftable_block_source source = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	const struct reftable_stats *stats = NULL;
 	char message[100] = { 0 };
 	int err, i, n;
@@ -526,7 +526,7 @@  static void test_table_refs_for(int indexed)
 
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 
 	struct reftable_iterator it = { NULL };
 	int j;
@@ -619,7 +619,7 @@  static void test_write_empty_table(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	struct reftable_block_source source = { NULL };
 	struct reftable_reader *rd = NULL;
 	struct reftable_ref_record rec = { NULL };
@@ -657,7 +657,7 @@  static void test_write_object_id_min_length(void)
 	};
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	struct reftable_ref_record ref = {
 		.update_index = 1,
 		.value_type = REFTABLE_REF_VAL1,
@@ -692,7 +692,7 @@  static void test_write_object_id_length(void)
 	};
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	struct reftable_ref_record ref = {
 		.update_index = 1,
 		.value_type = REFTABLE_REF_VAL1,
@@ -726,7 +726,7 @@  static void test_write_empty_key(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	struct reftable_ref_record ref = {
 		.refname = "",
 		.update_index = 1,
@@ -749,7 +749,7 @@  static void test_write_key_order(void)
 	struct reftable_write_options opts = { 0 };
 	struct strbuf buf = STRBUF_INIT;
 	struct reftable_writer *w =
-		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &buf, opts);
 	struct reftable_ref_record refs[2] = {
 		{
 			.refname = "b",
@@ -792,7 +792,7 @@  static void test_write_multiple_indices(void)
 	struct reftable_reader *reader;
 	int err, i;
 
-	writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);
+	writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, opts);
 	reftable_writer_set_limits(writer, 1, 1);
 	for (i = 0; i < 100; i++) {
 		struct reftable_ref_record ref = {
@@ -869,7 +869,7 @@  static void test_write_multi_level_index(void)
 	struct reftable_reader *reader;
 	int err;
 
-	writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, &opts);
+	writer = reftable_new_writer(&strbuf_add_void, &noop_flush, &writer_buf, opts);
 	reftable_writer_set_limits(writer, 1, 1);
 	for (size_t i = 0; i < 200; i++) {
 		struct reftable_ref_record ref = {
diff --git a/reftable/refname_test.c b/reftable/refname_test.c
index b9cc62554e..3468253be7 100644
--- a/reftable/refname_test.c
+++ b/reftable/refname_test.c
@@ -30,7 +30,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, &noop_flush, &buf, &opts);
+		reftable_new_writer(&strbuf_add_void, &noop_flush, &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 155bf0bbe2..44cb986465 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -92,7 +92,7 @@  struct reftable_stats {
 struct reftable_writer *
 reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		    int (*flush_func)(void *),
-		    void *writer_arg, struct reftable_write_options *opts);
+		    void *writer_arg, struct reftable_write_options opts);
 
 /* Set the range of update indices for the records we will add. When writing a
    table into a stack, the min should be at least
diff --git a/reftable/stack.c b/reftable/stack.c
index 3979657793..7b4fff7c9e 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -767,7 +767,7 @@  int reftable_addition_add(struct reftable_addition *add,
 	tab_fd = get_tempfile_fd(tab_file);
 
 	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
-				 &add->stack->opts);
+				 add->stack->opts);
 	err = write_table(wr, arg);
 	if (err < 0)
 		goto done;
@@ -861,7 +861,7 @@  static int stack_compact_locked(struct reftable_stack *st,
 	}
 
 	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,
-				 &tab_fd, &st->opts);
+				 &tab_fd, st->opts);
 	err = stack_write_compact(st, wr, first, last, config);
 	if (err < 0)
 		goto done;
diff --git a/reftable/writer.c b/reftable/writer.c
index 1d9ff0fbfa..ad2f2e6c65 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -122,20 +122,22 @@  static struct strbuf reftable_empty_strbuf = STRBUF_INIT;
 struct reftable_writer *
 reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		    int (*flush_func)(void *),
-		    void *writer_arg, struct reftable_write_options *opts)
+		    void *writer_arg, struct reftable_write_options opts)
 {
 	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
 	strbuf_init(&wp->block_writer_data.last_key, 0);
-	options_set_defaults(opts);
-	if (opts->block_size >= (1 << 24)) {
+
+	options_set_defaults(&opts);
+	if (opts.block_size >= (1 << 24)) {
 		/* TODO - error return? */
 		abort();
 	}
+
 	wp->last_key = reftable_empty_strbuf;
-	REFTABLE_CALLOC_ARRAY(wp->block, opts->block_size);
+	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
 	wp->write = writer_func;
 	wp->write_arg = writer_arg;
-	wp->opts = *opts;
+	wp->opts = opts;
 	wp->flush = flush_func;
 	writer_reinit_block_writer(wp, BLOCK_TYPE_REF);