reftable: expose write options as config

Hi,

the reftable format has some flexibility with regards to how exactly it writes the respective tables:

- The block size allows you to control how large each block is supposed to be. The bigger the block, the more records you can fit into it.

- Restart intervals control how often a restart point is written that breaks prefix compression. The lower the interval, the less disk space savings you get.

- Object indices can be enabled or disabled. These are optional and Git doesn't use them right now, so disabling them may be a sensible thing to do if you want to save some disk space.

- The geometric factor controls when we compact tables during auto compaction.

This patch series exposes all of these via a new set of configs so that they can be tweaked by the user as-needed. It's not expected that those are really of much importance for the "normal" user -- the defaults should be good enough. But for edge cases (huge repos with millions of refs) and for hosting providers these knobs can be helpful.

This patch series applies on top of d4cc1ec35f (Start the 2.46 cycle, 2024-04-30).

Patrick

Patrick Steinhardt (11):
reftable: consistently refer to reftable_write_options as opts
reftable: consistently pass write opts as value reftable/writer: drop static variable used to initialize strbuf reftable/writer: improve error when passed an invalid block size reftable/dump: support dumping a table's block structure refs/reftable: allow configuring block size
reftable: use uint16_t to track restart interval refs/reftable: allow configuring restart interval refs/reftable: allow disabling writing the object index
reftable: make the compaction factor configurable refs/reftable: allow configuring geometric factor

 Documentation/config.txt          |   2 +
 Documentation/config/reftable.txt |  49 +++++
 refs/reftable-backend.c           |  46 ++++-
 reftable/block.h                  |   2 +-
 reftable/dump.c                   |  12 +-
 reftable/merged_test.c            |   6 +-
 reftable/reader.c                 |  63 +++++++
 reftable/readwrite_test.c         |  26 +--
 reftable/refname_test.c           |   2 +-
 reftable/reftable-reader.h        |   2 +
 reftable/reftable-stack.h         |   2 +-
 reftable/reftable-writer.h        |  10 +-
 reftable/stack.c                  |  56 +++---
 reftable/stack.h                  |   5 +-
 reftable/stack_test.c             | 118 ++++++------
 reftable/writer.c                 |  20 +--
 t/t0613-reftable-write-options.sh | 286 ++++++++++++++++++++++++++++++
 17 files changed, 577 insertions(+), 130 deletions(-)
 create mode 100644 Documentation/config/reftable.txt
 create mode 100755 t/t0613-reftable-write-options.sh

base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377

2.45.0

Throughout the reftable library the reftable_write_options are sometimes referred to as cfg and sometimes as opts. Unify these to consistently use opts to avoid confusion.

While at it, touch up the coding style a bit by removing unneeded braces around one-line statements and newlines between variable declarations.

---
 reftable/dump.c           |   4 +-
 reftable/reftable-stack.h |   2 +-
 reftable/stack.c          |  43 +++++++-------
 reftable/stack.h          |   2 +-
 reftable/stack_test.c     | 114 +++++++++++++++++---------------------
 5 files changed, 75 insertions(+), 90 deletions(-)

index 26e0393c7d..9c770a10cc 100644
@@ -27,9 +27,9 @@ license that can be found in the LICENSE file or at
 static int compact_stack(const char *stackdir)
 {
 	struct reftable_stack *stack = NULL;
-	struct reftable_write_options cfg = { 0 };
+	struct reftable_write_options opts = { 0 };
 
-	int err = reftable_new_stack(&stack, stackdir, cfg);
+	int err = reftable_new_stack(&stack, stackdir, opts);
 	if (err < 0)
 		goto done;
 

index 1b602dda58..9c8e4eef49 100644
@@ -29,7 +29,7 @@ struct reftable_stack;
  *  stored in 'dir'. Typically, this should be .git/reftables.
  */
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       struct reftable_write_options config);
+		       struct reftable_write_options opts);
 
 /* returns the update_index at which a next table should be written. */
 uint64_t reftable_stack_next_update_index(struct reftable_stack *st);

index d43efa4760..97d7ebc043 100644
@@ -20,7 +20,7 @@ struct reftable_stack {
 
 	char *reftable_dir;
 
-	struct reftable_write_options config;
+	struct reftable_write_options opts;
 
 	struct reftable_reader **readers;
 	size_t readers_len;

2.45.0

I think one location was missed:

index 3316d55f19..40eb793b3c 100644
@@ -396,7 +396,7 @@ static void

test_reftable_stack_auto_compaction_fails_gracefully(void)

static void test_reftable_stack_validate_refname(void)
{
- struct reftable_write_options cfg = { 0 };
+ struct reftable_write_options opts = { 0 };
struct reftable_stack *st = NULL;
int err;
char *dir = get_tmp_dir(__LINE__);
@@ -410,7 +410,7 @@ static void test_reftable_stack_validate_refname(void)
};
char *additions[] = { "a", "a/b/c" };

- err = reftable_new_stack(&st, dir, cfg);
+ err = reftable_new_stack(&st, dir, opts);
EXPECT_ERR(err);

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

Rest of the patch looks good. Thanks

This section has been removed in 485c63cf5c (reftable: remove name checks, 2024-04-08), so it doesn't exist in master anymore. Which is cheating a bit because the topic does not build on top of anything where that commit would be reachable. But both rebasing the topic and willfully creating a conflict would probably make Junio's life harder now, so I'll just leave it at that and lie by ommission.

But thanks anyway for reading this carefully and double checking the results!

Patrick

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.

---
 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(-)

index 530fc82d1c..4ac81de8d4 100644
@@ -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",

index a6dbd214c5..27631a041b 100644
@@ -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 = {

index b9cc62554e..3468253be7 100644
@@ -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,

index 155bf0bbe2..44cb986465 100644
@@ -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

index 3979657793..7b4fff7c9e 100644
@@ -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;

index 1d9ff0fbfa..ad2f2e6c65 100644
@@ -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);
 

2.45.0

We have a static variable in the reftable writer code that is merely used to initialize the last_key of the writer. Convert the code to instead use strbuf_init() and drop the variable.

---
 reftable/writer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index ad2f2e6c65..7df6e53699 100644
@@ -117,15 +117,12 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
 	w->block_writer->restart_interval = w->opts.restart_interval;
 }
 
-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)
 {
 	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)) {

@@ -133,7 +130,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		abort();
 	}
 
-	wp->last_key = reftable_empty_strbuf;
+	strbuf_init(&wp->block_writer_data.last_key, 0);
+	strbuf_init(&wp->last_key, 0);
 	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
 	wp->write = writer_func;
 	wp->write_arg = writer_arg;

2.45.0

The reftable format only supports block sizes up to 16MB. When the writer is being passed a value bigger than that it simply calls abort(3P), which isn't all that helpful due to the lack of a proper error message.

Improve this by calling BUG() instead.

---
 reftable/writer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 7df6e53699..374b7d15ed 100644
@@ -125,10 +125,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
 
 	options_set_defaults(&opts);
-	if (opts.block_size >= (1 << 24)) {
-		/* TODO - error return? */
-		abort();
-	}
+	if (opts.block_size >= (1 << 24))
+		BUG("configured block size exceeds 16MB");
 
 	strbuf_init(&wp->block_writer_data.last_key, 0);
 	strbuf_init(&wp->last_key, 0);

2.45.0

We're about to introduce new configs that will allow users to have more control over how exactly reftables are written. To verify that these configs are effective we will need to take a peak into the actual blocks written by the reftable backend.

Introduce a new mode to the dumping logic that prints out the block structure. This logic can be invoked via test-tool dump-reftables -b.

---
 reftable/dump.c                   |   8 ++-
 reftable/reader.c                 |  63 ++++++++++++++++++
 reftable/reftable-reader.h        |   2 +
 t/t0613-reftable-write-options.sh | 102 ++++++++++++++++++++++++++++++
 4 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100755 t/t0613-reftable-write-options.sh

