mbox series

[v3,00/11] reftable: expose write options as config

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

Message

Patrick Steinhardt May 13, 2024, 8:17 a.m. UTC
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 <ps@pks.im>
     
      ## 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.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    - ## 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

Comments

karthik nayak May 17, 2024, 8:14 a.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> 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.
>

I did go through the patches and only had a small nit, but not worth a
re-roll.

I was also wondering what happens if users tweak these values when a
repository already contains reftables with different values. Seems like
it'll use the new configuration during new table creation and also
during autocompaction. Which makes sense.

Thanks
Karthik
Patrick Steinhardt May 17, 2024, 8:26 a.m. UTC | #2
On Fri, May 17, 2024 at 10:14:19AM +0200, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> >
> 
> I did go through the patches and only had a small nit, but not worth a
> re-roll.

Thanks!

> I was also wondering what happens if users tweak these values when a
> repository already contains reftables with different values. Seems like
> it'll use the new configuration during new table creation and also
> during autocompaction. Which makes sense.

Yup. It should be fine to change the values at will and for different
tables to be written with different configs.

Patrick
Justin Tobler May 21, 2024, 11:54 p.m. UTC | #3
On 24/05/13 10:17AM, Patrick Steinhardt wrote:
> 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.

Thanks Patrick! I reviewed this version and left a couple
comments/questions, but nothing that would neccessitate a reroll. :)

-Justin
Patrick Steinhardt May 22, 2024, 7:19 a.m. UTC | #4
On Tue, May 21, 2024 at 06:54:33PM -0500, Justin Tobler wrote:
> On 24/05/13 10:17AM, Patrick Steinhardt wrote:
> > 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.
> 
> Thanks Patrick! I reviewed this version and left a couple
> comments/questions, but nothing that would neccessitate a reroll. :)

Thanks for your review! As mentioned, I agree that there is no strong
motivator to reroll this series as the only changes would be a typo fix
in the second patch.

Patrick