index 9c770a10cc..24476cc2a9 100644
@@ -48,6 +48,7 @@ static void print_help(void)
 	printf("usage: dump [-cst] arg\n\n"
 	       "options: \n"
 	       "  -c compact\n"
+	       "  -b dump blocks\n"
 	       "  -t dump table\n"
 	       "  -s dump stack\n"
 	       "  -6 sha256 hash format\n"

@@ -58,6 +59,7 @@ static void print_help(void)
 int reftable_dump_main(int argc, char *const *argv)
 {
 	int err = 0;
+	int opt_dump_blocks = 0;
 	int opt_dump_table = 0;
 	int opt_dump_stack = 0;
 	int opt_compact = 0;

@@ -67,6 +69,8 @@ int reftable_dump_main(int argc, char *const *argv)
 	for (; argc > 1; argv++, argc--)
 		if (*argv[1] != '-')
 			break;
+		else if (!strcmp("-b", argv[1]))
+			opt_dump_blocks = 1;
 		else if (!strcmp("-t", argv[1]))
 			opt_dump_table = 1;
 		else if (!strcmp("-6", argv[1]))

@@ -88,7 +92,9 @@ int reftable_dump_main(int argc, char *const *argv)
 
 	arg = argv[1];
 
-	if (opt_dump_table) {
+	if (opt_dump_blocks) {
+		err = reftable_reader_print_blocks(arg);
+	} else if (opt_dump_table) {
 		err = reftable_reader_print_file(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);

index 4a4bc2fdf8..4a04857773 100644
@@ -97,5 +97,7 @@ void reftable_table_from_reader(struct reftable_table *tab,
 
 /* print table onto stdout for debugging. */
 int reftable_reader_print_file(const char *tablename);
+/* print blocks onto stdout for debugging. */
+int reftable_reader_print_blocks(const char *tablename);
 
 #endif

2.45.0

Add a new option reftable.blockSize that allows the user to control the block size used by the reftable library.

---
 Documentation/config.txt          |  2 +
 Documentation/config/reftable.txt | 14 ++++++
 refs/reftable-backend.c           | 32 +++++++++++++-
 t/t0613-reftable-write-options.sh | 72 +++++++++++++++++++++++++++++++
 4 files changed, 119 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/reftable.txt

index 70b448b132..fa1469e5e7 100644
@@ -497,6 +497,8 @@ include::config/rebase.txt[]
 
 include::config/receive.txt[]
 
+include::config/reftable.txt[]
+
 include::config/remote.txt[]
 
 include::config/remotes.txt[]

new file mode 100644
index 0000000000..fa7c4be014
@@ -0,0 +1,14 @@
+reftable.blockSize::
+	The size in bytes used by the reftable backend when writing blocks.
+	The block size is determined by the writer, and does not have to be a
+	power of 2. The block size must be larger than the longest reference
+	name or log entry used in the repository, as references cannot span
+	blocks.
++
+Powers of two that are friendly to the virtual memory system or
+filesystem (such as 4kB or 8kB) are recommended. Larger sizes (64kB) can
+yield better compression, with a possible increased cost incurred by
+readers during access.
++
+The largest block size is `16777215` bytes (15.99 MiB). The default value is
+`4096` bytes (4kB). A value of `0` will use the default value.

index 1cda48c504..c2c47a3bc1 100644
@@ -1,6 +1,7 @@
 #include "../git-compat-util.h"
 #include "../abspath.h"
 #include "../chdir-notify.h"
+#include "../config.h"
 #include "../environment.h"
 #include "../gettext.h"
 #include "../hash.h"

@@ -230,6 +231,23 @@ static int read_ref_without_reload(struct reftable_stack *stack,
 	return ret;
 }
 
+static int reftable_be_config(const char *var, const char *value,
+			      const struct config_context *ctx,
+			      void *_opts)
+{
+	struct reftable_write_options *opts = _opts;
+
+	if (!strcmp(var, "reftable.blocksize")) {
+		unsigned long block_size = git_config_ulong(var, value, ctx->kvi);
+		if (block_size > 16777215)
+			die("reftable block size cannot exceed 16MB");
+		opts->block_size = block_size;
+		return 0;
+	}
+
+	return 0;
+}
+
 static struct ref_store *reftable_be_init(struct repository *repo,
 					  const char *gitdir,
 					  unsigned int store_flags)

@@ -245,12 +263,24 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	base_ref_store_init(&refs->base, repo, gitdir, &refs_be_reftable);
 	strmap_init(&refs->worktree_stacks);
 	refs->store_flags = store_flags;
-	refs->write_options.block_size = 4096;
+
 	refs->write_options.hash_id = repo->hash_algo->format_id;
 	refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);
 	refs->write_options.disable_auto_compact =
 		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
 
+	git_config(reftable_be_config, &refs->write_options);
+
+	/*
+	 * It is somewhat unfortunate that we have to mirror the default block
+	 * size of the reftable library here. But given that the write options
+	 * wouldn't be updated by the library here, and given that we require
+	 * the proper block size to trim reflog message so that they fit, we
+	 * must set up a proper value here.
+	 */
+	if (!refs->write_options.block_size)
+		refs->write_options.block_size = 4096;
+
 	/*
 	 * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.
 	 * This stack contains both the shared and the main worktree refs.

2.45.0

Patrick Steinhardt <[email protected]> writes:

[snip]

It's unnecessary indeed. I first wanted to defend this, but then I noticed that I'm also being inconsistent here where the last branch won't have return 0; at the end of this series.

Will remove.

I think it's easier to read that way.

Headers in the "reftable/" directory which do not have a "reftable-" prefix are considered to be private. So those shouldn't be used.

Patrick

The restart interval can at most be UINT16_MAX as specified in the technical documentation of the reftable format. Furthermore, it cannot ever be negative. Regardless of that we use an int to track the restart interval.

Change the type to use an uint16_t instead.

---
 reftable/block.h           | 2 +-
 reftable/reftable-writer.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index ea4384a7e2..cd5577105d 100644
@@ -25,7 +25,7 @@ struct block_writer {
 	uint32_t header_off;
 
 	/* How often to restart keys. */
-	int restart_interval;
+	uint16_t restart_interval;
 	int hash_size;
 
 	/* Offset of next uint8_t to write. */

index 44cb986465..4cd8ebe6c7 100644
@@ -28,7 +28,7 @@ struct reftable_write_options {
 	unsigned skip_index_objects : 1;
 
 	/* how often to write complete keys in each block. */
-	int restart_interval;
+	uint16_t restart_interval;
 
 	/* 4-byte identifier ("sha1", "s256") of the hash.
 	 * Defaults to SHA1 if unset

2.45.0

Add a new option reftable.restartInterval that allows the user to control the restart interval when writing reftable records used by the reftable library.

---
 Documentation/config/reftable.txt | 19 ++++++++++++++
 refs/reftable-backend.c           |  6 +++++
 t/t0613-reftable-write-options.sh | 43 +++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+)

index fa7c4be014..16b915c75e 100644
@@ -12,3 +12,22 @@ readers during access.
 +
 The largest block size is `16777215` bytes (15.99 MiB). The default value is
 `4096` bytes (4kB). A value of `0` will use the default value.
+
+reftable.restartInterval::
+	The interval at which to create restart points. The reftable backend
+	determines the restart points at file creation. The process is
+	arbitrary, but every 16 or 64 records is recommended. Every 16 may be
+	more suitable for smaller block sizes (4k or 8k), every 64 for larger
+	block sizes (64k).
++
+More frequent restart points reduces prefix compression and increases
+space consumed by the restart table, both of which increase file size.
++
+Less frequent restart points makes prefix compression more effective,
+decreasing overall file size, with increased penalties for readers
+walking through more records after the binary search step.
++
+A maximum of `65535` restart points per block is supported.
++
+The default value is to create restart points every 16 records. A value of `0`
+will use the default value.

index c2c47a3bc1..a786143de2 100644
@@ -243,6 +243,12 @@ static int reftable_be_config(const char *var, const char *value,
 			die("reftable block size cannot exceed 16MB");
 		opts->block_size = block_size;
 		return 0;
+	} else if (!strcmp(var, "reftable.restartinterval")) {
+		unsigned long restart_interval = git_config_ulong(var, value, ctx->kvi);
+		if (restart_interval > UINT16_MAX)
+			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
+		opts->restart_interval = restart_interval;
+		return 0;
 	}
 
 	return 0;

2.45.0

Besides the expected "ref" and "log" records, the reftable library also writes "obj" records. These are basically a reverse mapping of object IDs to their respective ref records so that it becomes efficient to figure out which references point to a specific object. The motivation for this data structure is the "uploadpack.allowTipSHA1InWant" config, which allows a client to fetch any object by its hash that has a ref pointing to it.

This reverse index is not used by Git at all though, and the expectation is that most hosters nowadays use "uploadpack.allowAnySHA1InWant". It may thus be preferable for many users to disable writing these optional object indices altogether to safe some precious disk space.

Add a new config "reftable.indexObjects" that allows the user to disable the object index altogether.

---
 Documentation/config/reftable.txt |  6 +++
 refs/reftable-backend.c           |  3 ++
 t/t0613-reftable-write-options.sh | 69 +++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)

index 16b915c75e..6e4466f3c5 100644
@@ -31,3 +31,9 @@ A maximum of `65535` restart points per block is supported.
 +
 The default value is to create restart points every 16 records. A value of `0`
 will use the default value.
+
+reftable.indexObjects::
+	Whether the reftable backend shall write object blocks. Object blocks
+	are a reverse mapping of object ID to the references pointing to them.
++
+The default value is `true`.

index a786143de2..5298fcef6e 100644
@@ -249,6 +249,9 @@ static int reftable_be_config(const char *var, const char *value,
 			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
 		opts->restart_interval = restart_interval;
 		return 0;
+	} else if (!strcmp(var, "reftable.indexobjects")) {
+		opts->skip_index_objects = !git_config_bool(var, value);
+		return 0;
 	}
 
 	return 0;

2.45.0

When auto-compacting, the reftable library packs references such that the sizes of the tables form a geometric sequence. The factor for this geometric sequence is hardcoded to 2 right now. We're about to expose this as a config option though, so let's expose the factor via write options.

---
 reftable/reftable-writer.h |  6 ++++++
 reftable/stack.c           | 13 +++++++++----
 reftable/stack.h           |  3 ++-
 reftable/stack_test.c      |  4 ++--
 4 files changed, 19 insertions(+), 7 deletions(-)

index 4cd8ebe6c7..155457b042 100644
@@ -49,6 +49,12 @@ struct reftable_write_options {
 
 	/* boolean: Prevent auto-compaction of tables. */
 	unsigned disable_auto_compact : 1;
+
+	/*
+	 * Geometric sequence factor used by auto-compaction to decide which
+	 * tables to compact. Defaults to 2 if unset.
+	 */
+	uint8_t auto_compaction_factor;
 };
 
 /* reftable_block_stats holds statistics for a single block type */

index 7b4fff7c9e..6b0f8e13e7 100644
@@ -1215,12 +1215,16 @@ static int segment_size(struct segment *s)
 	return s->end - s->start;
 }
 
-struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
+					  uint8_t factor)
 {
 	struct segment seg = { 0 };
 	uint64_t bytes;
 	size_t i;
 
+	if (!factor)
+		factor = 2;
+
 	/*
 	 * If there are no tables or only a single one then we don't have to
 	 * compact anything. The sequence is geometric by definition already.

@@ -1252,7 +1256,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
 	 * 	64, 32, 16, 8, 4, 3, 1
 	 */
 	for (i = n - 1; i > 0; i--) {
-		if (sizes[i - 1] < sizes[i] * 2) {
+		if (sizes[i - 1] < sizes[i] * factor) {
 			seg.end = i + 1;
 			bytes = sizes[i];
 			break;

@@ -1278,7 +1282,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
 		uint64_t curr = bytes;
 		bytes += sizes[i - 1];
 
-		if (sizes[i - 1] < curr * 2) {
+		if (sizes[i - 1] < curr * factor) {
 			seg.start = i - 1;
 			seg.bytes = bytes;
 		}

@@ -1304,7 +1308,8 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
 {
 	uint64_t *sizes = stack_table_sizes_for_compaction(st);
 	struct segment seg =
-		suggest_compaction_segment(sizes, st->merged->stack_len);
+		suggest_compaction_segment(sizes, st->merged->stack_len,
+					   st->opts.auto_compaction_factor);
 	reftable_free(sizes);
 	if (segment_size(&seg) > 0)
 		return stack_compact_range_stats(st, seg.start, seg.end - 1,

index 97d7ebc043..5b45cff4f7 100644
@@ -35,6 +35,7 @@ struct segment {
 	uint64_t bytes;
 };
 
-struct segment suggest_compaction_segment(uint64_t *sizes, size_t n);
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
+					  uint8_t factor);
 
 #endif

index 3316d55f19..f6c11ef18d 100644
@@ -767,7 +767,7 @@ static void test_suggest_compaction_segment(void)
 {
 	uint64_t sizes[] = { 512, 64, 17, 16, 9, 9, 9, 16, 2, 16 };
 	struct segment min =
-		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
+		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes), 2);
 	EXPECT(min.start == 1);
 	EXPECT(min.end == 10);
 }

@@ -776,7 +776,7 @@ static void test_suggest_compaction_segment_nothing(void)
 {
 	uint64_t sizes[] = { 64, 32, 16, 8, 4, 2 };
 	struct segment result =
-		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
+		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes), 2);
 	EXPECT(result.start == result.end);
 }
 

2.45.0

[snip]

Good idea, will do.

Patrick

Allow configuring the geometric factor used by the auto-compaction algorithm whenever a new table is appended to the stack of tables.

---
 Documentation/config/reftable.txt | 10 ++++++++++
 refs/reftable-backend.c           |  5 +++++
 2 files changed, 15 insertions(+)

index 6e4466f3c5..1c381dda04 100644
@@ -37,3 +37,13 @@ reftable.indexObjects::
 	are a reverse mapping of object ID to the references pointing to them.
 +
 The default value is `true`.
+
+reftable.geometricFactor::
+	Whenever the reftable backend appends a new table to the table it
+	performs auto compaction to ensure that there is only a handful of
+	tables. The backend does this by ensuring that tables form a geometric
+	sequence regarding the respective sizes of each table.
++
+By default, the geometric sequence uses a factor of 2, meaning that for any
+table, the next-biggest table must at least be twice as big. A maximum factor
+of 256 is supported.

index 5298fcef6e..657d227c12 100644
@@ -252,6 +252,11 @@ static int reftable_be_config(const char *var, const char *value,
 	} else if (!strcmp(var, "reftable.indexobjects")) {
 		opts->skip_index_objects = !git_config_bool(var, value);
 		return 0;
+	} else if (!strcmp(var, "reftable.geometricfactor")) {
+		unsigned long factor = git_config_ulong(var, value, ctx->kvi);
+		if (factor > UINT8_MAX)
+			die("reftable geometric factor cannot exceed %u", (unsigned)UINT8_MAX);
+		opts->auto_compaction_factor = factor;
 	}
 
 	return 0;

2.45.0

This should rather be "to the stack,". Good catch.

Patrick

Ugh. I actually intended to pull in ps/reftable-write-optim as a dependency because I know it causes conflicts. But I screwed this up as I thought that the topic was merged into "master" already, even though it has only hit "next".

I'll refrain from sending a new version immediately though and will wait for reviews first. Once those are in I will pull in the above topic.

Patrick

I saw [01/11] has changes to stack_check_addition() that disappeared by the other topic, and the resolution is to remove the function (as nobody calls it anyway). Also [02/11] has changes to refname_test.c that can be resolved by just removing the file.

If there are other semantic conflict resolution needed, such a rebasing is appreciated, but otherwise, there is no strong need to rebase.

The mention of the name of the other topic that has interactions was very much helpful.

Thanks.

Yup, sounds right to me.

No other semantic conflicts I'm aware of, no. Thanks!

Patrick

I have reviewed these patches and I have nothing to add. Thanks Patrick!

-Justin

Hello Patrick,

I'v gone through the commits and apart from a few small comments, I think it looks great already.

Thanks,
Karthik

Thanks for your review!

Patrick

Hi,

this is the second version of my patch series that exposes various options of the reftable writer via Git configuration.

Changes compared to v1:

- Drop unneeded return statements.

- Move default geometric factor into "constants.h".

- Fix a typo in a commit message.

Thanks!

Patrick

Patrick Steinhardt (11):
reftable: consistently refer to reftable_write_options as opts
reftable: consistently pass write opts as value reftable/writer: drop static variable used to initialize strbuf reftable/writer: improve error when passed an invalid block size reftable/dump: support dumping a table's block structure refs/reftable: allow configuring block size
reftable: use uint16_t to track restart interval refs/reftable: allow configuring restart interval refs/reftable: allow disabling writing the object index
reftable: make the compaction factor configurable refs/reftable: allow configuring geometric factor

 Documentation/config.txt          |   2 +
 Documentation/config/reftable.txt |  49 +++++
 refs/reftable-backend.c           |  43 ++++-
 reftable/block.h                  |   2 +-
 reftable/constants.h              |   1 +
 reftable/dump.c                   |  12 +-
 reftable/merged_test.c            |   6 +-
 reftable/reader.c                 |  63 +++++++
 reftable/readwrite_test.c         |  26 +--
 reftable/refname_test.c           |   2 +-
 reftable/reftable-reader.h        |   2 +
 reftable/reftable-stack.h         |   2 +-
 reftable/reftable-writer.h        |  10 +-
 reftable/stack.c                  |  57 +++---
 reftable/stack.h                  |   5 +-
 reftable/stack_test.c             | 118 ++++++------
 reftable/writer.c                 |  20 +--
 t/t0613-reftable-write-options.sh | 286 ++++++++++++++++++++++++++++++
 18 files changed, 576 insertions(+), 130 deletions(-)
 create mode 100644 Documentation/config/reftable.txt
 create mode 100755 t/t0613-reftable-write-options.sh

Range-diff against v1:

 1:  47cee6e25e =  1:  7efa566306 reftable: consistently refer to `reftable_write_options` as `opts`
 2:  d8a0764e87 =  2:  e6f8fc09c2 reftable: consistently pass write opts as value
 3:  c040f81fba =  3:  aa2903e3e5 reftable/writer: drop static variable used to initialize strbuf
 4:  ef79bb1b7b =  4:  5e7cbb7b19 reftable/writer: improve error when passed an invalid block size
 5:  4d4407d4a4 =  5:  ed1c150d90 reftable/dump: support dumping a table's block structure
 6:  b4e4db5735 !  6:  be5bdc6dc1 refs/reftable: allow configuring block size
    @@ refs/reftable-backend.c: static int read_ref_without_reload(struct reftable_stac
     +		if (block_size > 16777215)
     +			die("reftable block size cannot exceed 16MB");
     +		opts->block_size = block_size;
    -+		return 0;
     +	}
     +
     +	return 0;
 7:  79d9e07ca9 =  7:  05e8d1df2d reftable: use `uint16_t` to track restart interval
 8:  653ec4dfa5 !  8:  bc0bf65553 refs/reftable: allow configuring restart interval
    @@ Documentation/config/reftable.txt: readers during access.
     
      ## refs/reftable-backend.c ##
     @@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const char *value,
    + 		if (block_size > 16777215)
      			die("reftable block size cannot exceed 16MB");
      		opts->block_size = block_size;
    - 		return 0;
     +	} else if (!strcmp(var, "reftable.restartinterval")) {
     +		unsigned long restart_interval = git_config_ulong(var, value, ctx->kvi);
     +		if (restart_interval > UINT16_MAX)
     +			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
     +		opts->restart_interval = restart_interval;
    -+		return 0;
      	}
      
      	return 0;
 9:  6f2c481acc !  9:  6bc240fd0c refs/reftable: allow disabling writing the object index
    @@ Documentation/config/reftable.txt: A maximum of `65535` restart points per block
     
      ## refs/reftable-backend.c ##
     @@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const char *value,
    + 		if (restart_interval > UINT16_MAX)
      			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
      		opts->restart_interval = restart_interval;
    - 		return 0;
     +	} else if (!strcmp(var, "reftable.indexobjects")) {
     +		opts->skip_index_objects = !git_config_bool(var, value);
    -+		return 0;
      	}
      
      	return 0;

10: 30e2e33479 ! 10: 9d4c1f0340 reftable: make the compaction factor configurable
@@ Commit message

+ ## reftable/constants.h ##
+@@ reftable/constants.h: license that can be found in the LICENSE file or at +
+ #define MAX_RESTARTS ((1 << 16) - 1)
+ #define DEFAULT_BLOCK_SIZE 4096
++#define DEFAULT_GEOMETRIC_FACTOR 2
+
+ #endif
+
## reftable/reftable-writer.h ##
@@ reftable/reftable-writer.h: struct reftable_write_options {

@@ reftable/reftable-writer.h: struct reftable_write_options {
/* reftable_block_stats holds statistics for a single block type */

## reftable/stack.c ##
+@@ reftable/stack.c: license that can be found in the LICENSE file or at +
+ #include "../write-or-die.h"
+ #include "system.h"
++#include "constants.h"
+ #include "merged.h"
+ #include "reader.h"
+ #include "refname.h"
@@ reftable/stack.c: static int segment_size(struct segment *s)
return s->end - s->start;
}
@@ reftable/stack.c: static int segment_size(struct segment *s)
size_t i;

+ if (!factor)
-+ factor = 2;
++ factor = DEFAULT_GEOMETRIC_FACTOR;
+
/*
* If there are no tables or only a single one then we don't have to
11: 861f2e72d9 ! 11: e1282e53fb refs/reftable: allow configuring geometric factor
@@ Documentation/config/reftable.txt: reftable.indexObjects::
The default value is true.
+
+reftable.geometricFactor::
-+ Whenever the reftable backend appends a new table to the table it
++ Whenever the reftable backend appends a new table to the stack, it
+ performs auto compaction to ensure that there is only a handful of
+ tables. The backend does this by ensuring that tables form a geometric
+ sequence regarding the respective sizes of each table.
@@ Documentation/config/reftable.txt: reftable.indexObjects::

## refs/reftable-backend.c ##
@@ refs/reftable-backend.c: static int reftable_be_config(const char *var, const char *value,
+ opts->restart_interval = restart_interval;
} else if (!strcmp(var, "reftable.indexobjects")) {
opts->skip_index_objects = !git_config_bool(var, value);
- return 0;
+ } else if (!strcmp(var, "reftable.geometricfactor")) {
+ unsigned long factor = git_config_ulong(var, value, ctx->kvi);
+ if (factor > UINT8_MAX)

base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377

2.45.0

Throughout the reftable library the reftable_write_options are sometimes referred to as cfg and sometimes as opts. Unify these to consistently use opts to avoid confusion.

While at it, touch up the coding style a bit by removing unneeded braces around one-line statements and newlines between variable declarations.

---
 reftable/dump.c           |   4 +-
 reftable/reftable-stack.h |   2 +-
 reftable/stack.c          |  43 +++++++-------
 reftable/stack.h          |   2 +-
 reftable/stack_test.c     | 114 +++++++++++++++++---------------------
 5 files changed, 75 insertions(+), 90 deletions(-)

index 26e0393c7d..9c770a10cc 100644
@@ -27,9 +27,9 @@ license that can be found in the LICENSE file or at
 static int compact_stack(const char *stackdir)
 {
 	struct reftable_stack *stack = NULL;
-	struct reftable_write_options cfg = { 0 };
+	struct reftable_write_options opts = { 0 };
 
-	int err = reftable_new_stack(&stack, stackdir, cfg);
+	int err = reftable_new_stack(&stack, stackdir, opts);
 	if (err < 0)
 		goto done;
 

index 1b602dda58..9c8e4eef49 100644
@@ -29,7 +29,7 @@ struct reftable_stack;
  *  stored in 'dir'. Typically, this should be .git/reftables.
  */
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       struct reftable_write_options config);
+		       struct reftable_write_options opts);
 
 /* returns the update_index at which a next table should be written. */
 uint64_t reftable_stack_next_update_index(struct reftable_stack *st);

index d43efa4760..97d7ebc043 100644
@@ -20,7 +20,7 @@ struct reftable_stack {
 
 	char *reftable_dir;
 
-	struct reftable_write_options config;
+	struct reftable_write_options opts;
 
 	struct reftable_reader **readers;
 	size_t readers_len;

2.45.0

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.

---
 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(-)

index 530fc82d1c..4ac81de8d4 100644
@@ -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",

index a6dbd214c5..27631a041b 100644
@@ -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 = {

index b9cc62554e..3468253be7 100644
@@ -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,

index 155bf0bbe2..44cb986465 100644
@@ -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

index 3979657793..7b4fff7c9e 100644
@@ -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;

index 1d9ff0fbfa..ad2f2e6c65 100644
@@ -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);
 

2.45.0

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

We have a static variable in the reftable writer code that is merely used to initialize the last_key of the writer. Convert the code to instead use strbuf_init() and drop the variable.

---
 reftable/writer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index ad2f2e6c65..7df6e53699 100644
@@ -117,15 +117,12 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
 	w->block_writer->restart_interval = w->opts.restart_interval;
 }
 
-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)
 {
 	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)) {

@@ -133,7 +130,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 		abort();
 	}
 
-	wp->last_key = reftable_empty_strbuf;
+	strbuf_init(&wp->block_writer_data.last_key, 0);
+	strbuf_init(&wp->last_key, 0);
 	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
 	wp->write = writer_func;
 	wp->write_arg = writer_arg;

2.45.0

Nice. There is no guarantee that a structure assignment of a just-initialized empty strbuf to another will stay to be a trouble free operation, and using an explicit _init() call is the right thing to do.

The reftable format only supports block sizes up to 16MB. When the writer is being passed a value bigger than that it simply calls abort(3P), which isn't all that helpful due to the lack of a proper error message.

Improve this by calling BUG() instead.

---
 reftable/writer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index 7df6e53699..374b7d15ed 100644
@@ -125,10 +125,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
 
 	options_set_defaults(&opts);
-	if (opts.block_size >= (1 << 24)) {
-		/* TODO - error return? */
-		abort();
-	}
+	if (opts.block_size >= (1 << 24))
+		BUG("configured block size exceeds 16MB");
 
 	strbuf_init(&wp->block_writer_data.last_key, 0);
 	strbuf_init(&wp->last_key, 0);

2.45.0

As a "git" person, I do not mind this at all.

But doesn't the reftable/ library codebase want to avoid things like BUG() that are very much tied to our codebase, for the same reason as it avoids things like xmalloc(), xcalloc(), and ALLOC_GROW()?

We may have crossed the bridge long time ago, though. We see a handful calls to BUG() already inside reftable/ directory.

Exactly. No matter what, once there will be a second user of the reftable library we will have to figure out a maintainable way to ensure that the library can be used by other projects, too. And that will require some larger refactorings anyway.

I think initially, the intent was to have a "system.h" header that contains a bunch of wrappers that bridge the gap between reftables and Git. I feel like this abstraction does not make any sense though in its current form as it is simply being included by the reftable code, which then uses the Git functions directly.

I think eventually, we will have to adapt this such that the Git includes do not leak into the reftable code at all. Instead, we should have a shim "system.c" that carries the Git-specific includes and then implements a couple of wrapper functions. "system.h" would then only be carrying function declarations of those wrappers.

That's a larger topic though, and I think that tackling this now would be premature without any potential users of the reftable library.

Patrick

We're about to introduce new configs that will allow users to have more control over how exactly reftables are written. To verify that these configs are effective we will need to take a peak into the actual blocks written by the reftable backend.

Introduce a new mode to the dumping logic that prints out the block structure. This logic can be invoked via test-tool dump-reftables -b.

---
 reftable/dump.c                   |   8 ++-
 reftable/reader.c                 |  63 ++++++++++++++++++
 reftable/reftable-reader.h        |   2 +
 t/t0613-reftable-write-options.sh | 102 ++++++++++++++++++++++++++++++
 4 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100755 t/t0613-reftable-write-options.sh

index 9c770a10cc..24476cc2a9 100644
@@ -48,6 +48,7 @@ static void print_help(void)
 	printf("usage: dump [-cst] arg\n\n"
 	       "options: \n"
 	       "  -c compact\n"
+	       "  -b dump blocks\n"
 	       "  -t dump table\n"
 	       "  -s dump stack\n"
 	       "  -6 sha256 hash format\n"

@@ -58,6 +59,7 @@ static void print_help(void)
 int reftable_dump_main(int argc, char *const *argv)
 {
 	int err = 0;
+	int opt_dump_blocks = 0;
 	int opt_dump_table = 0;
 	int opt_dump_stack = 0;
 	int opt_compact = 0;

@@ -67,6 +69,8 @@ int reftable_dump_main(int argc, char *const *argv)
 	for (; argc > 1; argv++, argc--)
 		if (*argv[1] != '-')
 			break;
+		else if (!strcmp("-b", argv[1]))
+			opt_dump_blocks = 1;
 		else if (!strcmp("-t", argv[1]))
 			opt_dump_table = 1;
 		else if (!strcmp("-6", argv[1]))

@@ -88,7 +92,9 @@ int reftable_dump_main(int argc, char *const *argv)
 
 	arg = argv[1];
 
-	if (opt_dump_table) {
+	if (opt_dump_blocks) {
+		err = reftable_reader_print_blocks(arg);
+	} else if (opt_dump_table) {
 		err = reftable_reader_print_file(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);

index 4a4bc2fdf8..4a04857773 100644
@@ -97,5 +97,7 @@ void reftable_table_from_reader(struct reftable_table *tab,
 
 /* print table onto stdout for debugging. */
 int reftable_reader_print_file(const char *tablename);
+/* print blocks onto stdout for debugging. */
+int reftable_reader_print_blocks(const char *tablename);
 
 #endif

2.45.0

This step somehow looks familiar. Perhaps that is because I read it more carefully than other steps during the last round.

Looking good to me.

Add a new option reftable.blockSize that allows the user to control the block size used by the reftable library.

---
 Documentation/config.txt          |  2 +
 Documentation/config/reftable.txt | 14 ++++++
 refs/reftable-backend.c           | 31 ++++++++++++-
 t/t0613-reftable-write-options.sh | 72 +++++++++++++++++++++++++++++++
 4 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/config/reftable.txt

index 70b448b132..fa1469e5e7 100644
@@ -497,6 +497,8 @@ include::config/rebase.txt[]
 
 include::config/receive.txt[]
 
+include::config/reftable.txt[]
+
 include::config/remote.txt[]
 
 include::config/remotes.txt[]

new file mode 100644
index 0000000000..fa7c4be014
@@ -0,0 +1,14 @@
+reftable.blockSize::
+	The size in bytes used by the reftable backend when writing blocks.
+	The block size is determined by the writer, and does not have to be a
+	power of 2. The block size must be larger than the longest reference
+	name or log entry used in the repository, as references cannot span
+	blocks.
++
+Powers of two that are friendly to the virtual memory system or
+filesystem (such as 4kB or 8kB) are recommended. Larger sizes (64kB) can
+yield better compression, with a possible increased cost incurred by
+readers during access.
++
+The largest block size is `16777215` bytes (15.99 MiB). The default value is
+`4096` bytes (4kB). A value of `0` will use the default value.

index 1cda48c504..bd9999cefc 100644
@@ -1,6 +1,7 @@
 #include "../git-compat-util.h"
 #include "../abspath.h"
 #include "../chdir-notify.h"
+#include "../config.h"
 #include "../environment.h"
 #include "../gettext.h"
 #include "../hash.h"

@@ -230,6 +231,22 @@ static int read_ref_without_reload(struct reftable_stack *stack,
 	return ret;
 }
 
+static int reftable_be_config(const char *var, const char *value,
+			      const struct config_context *ctx,
+			      void *_opts)
+{
+	struct reftable_write_options *opts = _opts;
+
+	if (!strcmp(var, "reftable.blocksize")) {
+		unsigned long block_size = git_config_ulong(var, value, ctx->kvi);
+		if (block_size > 16777215)
+			die("reftable block size cannot exceed 16MB");
+		opts->block_size = block_size;
+	}
+
+	return 0;
+}
+
 static struct ref_store *reftable_be_init(struct repository *repo,
 					  const char *gitdir,
 					  unsigned int store_flags)

@@ -245,12 +262,24 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	base_ref_store_init(&refs->base, repo, gitdir, &refs_be_reftable);
 	strmap_init(&refs->worktree_stacks);
 	refs->store_flags = store_flags;
-	refs->write_options.block_size = 4096;
+
 	refs->write_options.hash_id = repo->hash_algo->format_id;
 	refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);
 	refs->write_options.disable_auto_compact =
 		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
 
+	git_config(reftable_be_config, &refs->write_options);
+
+	/*
+	 * It is somewhat unfortunate that we have to mirror the default block
+	 * size of the reftable library here. But given that the write options
+	 * wouldn't be updated by the library here, and given that we require
+	 * the proper block size to trim reflog message so that they fit, we
+	 * must set up a proper value here.
+	 */
+	if (!refs->write_options.block_size)
+		refs->write_options.block_size = 4096;
+
 	/*
 	 * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.
 	 * This stack contains both the shared and the main worktree refs.

2.45.0

The restart interval can at most be UINT16_MAX as specified in the technical documentation of the reftable format. Furthermore, it cannot ever be negative. Regardless of that we use an int to track the restart interval.

Change the type to use an uint16_t instead.

---
 reftable/block.h           | 2 +-
 reftable/reftable-writer.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index ea4384a7e2..cd5577105d 100644
@@ -25,7 +25,7 @@ struct block_writer {
 	uint32_t header_off;
 
 	/* How often to restart keys. */
-	int restart_interval;
+	uint16_t restart_interval;
 	int hash_size;
 
 	/* Offset of next uint8_t to write. */

index 44cb986465..4cd8ebe6c7 100644
@@ -28,7 +28,7 @@ struct reftable_write_options {
 	unsigned skip_index_objects : 1;
 
 	/* how often to write complete keys in each block. */
-	int restart_interval;
+	uint16_t restart_interval;
 
 	/* 4-byte identifier ("sha1", "s256") of the hash.
 	 * Defaults to SHA1 if unset

2.45.0

Not wrong per-se, but this one is more or less a Meh, as we know we do not work on 16-bit platforms anyway.

Yeah, my intent isn't really "platform portability". It's rather that I think that an uint16_t documents the accepted range of values much better than int does.

Patrick

Add a new option reftable.restartInterval that allows the user to control the restart interval when writing reftable records used by the reftable library.

---
 Documentation/config/reftable.txt | 19 ++++++++++++++
 refs/reftable-backend.c           |  5 ++++
 t/t0613-reftable-write-options.sh | 43 +++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

index fa7c4be014..16b915c75e 100644
@@ -12,3 +12,22 @@ readers during access.
 +
 The largest block size is `16777215` bytes (15.99 MiB). The default value is
 `4096` bytes (4kB). A value of `0` will use the default value.
+
+reftable.restartInterval::
+	The interval at which to create restart points. The reftable backend
+	determines the restart points at file creation. The process is
+	arbitrary, but every 16 or 64 records is recommended. Every 16 may be
+	more suitable for smaller block sizes (4k or 8k), every 64 for larger
+	block sizes (64k).
++
+More frequent restart points reduces prefix compression and increases
+space consumed by the restart table, both of which increase file size.
++
+Less frequent restart points makes prefix compression more effective,
+decreasing overall file size, with increased penalties for readers
+walking through more records after the binary search step.
++
+A maximum of `65535` restart points per block is supported.
++
+The default value is to create restart points every 16 records. A value of `0`
+will use the default value.

index bd9999cefc..9972dfc1a3 100644
@@ -242,6 +242,11 @@ static int reftable_be_config(const char *var, const char *value,
 		if (block_size > 16777215)
 			die("reftable block size cannot exceed 16MB");
 		opts->block_size = block_size;
+	} else if (!strcmp(var, "reftable.restartinterval")) {
+		unsigned long restart_interval = git_config_ulong(var, value, ctx->kvi);
+		if (restart_interval > UINT16_MAX)
+			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
+		opts->restart_interval = restart_interval;
 	}
 
 	return 0;

2.45.0

The latter is what I wanted to say, but I agree that it's hard to parse. And honestly, I don't even know how arbitrary it is, so I should probably not claim something like this in the first place.

Will do.

Patrick

Besides the expected "ref" and "log" records, the reftable library also writes "obj" records. These are basically a reverse mapping of object IDs to their respective ref records so that it becomes efficient to figure out which references point to a specific object. The motivation for this data structure is the "uploadpack.allowTipSHA1InWant" config, which allows a client to fetch any object by its hash that has a ref pointing to it.

This reverse index is not used by Git at all though, and the expectation is that most hosters nowadays use "uploadpack.allowAnySHA1InWant". It may thus be preferable for many users to disable writing these optional object indices altogether to safe some precious disk space.

Add a new config "reftable.indexObjects" that allows the user to disable the object index altogether.

---
 Documentation/config/reftable.txt |  6 +++
 refs/reftable-backend.c           |  2 +
 t/t0613-reftable-write-options.sh | 69 +++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

index 16b915c75e..6e4466f3c5 100644
@@ -31,3 +31,9 @@ A maximum of `65535` restart points per block is supported.
 +
 The default value is to create restart points every 16 records. A value of `0`
 will use the default value.
+
+reftable.indexObjects::
+	Whether the reftable backend shall write object blocks. Object blocks
+	are a reverse mapping of object ID to the references pointing to them.
++
+The default value is `true`.

index 9972dfc1a3..63b75f770d 100644
@@ -247,6 +247,8 @@ static int reftable_be_config(const char *var, const char *value,
 		if (restart_interval > UINT16_MAX)
 			die("reftable block size cannot exceed %u", (unsigned)UINT16_MAX);
 		opts->restart_interval = restart_interval;
+	} else if (!strcmp(var, "reftable.indexobjects")) {
+		opts->skip_index_objects = !git_config_bool(var, value);
 	}
 
 	return 0;

2.45.0

When auto-compacting, the reftable library packs references such that the sizes of the tables form a geometric sequence. The factor for this geometric sequence is hardcoded to 2 right now. We're about to expose this as a config option though, so let's expose the factor via write options.

---
 reftable/constants.h       |  1 +
 reftable/reftable-writer.h |  6 ++++++
 reftable/stack.c           | 14 ++++++++++----
 reftable/stack.h           |  3 ++-
 reftable/stack_test.c      |  4 ++--
 5 files changed, 21 insertions(+), 7 deletions(-)

index 5eee72c4c1..f6beb843eb 100644
@@ -17,5 +17,6 @@ license that can be found in the LICENSE file or at
 
 #define MAX_RESTARTS ((1 << 16) - 1)
 #define DEFAULT_BLOCK_SIZE 4096
+#define DEFAULT_GEOMETRIC_FACTOR 2
 
 #endif

index 4cd8ebe6c7..155457b042 100644
@@ -49,6 +49,12 @@ struct reftable_write_options {
 
 	/* boolean: Prevent auto-compaction of tables. */
 	unsigned disable_auto_compact : 1;
+
+	/*
+	 * Geometric sequence factor used by auto-compaction to decide which
+	 * tables to compact. Defaults to 2 if unset.
+	 */
+	uint8_t auto_compaction_factor;
 };
 
 /* reftable_block_stats holds statistics for a single block type */

index 7b4fff7c9e..762954b181 100644
@@ -10,6 +10,7 @@ license that can be found in the LICENSE file or at
 
 #include "../write-or-die.h"
 #include "system.h"
+#include "constants.h"
 #include "merged.h"
 #include "reader.h"
 #include "refname.h"

@@ -1215,12 +1216,16 @@ static int segment_size(struct segment *s)
 	return s->end - s->start;
 }
 
-struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
+					  uint8_t factor)
 {
 	struct segment seg = { 0 };
 	uint64_t bytes;
 	size_t i;
 
+	if (!factor)
+		factor = DEFAULT_GEOMETRIC_FACTOR;
+
 	/*
 	 * If there are no tables or only a single one then we don't have to
 	 * compact anything. The sequence is geometric by definition already.

@@ -1252,7 +1257,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
 	 * 	64, 32, 16, 8, 4, 3, 1
 	 */
 	for (i = n - 1; i > 0; i--) {
-		if (sizes[i - 1] < sizes[i] * 2) {
+		if (sizes[i - 1] < sizes[i] * factor) {
 			seg.end = i + 1;
 			bytes = sizes[i];
 			break;

@@ -1278,7 +1283,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
 		uint64_t curr = bytes;
 		bytes += sizes[i - 1];
 
-		if (sizes[i - 1] < curr * 2) {
+		if (sizes[i - 1] < curr * factor) {
 			seg.start = i - 1;
 			seg.bytes = bytes;
 		}

@@ -1304,7 +1309,8 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
 {
 	uint64_t *sizes = stack_table_sizes_for_compaction(st);
 	struct segment seg =
-		suggest_compaction_segment(sizes, st->merged->stack_len);
+		suggest_compaction_segment(sizes, st->merged->stack_len,
+					   st->opts.auto_compaction_factor);
 	reftable_free(sizes);
 	if (segment_size(&seg) > 0)
 		return stack_compact_range_stats(st, seg.start, seg.end - 1,

index 97d7ebc043..5b45cff4f7 100644
@@ -35,6 +35,7 @@ struct segment {
 	uint64_t bytes;
 };
 
-struct segment suggest_compaction_segment(uint64_t *sizes, size_t n);
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
+					  uint8_t factor);
 
 #endif

index 3316d55f19..f6c11ef18d 100644
@@ -767,7 +767,7 @@ static void test_suggest_compaction_segment(void)
 {
 	uint64_t sizes[] = { 512, 64, 17, 16, 9, 9, 9, 16, 2, 16 };
 	struct segment min =
-		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
+		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes), 2);
 	EXPECT(min.start == 1);
 	EXPECT(min.end == 10);
 }

@@ -776,7 +776,7 @@ static void test_suggest_compaction_segment_nothing(void)
 {
 	uint64_t sizes[] = { 64, 32, 16, 8, 4, 2 };
 	struct segment result =
-		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
+		suggest_compaction_segment(sizes, ARRAY_SIZE(sizes), 2);
 	EXPECT(result.start == result.end);
 }
 

2.45.0

Hmph. It is unclear if having this as uint8_t gives us a useful enhancement, but perhaps in the future hosters may find a more aggressive geometric sequence is better for their workload or something and raise it to 3 or 4? I was actually wondering if a base smaller than 2 (e.g. fibonacci) may work better.

Anyway, making it configurable is a good first step. Allowing a bit finer grained setting than just integral values can be done later if it proves necessary.

That's a fair point indeed. I had similar issues with git-repack(1)'s --geometric= option, where you can also only pick integers. There's also a similar discussion in the patch series by Taylor [1], where I proposed to maybe introduce floats into git-config(1).

So this may be good enough for now, and when we gain the ability to parse floats we may convert this to accept floats, as well. An alternative would be to convert this to percent, where the default value would be 200. That should give sufficient flexibility without having to introduce floats.

Patrick

[1]: https://lore.kernel.org/git/[email protected]/

There already is an established way to specify float with arbitrary precision on the command line if we limit the value to 0..1, by the way.

https://lore.kernel.org/git/[email protected]/

It is amusing to revisit an ancient discussion thread. I can see, on the same thread in the discussion following that message, that back in May 2005, we were discussing "intent to add" that did not get invented until mid 2008 already ;-).

The problem is that we don't want to limit the value to 0..1, but to 1..n. 1 really is the lowest semi-sensible number you can pick and means that all tables should have the same size. And in practice, there is no upper limit, even though it's probably not all that reasonable to pick anything beyond 10.

In any case, I'd propose to keep this as-is for now. The simple reason is that we have preexisting commands (git repack -g) and in-flight series (pseudo-merge bitmaps) that also use plain integers to represent the geometric factor. So I'm aiming for consistency. From thereon we can then iterate and design a solution that works for all of them, e.g. by allowing for floats outside of the 0..1 range in whatever form.

:)

Patrick

Allow configuring the geometric factor used by the auto-compaction algorithm whenever a new table is appended to the stack of tables.

---
 Documentation/config/reftable.txt | 10 ++++++++++
 refs/reftable-backend.c           |  5 +++++
 2 files changed, 15 insertions(+)

index 6e4466f3c5..f928d1029b 100644
@@ -37,3 +37,13 @@ reftable.indexObjects::
 	are a reverse mapping of object ID to the references pointing to them.
 +
 The default value is `true`.
+
+reftable.geometricFactor::
+	Whenever the reftable backend appends a new table to the stack, it
+	performs auto compaction to ensure that there is only a handful of
+	tables. The backend does this by ensuring that tables form a geometric
+	sequence regarding the respective sizes of each table.
++
+By default, the geometric sequence uses a factor of 2, meaning that for any
+table, the next-biggest table must at least be twice as big. A maximum factor
+of 256 is supported.

index 63b75f770d..c4bd0fe1f9 100644
@@ -249,6 +249,11 @@ static int reftable_be_config(const char *var, const char *value,
 		opts->restart_interval = restart_interval;
 	} else if (!strcmp(var, "reftable.indexobjects")) {
 		opts->skip_index_objects = !git_config_bool(var, value);
+	} else if (!strcmp(var, "reftable.geometricfactor")) {
+		unsigned long factor = git_config_ulong(var, value, ctx->kvi);
+		if (factor > UINT8_MAX)
+			die("reftable geometric factor cannot exceed %u", (unsigned)UINT8_MAX);
+		opts->auto_compaction_factor = factor;
 	}
 
 	return 0;

2.45.0

The range diff looks good to me, thanks for the quick iteration :)

Hi,

this is the third version of my patch series that exposes several write options of the reftable library via Git configs.

Changes compared to v2:

- Adapted patch 2 such that we now pass options as const pointers instead of by value.

- Removed a confusing sentence in the documentation of the restart points in patch 8.

Other than that I decided to rebase this on top of the current "master" branch at 0f3415f1f8 (The second batch, 2024-05-08). This is because the revamped patch 2 would cause new conflicts with 485c63cf5c (reftable:
remove name checks, 2024-04-08) that didn't exist in v2 of this patch series yet. Rebasing thus seemed like the more reasonable option.

Patrick

Patrick Steinhardt (11):
reftable: consistently refer to reftable_write_options as opts
reftable: pass opts as constant pointer reftable/writer: drop static variable used to initialize strbuf reftable/writer: improve error when passed an invalid block size reftable/dump: support dumping a table's block structure refs/reftable: allow configuring block size
reftable: use uint16_t to track restart interval refs/reftable: allow configuring restart interval refs/reftable: allow disabling writing the object index
reftable: make the compaction factor configurable refs/reftable: allow configuring geometric factor

 Documentation/config.txt          |   2 +
 Documentation/config/reftable.txt |  48 +++++
 refs/reftable-backend.c           |  49 ++++-
 reftable/block.h                  |   2 +-
 reftable/constants.h              |   1 +
 reftable/dump.c                   |  12 +-
 reftable/reader.c                 |  63 +++++++
 reftable/reftable-reader.h        |   2 +
 reftable/reftable-stack.h         |   2 +-
 reftable/reftable-writer.h        |  10 +-
 reftable/stack.c                  |  58 +++---
 reftable/stack.h                  |   5 +-
 reftable/stack_test.c             | 118 ++++++------
 reftable/writer.c                 |  23 +--
 t/t0613-reftable-write-options.sh | 286 ++++++++++++++++++++++++++++++
 15 files changed, 566 insertions(+), 115 deletions(-)
 create mode 100644 Documentation/config/reftable.txt
 create mode 100755 t/t0613-reftable-write-options.sh

Range-diff against v2:
1: 7efa566306 ! 1: 71f4e31cf7 reftable: consistently refer to reftable_write_options as opts
@@ reftable/stack.c: static uint64_t *stack_table_sizes_for_compaction(struct refta
int overhead = header_size(version) - 1;
int i = 0;
for (i = 0; i < st->merged->stack_len; i++) {
-@@ reftable/stack.c: static int stack_check_addition(struct reftable_stack *st,
- int len = 0;
- int i = 0;
-
-- if (st->config.skip_name_check)
-+ if (st->opts.skip_name_check)
- return 0;
-
- err = reftable_block_source_from_file(&src, new_tab_name);
@@ reftable/stack.c: int reftable_stack_clean(struct reftable_stack *st)
int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
{
2: e6f8fc09c2 ! 2: f1c9914a77 reftable: consistently pass write opts as value
@@ Metadata
Author: Patrick Steinhardt <[email protected]>

## Commit message ##
- reftable: consistently pass write opts as value
+ reftable: pass opts as constant pointer

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
@@ Commit message
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.
+ Adapt the code so that we do not modify the caller-provided values
+ anymore. While at it, refactor the code to code to consistently pass the
+ options as a constant pointer to clarify that the caller-provided opts
+ will not ever get modified.

- ## reftable/merged_test.c ##
-@@ reftable/merged_test.c: static void write_test_table(struct strbuf *buf,
+ ## refs/reftable-backend.c ##
+@@ refs/reftable-backend.c: static struct reftable_stack *stack_for(struct reftable_ref_store *store,
+ store->base.repo->commondir, wtname_buf.buf);
+
+ store->err = reftable_new_stack(&stack, wt_dir.buf,
+- store->write_options);
++ &store->write_options);
+ assert(store->err != REFTABLE_API_ERROR);
+ strmap_put(&store->worktree_stacks, wtname_buf.buf, stack);
}
+@@ refs/reftable-backend.c: static struct ref_store *reftable_be_init(struct repository *repo,
}
+ strbuf_addstr(&path, "/reftable");
+ refs->err = reftable_new_stack(&refs->main_stack, path.buf,
+- refs->write_options);
++ &refs->write_options);
+ if (refs->err)
+ goto done;

-- 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++) {
-@@ reftable/merged_test.c: 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++) {
-@@ reftable/merged_test.c: 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);
+@@ refs/reftable-backend.c: static struct ref_store *reftable_be_init(struct repository *repo,
+ strbuf_addf(&path, "%s/reftable", gitdir);

- struct reftable_ref_record rec = {
- .refname = "master",
+ refs->err = reftable_new_stack(&refs->worktree_stack, path.buf,
+- refs->write_options);
++ &refs->write_options);
+ if (refs->err)
+ goto done;
+ }

- ## reftable/readwrite_test.c ##
-@@ reftable/readwrite_test.c: 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 };
-@@ reftable/readwrite_test.c: 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.
-@@ reftable/readwrite_test.c: 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);
-@@ reftable/readwrite_test.c: 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++) {
-@@ reftable/readwrite_test.c: 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;
-@@ reftable/readwrite_test.c: 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;
-@@ reftable/readwrite_test.c: static void test_write_empty_table(void)
+ ## reftable/dump.c ##
+@@ reftable/dump.c: static int compact_stack(const char *stackdir)
+ struct reftable_stack *stack = NULL;
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 };
-@@ reftable/readwrite_test.c: 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,
-@@ reftable/readwrite_test.c: 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,
-@@ reftable/readwrite_test.c: 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,
-@@ reftable/readwrite_test.c: 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",
-@@ reftable/readwrite_test.c: 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 = {
-@@ reftable/readwrite_test.c: static void test_write_multi_level_index(void)
- struct reftable_reader *reader;
- int err;
+- int err = reftable_new_stack(&stack, stackdir, opts);
++ int err = reftable_new_stack(&stack, stackdir, &opts);
+ if (err < 0)
+ goto done;

-- 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 = {

- ## reftable/refname_test.c ##
-@@ reftable/refname_test.c: 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,
+ ## reftable/reftable-stack.h ##
+@@ reftable/reftable-stack.h: struct reftable_stack;
+ * stored in 'dir'. Typically, this should be .git/reftables.
+ */
+ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+- struct reftable_write_options opts);
++ const struct reftable_write_options *opts);
+
+ /* returns the update_index at which a next table should be written. */
+ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);

## reftable/reftable-writer.h ##
@@ reftable/reftable-writer.h: struct reftable_stats {
@@ reftable/reftable-writer.h: struct reftable_stats {
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);
++ void *writer_arg, const 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

## reftable/stack.c ##
-@@ reftable/stack.c: int reftable_addition_add(struct reftable_addition *add,
- tab_fd = get_tempfile_fd(tab_file);
+@@ reftable/stack.c: static int reftable_fd_flush(void *arg)
+ }
+
+ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
+- struct reftable_write_options opts)
++ const struct reftable_write_options *_opts)
+ {
+ struct reftable_stack *p = reftable_calloc(1, sizeof(*p));
+ struct strbuf list_file_name = STRBUF_INIT;
++ struct reftable_write_options opts = {0};
+ int err = 0;

- wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
-- &add->stack->opts);
-+ add->stack->opts);
- err = write_table(wr, arg);
++ if (_opts)
++ opts = *_opts;
+ if (opts.hash_id == 0)
+ opts.hash_id = GIT_SHA1_FORMAT_ID;
+
+@@ reftable/stack.c: int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
+ struct reftable_merged_table *merged = NULL;
+ struct reftable_table table = { NULL };
+
+- int err = reftable_new_stack(&stack, stackdir, opts);
++ int err = reftable_new_stack(&stack, stackdir, &opts);
if (err < 0)
goto done;
-@@ reftable/stack.c: static int stack_compact_locked(struct reftable_stack *st,
+
+
+ ## reftable/stack_test.c ##
+@@ reftable/stack_test.c: static void test_reftable_stack_add_one(void)
+ };
+ struct reftable_ref_record dest = { NULL };
+ struct stat stat_result = { 0 };
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st, &write_test_ref, &ref);
+@@ reftable/stack_test.c: static void test_reftable_stack_uptodate(void)
+ /* simulate multi-process access to the same stack
+ by creating two stacks for the same directory.
+ */
+- err = reftable_new_stack(&st1, dir, opts);
++ err = reftable_new_stack(&st1, dir, &opts);
+ EXPECT_ERR(err);
+
+- err = reftable_new_stack(&st2, dir, opts);
++ err = reftable_new_stack(&st2, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st1, &write_test_ref, &ref1);
+@@ reftable/stack_test.c: static void test_reftable_stack_transaction_api(void)
+ };
+ struct reftable_ref_record dest = { NULL };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ reftable_addition_destroy(add);
+@@ reftable/stack_test.c: static void test_reftable_stack_transaction_api_performs_auto_compaction(void)
+ struct reftable_stack *st = NULL;
+ int i, n = 20, err;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i <= n; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction_fails_gracefully(void)
+ char *dir = get_tmp_dir(__LINE__);
+ int err;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st, write_test_ref, &ref);
+@@ reftable/stack_test.c: static void test_reftable_stack_update_index_check(void)
+ .value.symref = "master",
+ };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st, &write_test_ref, &ref1);
+@@ reftable/stack_test.c: static void test_reftable_stack_lock_failure(void)
+ struct reftable_stack *st = NULL;
+ int err, i;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+ for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) {
+ err = reftable_stack_add(st, &write_error, &i);
+@@ reftable/stack_test.c: static void test_reftable_stack_add(void)
+ struct stat stat_result;
+ int N = ARRAY_SIZE(refs);
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i < N; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_log_normalize(void)
+ .update_index = 1,
+ };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ input.value.update.message = "one\ntwo";
+@@ reftable/stack_test.c: static void test_reftable_stack_tombstone(void)
+ struct reftable_ref_record dest = { NULL };
+ struct reftable_log_record log_dest = { NULL };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ /* even entries add the refs, odd entries delete them. */
+@@ reftable/stack_test.c: static void test_reftable_stack_hash_id(void)
+ struct reftable_stack *st_default = NULL;
+ struct reftable_ref_record dest = { NULL };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st, &write_test_ref, &ref);
+ EXPECT_ERR(err);
+
+ /* can't read it with the wrong hash ID. */
+- err = reftable_new_stack(&st32, dir, opts32);
++ err = reftable_new_stack(&st32, dir, &opts32);
+ EXPECT(err == REFTABLE_FORMAT_ERROR);
+
+ /* check that we can read it back with default opts too. */
+- err = reftable_new_stack(&st_default, dir, opts_default);
++ err = reftable_new_stack(&st_default, dir, &opts_default);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_read_ref(st_default, "master", &dest);
+@@ reftable/stack_test.c: static void test_reflog_expire(void)
+ };
+ struct reftable_log_record log = { NULL };
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 1; i <= N; i++) {
+@@ reftable/stack_test.c: static void test_empty_add(void)
+ char *dir = get_tmp_dir(__LINE__);
+ struct reftable_stack *st2 = NULL;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_add(st, &write_nothing, NULL);
+ EXPECT_ERR(err);
+
+- err = reftable_new_stack(&st2, dir, opts);
++ err = reftable_new_stack(&st2, dir, &opts);
+ EXPECT_ERR(err);
+ clear_dir(dir);
+ reftable_stack_destroy(st);
+@@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
+ int err, i;
+ int N = 100;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i < N; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_add_performs_auto_compaction(void)
+ char *dir = get_tmp_dir(__LINE__);
+ int err, i, n = 20;
+
+- err = reftable_new_stack(&st, dir, opts);
++ err = reftable_new_stack(&st, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i <= n; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent(void)
+ int err, i;
+ int N = 3;
+
+- err = reftable_new_stack(&st1, dir, opts);
++ err = reftable_new_stack(&st1, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i < N; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent(void)
+ EXPECT_ERR(err);
}

- 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;
+- err = reftable_new_stack(&st2, dir, opts);
++ err = reftable_new_stack(&st2, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_compact_all(st1, NULL);
+@@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_clean(void)
+ int err, i;
+ int N = 3;
+
+- err = reftable_new_stack(&st1, dir, opts);
++ err = reftable_new_stack(&st1, dir, &opts);
+ EXPECT_ERR(err);
+
+ for (i = 0; i < N; i++) {
+@@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_clean(void)
+ EXPECT_ERR(err);
+ }
+
+- err = reftable_new_stack(&st2, dir, opts);
++ err = reftable_new_stack(&st2, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_compact_all(st1, NULL);
+@@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_clean(void)
+ unclean_stack_close(st1);
+ unclean_stack_close(st2);
+
+- err = reftable_new_stack(&st3, dir, opts);
++ err = reftable_new_stack(&st3, dir, &opts);
+ EXPECT_ERR(err);
+
+ err = reftable_stack_clean(st3);

## reftable/writer.c ##
@@ reftable/writer.c: static struct strbuf reftable_empty_strbuf = STRBUF_INIT;
@@ reftable/writer.c: static struct strbuf reftable_empty_strbuf = STRBUF_INIT;
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)
++ void *writer_arg, const struct reftable_write_options *_opts)
{
struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
- strbuf_init(&wp->block_writer_data.last_key, 0);
+- strbuf_init(&wp->block_writer_data.last_key, 0);
- options_set_defaults(opts);
- if (opts->block_size >= (1 << 24)) {
++ struct reftable_write_options opts = {0};
+
++ if (_opts)
++ opts = *_opts;
+ options_set_defaults(&opts);
+ if (opts.block_size >= (1 << 24)) {
/* TODO - error return? */
abort();
}
+
++ strbuf_init(&wp->block_writer_data.last_key, 0);
wp->last_key = reftable_empty_strbuf;
- REFTABLE_CALLOC_ARRAY(wp->block, opts->block_size);
+ REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
3: aa2903e3e5 ! 3: ef14bf7195 reftable/writer: drop static variable used to initialize strbuf
@@ reftable/writer.c: static void writer_reinit_block_writer(struct reftable_writer
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)
- {
- 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)) {
@@ reftable/writer.c: reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
- abort();
}

+ strbuf_init(&wp->block_writer_data.last_key, 0);
- wp->last_key = reftable_empty_strbuf;
-+ strbuf_init(&wp->block_writer_data.last_key, 0);
+ strbuf_init(&wp->last_key, 0);
REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
wp->write = writer_func;
4: 5e7cbb7b19 ! 4: 8ec26646f2 reftable/writer: improve error when passed an invalid block size
@@ Commit message

## reftable/writer.c ##
@@ reftable/writer.c: reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
- struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
-
+ if (_opts)
+ opts = *_opts;
options_set_defaults(&opts);
- if (opts.block_size >= (1 << 24)) {
- /* TODO - error return? */

 5:  ed1c150d90 =  5:  c4377180ef reftable/dump: support dumping a table's block structure
 6:  be5bdc6dc1 =  6:  70720af4d3 refs/reftable: allow configuring block size
 7:  05e8d1df2d =  7:  b3fe81b7b7 reftable: use `uint16_t` to track restart interval
 8:  bc0bf65553 !  8:  2b15795707 refs/reftable: allow configuring restart interval
    @@ Documentation/config/reftable.txt: readers during access.
     +
     +reftable.restartInterval::
     +	The interval at which to create restart points. The reftable backend
    -+	determines the restart points at file creation. The process is
    -+	arbitrary, but every 16 or 64 records is recommended. Every 16 may be
    ++	determines the restart points at file creation. Every 16 may be
     +	more suitable for smaller block sizes (4k or 8k), every 64 for larger
     +	block sizes (64k).
     ++
 9:  6bc240fd0c =  9:  b128d584a5 refs/reftable: allow disabling writing the object index
10:  9d4c1f0340 ! 10:  fb1ca02e77 reftable: make the compaction factor configurable

@@ reftable/stack.c: license that can be found in the LICENSE file or at
+#include "constants.h"
#include "merged.h"
#include "reader.h"
- #include "refname.h"
+ #include "reftable-error.h"
@@ reftable/stack.c: static int segment_size(struct segment *s)
return s->end - s->start;
}
11: e1282e53fb = 11: d341741eb0 refs/reftable: allow configuring geometric factor

2.45.GIT

Throughout the reftable library the reftable_write_options are sometimes referred to as cfg and sometimes as opts. Unify these to consistently use opts to avoid confusion.

While at it, touch up the coding style a bit by removing unneeded braces around one-line statements and newlines between variable declarations.

---
 reftable/dump.c           |   4 +-
 reftable/reftable-stack.h |   2 +-
 reftable/stack.c          |  41 +++++++-------
 reftable/stack.h          |   2 +-
 reftable/stack_test.c     | 114 +++++++++++++++++---------------------
 5 files changed, 74 insertions(+), 89 deletions(-)

index 26e0393c7d..9c770a10cc 100644
@@ -27,9 +27,9 @@ license that can be found in the LICENSE file or at
 static int compact_stack(const char *stackdir)
 {
 	struct reftable_stack *stack = NULL;
-	struct reftable_write_options cfg = { 0 };
+	struct reftable_write_options opts = { 0 };
 
-	int err = reftable_new_stack(&stack, stackdir, cfg);
+	int err = reftable_new_stack(&stack, stackdir, opts);
 	if (err < 0)
 		goto done;
 

index 1b602dda58..9c8e4eef49 100644
@@ -29,7 +29,7 @@ struct reftable_stack;
  *  stored in 'dir'. Typically, this should be .git/reftables.
  */
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       struct reftable_write_options config);
+		       struct reftable_write_options opts);
 
 /* returns the update_index at which a next table should be written. */
 uint64_t reftable_stack_next_update_index(struct reftable_stack *st);

index d43efa4760..97d7ebc043 100644
@@ -20,7 +20,7 @@ struct reftable_stack {
 
 	char *reftable_dir;
 
-	struct reftable_write_options config;
+	struct reftable_write_options opts;
 
 	struct reftable_reader **readers;
 	size_t readers_len;

2.45.GIT

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.

Adapt the code so that we do not modify the caller-provided values anymore. While at it, refactor the code to code to consistently pass the options as a constant pointer to clarify that the caller-provided opts will not ever get modified.

---
 refs/reftable-backend.c    |  6 ++---
 reftable/dump.c            |  2 +-
 reftable/reftable-stack.h  |  2 +-
 reftable/reftable-writer.h |  2 +-
 reftable/stack.c           |  7 ++++--
 reftable/stack_test.c      | 48 +++++++++++++++++++-------------------
 reftable/writer.c          | 17 +++++++++-----
 7 files changed, 46 insertions(+), 38 deletions(-)

index 010ef811b6..f8f930380d 100644
@@ -129,7 +129,7 @@ static struct reftable_stack *stack_for(struct reftable_ref_store *store,
 				    store->base.repo->commondir, wtname_buf.buf);
 
 			store->err = reftable_new_stack(&stack, wt_dir.buf,
-							store->write_options);
+							&store->write_options);
 			assert(store->err != REFTABLE_API_ERROR);
 			strmap_put(&store->worktree_stacks, wtname_buf.buf, stack);
 		}

@@ -263,7 +263,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	}
 	strbuf_addstr(&path, "/reftable");
 	refs->err = reftable_new_stack(&refs->main_stack, path.buf,
-				       refs->write_options);
+				       &refs->write_options);
 	if (refs->err)
 		goto done;
 

@@ -280,7 +280,7 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 		strbuf_addf(&path, "%s/reftable", gitdir);
 
 		refs->err = reftable_new_stack(&refs->worktree_stack, path.buf,
-					       refs->write_options);
+					       &refs->write_options);
 		if (refs->err)
 			goto done;
 	}

index 9c770a10cc..586f3eb288 100644
@@ -29,7 +29,7 @@ static int compact_stack(const char *stackdir)
 	struct reftable_stack *stack = NULL;
 	struct reftable_write_options opts = { 0 };
 
-	int err = reftable_new_stack(&stack, stackdir, opts);
+	int err = reftable_new_stack(&stack, stackdir, &opts);
 	if (err < 0)
 		goto done;
 

index 9c8e4eef49..c15632c401 100644
@@ -29,7 +29,7 @@ struct reftable_stack;
  *  stored in 'dir'. Typically, this should be .git/reftables.
  */
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       struct reftable_write_options opts);
+		       const struct reftable_write_options *opts);
 
 /* returns the update_index at which a next table should be written. */
 uint64_t reftable_stack_next_update_index(struct reftable_stack *st);

index b601a69a40..03df3a4963 100644
@@ -88,7 +88,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, const 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

index 54e7473f3a..d2e68be7e8 100644
@@ -54,12 +54,15 @@ static int reftable_fd_flush(void *arg)
 }
 
 int reftable_new_stack(struct reftable_stack **dest, const char *dir,
-		       struct reftable_write_options opts)
+		       const struct reftable_write_options *_opts)
 {
 	struct reftable_stack *p = reftable_calloc(1, sizeof(*p));
 	struct strbuf list_file_name = STRBUF_INIT;
+	struct reftable_write_options opts = {0};
 	int err = 0;
 
+	if (_opts)
+		opts = *_opts;
 	if (opts.hash_id == 0)
 		opts.hash_id = GIT_SHA1_FORMAT_ID;
 

@@ -1438,7 +1441,7 @@ int reftable_stack_print_directory(const char *stackdir, uint32_t hash_id)
 	struct reftable_merged_table *merged = NULL;
 	struct reftable_table table = { NULL };
 
-	int err = reftable_new_stack(&stack, stackdir, opts);
+	int err = reftable_new_stack(&stack, stackdir, &opts);
 	if (err < 0)
 		goto done;
 

index 10eccaaa07..4cc6e2ebd8 100644
@@ -122,20 +122,25 @@ 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, const 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)) {
+	struct reftable_write_options opts = {0};
+
+	if (_opts)
+		opts = *_opts;
+	options_set_defaults(&opts);
+	if (opts.block_size >= (1 << 24)) {
 		/* TODO - error return? */
 		abort();
 	}
+
+	strbuf_init(&wp->block_writer_data.last_key, 0);
 	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);
 

2.45.GIT

So from v2, we changed this from passing the value to passing the address. Mostly, we're trying to stay consistent and have to pick either, so this makes more sense since we're passing around big structs.

s/refatble/reftable

Doesn't really matter, but would it be more accurate to say "pointer to a constant type"?

Overall, I like this change. Improves consistency and readability :)

-Justin

True. As you mentioned, I'll not reroll this series just for these two small issues though.

Patrick

We have a static variable in the reftable writer code that is merely used to initialize the last_key of the writer. Convert the code to instead use strbuf_init() and drop the variable.

---
 reftable/writer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 4cc6e2ebd8..a043025b01 100644
@@ -117,8 +117,6 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
 	w->block_writer->restart_interval = w->opts.restart_interval;
 }
 
-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 *),

@@ -136,7 +134,7 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 	}
 
 	strbuf_init(&wp->block_writer_data.last_key, 0);
-	wp->last_key = reftable_empty_strbuf;
+	strbuf_init(&wp->last_key, 0);
 	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
 	wp->write = writer_func;
 	wp->write_arg = writer_arg;

2.45.GIT

The reftable format only supports block sizes up to 16MB. When the writer is being passed a value bigger than that it simply calls abort(3P), which isn't all that helpful due to the lack of a proper error message.

Improve this by calling BUG() instead.

---
 reftable/writer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index a043025b01..45b3e9ce1f 100644
@@ -128,10 +128,8 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 	if (_opts)
 		opts = *_opts;
 	options_set_defaults(&opts);
-	if (opts.block_size >= (1 << 24)) {
-		/* TODO - error return? */
-		abort();
-	}
+	if (opts.block_size >= (1 << 24))
+		BUG("configured block size exceeds 16MB");
 
 	strbuf_init(&wp->block_writer_data.last_key, 0);
 	strbuf_init(&wp->last_key, 0);

2.45.GIT

We're about to introduce new configs that will allow users to have more control over how exactly reftables are written. To verify that these configs are effective we will need to take a peak into the actual blocks written by the reftable backend.

Introduce a new mode to the dumping logic that prints out the block structure. This logic can be invoked via test-tool dump-reftables -b.

---
 reftable/dump.c                   |   8 ++-
 reftable/reader.c                 |  63 ++++++++++++++++++
 reftable/reftable-reader.h        |   2 +
 t/t0613-reftable-write-options.sh | 102 ++++++++++++++++++++++++++++++
 4 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100755 t/t0613-reftable-write-options.sh

index 586f3eb288..41abbb8ecf 100644
@@ -48,6 +48,7 @@ static void print_help(void)
 	printf("usage: dump [-cst] arg\n\n"
 	       "options: \n"
 	       "  -c compact\n"
+	       "  -b dump blocks\n"
 	       "  -t dump table\n"
 	       "  -s dump stack\n"
 	       "  -6 sha256 hash format\n"

@@ -58,6 +59,7 @@ static void print_help(void)
 int reftable_dump_main(int argc, char *const *argv)
 {
 	int err = 0;
+	int opt_dump_blocks = 0;
 	int opt_dump_table = 0;
 	int opt_dump_stack = 0;
 	int opt_compact = 0;

@@ -67,6 +69,8 @@ int reftable_dump_main(int argc, char *const *argv)
 	for (; argc > 1; argv++, argc--)
 		if (*argv[1] != '-')
 			break;
+		else if (!strcmp("-b", argv[1]))
+			opt_dump_blocks = 1;
 		else if (!strcmp("-t", argv[1]))
 			opt_dump_table = 1;
 		else if (!strcmp("-6", argv[1]))

@@ -88,7 +92,9 @@ int reftable_dump_main(int argc, char *const *argv)
 
 	arg = argv[1];
 
-	if (opt_dump_table) {
+	if (opt_dump_blocks) {
+		err = reftable_reader_print_blocks(arg);
+	} else if (opt_dump_table) {
 		err = reftable_reader_print_file(arg);
 	} else if (opt_dump_stack) {
 		err = reftable_stack_print_directory(arg, opt_hash_id);

index 4a4bc2fdf8..4a04857773 100644
@@ -97,5 +97,7 @@ void reftable_table_from_reader(struct reftable_table *tab,
 
 /* print table onto stdout for debugging. */
 int reftable_reader_print_file(const char *tablename);
+/* print blocks onto stdout for debugging. */
+int reftable_reader_print_blocks(const char *tablename);
 
 #endif

2.45.GIT