Thread (105 messages) 105 messages, 5 authors, 21d ago
COLD21d

[PATCH v5 10/11] refs/reftable: lazy-load configuration to fix chicken-and-egg

From: Patrick Steinhardt <hidden>
Date: 2026-06-22 08:28:56
Subsystem: the rest · Maintainer: Linus Torvalds

Same as with the "files" backend, the "reftable" backend also has a
chicken-and-egg problem with "onbranch" conditions. Fix this issue the
same as we did with the "files" backend by lazy-loading configuration.

Now that both the "files" and the "reftable" backend handle this
properly, add a generic test to t1400 that verifies that the user can
configure "core.logAllRefUpdates" via an "onbranch" condition. This is
mostly a nonsensical thing to do in the first place, but it serves as a
good sanity chekc.

Note that we had to move `should_write_log()` around so that it can
access the new `reftable_be_write_options()` function.

Signed-off-by: Patrick Steinhardt <redacted>
---
 refs/reftable-backend.c           | 146 ++++++++++++++++++++++----------------
 t/t0613-reftable-write-options.sh |  19 +++++
 t/t1400-update-ref.sh             |  12 ++++
 3 files changed, 116 insertions(+), 61 deletions(-)
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 608d71cf10..d74131a5ae 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -141,10 +141,21 @@ struct reftable_ref_store {
 	 */
 	struct strmap worktree_backends;
 	struct reftable_stack_options stack_options;
-	struct reftable_write_options write_options;
+
+	/*
+	 * Options used when writing to or compacting the reftable stacks.
+	 * These are parsed from the configuration lazily on first use via
+	 * `reftable_be_write_options()` so that we don't have to access the
+	 * configuration when initializing the ref store. Do not access these
+	 * fields directly, but use the accessor instead.
+	 */
+	struct reftable_be_write_options {
+		struct reftable_write_options opts;
+		enum log_refs_config log_all_ref_updates;
+		bool initialized;
+	} write_opts_lazy_loaded;
 
 	unsigned int store_flags;
-	enum log_refs_config log_all_ref_updates;
 	int err;
 };
 
@@ -285,26 +296,6 @@ static int backend_for(struct reftable_backend **out,
 	return ret;
 }
 
-static int should_write_log(struct reftable_ref_store *refs, const char *refname)
-{
-	enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;
-	if (log_refs_cfg == LOG_REFS_UNSET)
-		log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
-
-	switch (log_refs_cfg) {
-	case LOG_REFS_NONE:
-		return refs_reflog_exists(&refs->base, refname);
-	case LOG_REFS_ALWAYS:
-		return 1;
-	case LOG_REFS_NORMAL:
-		if (should_autocreate_reflog(log_refs_cfg, refname))
-			return 1;
-		return refs_reflog_exists(&refs->base, refname);
-	default:
-		BUG("unhandled core.logAllRefUpdates value %d", log_refs_cfg);
-	}
-}
-
 static void fill_reftable_log_record(struct reftable_log_record *log, const struct ident_split *split)
 {
 	const char *tz_begin;
@@ -336,38 +327,72 @@ static int reftable_be_config(const char *var, const char *value,
 			      void *payload)
 {
 	struct reftable_ref_store *refs = payload;
+	struct reftable_be_write_options *opts = &refs->write_opts_lazy_loaded;
 
 	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");
-		refs->write_options.block_size = block_size;
+		opts->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);
-		refs->write_options.restart_interval = restart_interval;
+		opts->opts.restart_interval = restart_interval;
 	} else if (!strcmp(var, "reftable.indexobjects")) {
-		refs->write_options.skip_index_objects = !git_config_bool(var, value);
+		opts->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);
-		refs->write_options.auto_compaction_factor = factor;
+		opts->opts.auto_compaction_factor = factor;
 	} else if (!strcmp(var, "reftable.locktimeout")) {
 		int64_t lock_timeout = git_config_int64(var, value, ctx->kvi);
 		if (lock_timeout > LONG_MAX)
 			die("reftable lock timeout cannot exceed %"PRIdMAX, (intmax_t)LONG_MAX);
 		if (lock_timeout < 0 && lock_timeout != -1)
 			die("reftable lock timeout does not support negative values other than -1");
-		refs->write_options.lock_timeout_ms = lock_timeout;
+		opts->opts.lock_timeout_ms = lock_timeout;
 	} else if (!strcmp(var, "core.logallrefupdates")) {
-		refs->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
+		opts->log_all_ref_updates = refs_parse_log_all_ref_updates_config(value);
 	}
 
 	return 0;
 }
 
+static const struct reftable_be_write_options *reftable_be_write_options(struct reftable_ref_store *refs)
+{
+	struct reftable_be_write_options *opts = &refs->write_opts_lazy_loaded;
+	mode_t mask;
+
+	if (opts->initialized)
+		return opts;
+
+	mask = umask(0);
+	umask(mask);
+
+	opts->opts.default_permissions = calc_shared_perm(refs->base.repo, 0666 & ~mask);
+	opts->opts.disable_auto_compact =
+		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
+	opts->opts.lock_timeout_ms = 100;
+	opts->log_all_ref_updates = LOG_REFS_UNSET;
+
+	repo_config(refs->base.repo, reftable_be_config, refs);
+
+	/*
+	 * 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 (!opts->opts.block_size)
+		opts->opts.block_size = 4096;
+
+	opts->initialized = true;
+	return opts;
+}
+
 static void reftable_be_reparent(const char *name UNUSED,
 				 const char *old_cwd,
 				 const char *new_cwd,
@@ -391,10 +416,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	struct strbuf refdir = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
 	bool is_worktree;
-	mode_t mask;
-
-	mask = umask(0);
-	umask(mask);
 
 	refs_compute_filesystem_location(gitdir, payload, &is_worktree, &refdir,
 					 &ref_common_dir);
@@ -413,23 +434,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	default:
 		BUG("unknown hash algorithm %d", repo->hash_algo->format_id);
 	}
-	refs->write_options.default_permissions = calc_shared_perm(repo, 0666 & ~mask);
-	refs->write_options.disable_auto_compact =
-		!git_env_bool("GIT_TEST_REFTABLE_AUTOCOMPACTION", 1);
-	refs->write_options.lock_timeout_ms = 100;
-	refs->log_all_ref_updates = LOG_REFS_UNSET;
-
-	repo_config(repo, reftable_be_config, refs);
-
-	/*
-	 * 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.
@@ -998,7 +1002,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
 		struct reftable_addition *addition;
 
 		ret = reftable_stack_new_addition(&addition, be->stack,
-						  &refs->write_options,
+						  &reftable_be_write_options(refs)->opts,
 						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 		if (ret) {
 			if (ret == REFTABLE_LOCK_ERROR)
@@ -1437,6 +1441,26 @@ static int transaction_update_cmp(const void *a, const void *b)
 	return strcmp(update_a->update->refname, update_b->update->refname);
 }
 
+static int should_write_log(struct reftable_ref_store *refs, const char *refname)
+{
+	enum log_refs_config log_refs_cfg = reftable_be_write_options(refs)->log_all_ref_updates;
+	if (log_refs_cfg == LOG_REFS_UNSET)
+		log_refs_cfg = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
+
+	switch (log_refs_cfg) {
+	case LOG_REFS_NONE:
+		return refs_reflog_exists(&refs->base, refname);
+	case LOG_REFS_ALWAYS:
+		return 1;
+	case LOG_REFS_NORMAL:
+		if (should_autocreate_reflog(log_refs_cfg, refname))
+			return 1;
+		return refs_reflog_exists(&refs->base, refname);
+	default:
+		BUG("unhandled core.logAllRefUpdates value %d", log_refs_cfg);
+	}
+}
+
 static int write_transaction_table(struct reftable_writer *writer, void *cb_data)
 {
 	struct write_transaction_table_arg *arg = cb_data;
@@ -1571,7 +1595,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
 				memcpy(log->value.update.old_hash,
 				       tx_update->current_oid.hash, GIT_MAX_RAWSZ);
 				log->value.update.message =
-					xstrndup(u->msg, arg->refs->write_options.block_size / 2);
+					xstrndup(u->msg, reftable_be_write_options(arg->refs)->opts.block_size / 2);
 			}
 		}
 
@@ -1687,9 +1711,9 @@ static int reftable_be_optimize(struct ref_store *ref_store,
 		stack = refs->main_backend.stack;
 
 	if (opts->flags & REFS_OPTIMIZE_AUTO)
-		ret = reftable_stack_auto_compact(stack, &refs->write_options);
+		ret = reftable_stack_auto_compact(stack, &reftable_be_write_options(refs)->opts);
 	else
-		ret = reftable_stack_compact_all(stack, &refs->write_options, NULL);
+		ret = reftable_stack_compact_all(stack, &reftable_be_write_options(refs)->opts, NULL);
 	if (ret < 0) {
 		ret = error(_("unable to compact stack: %s"),
 			    reftable_error_str(ret));
@@ -1723,7 +1747,7 @@ static int reftable_be_optimize_required(struct ref_store *ref_store,
 	if (opts->flags & REFS_OPTIMIZE_AUTO)
 		use_heuristics = true;
 
-	return reftable_stack_compaction_required(stack, &refs->write_options,
+	return reftable_stack_compaction_required(stack, &reftable_be_write_options(refs)->opts,
 						  use_heuristics, required);
 }
 
@@ -1843,7 +1867,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 		logs[logs_nr].refname = xstrdup(arg->newname);
 		logs[logs_nr].update_index = deletion_ts;
 		logs[logs_nr].value.update.message =
-			xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
+			xstrndup(arg->logmsg, reftable_be_write_options(arg->refs)->opts.block_size / 2);
 		memcpy(logs[logs_nr].value.update.old_hash, old_ref.value.val1, GIT_MAX_RAWSZ);
 		logs_nr++;
 
@@ -1882,7 +1906,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
 	logs[logs_nr].refname = xstrdup(arg->newname);
 	logs[logs_nr].update_index = creation_ts;
 	logs[logs_nr].value.update.message =
-		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
+		xstrndup(arg->logmsg, reftable_be_write_options(arg->refs)->opts.block_size / 2);
 	memcpy(logs[logs_nr].value.update.new_hash, old_ref.value.val1, GIT_MAX_RAWSZ);
 	logs_nr++;
 
@@ -1981,7 +2005,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
 	if (ret)
 		goto done;
 	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
-				 &refs->write_options,
+				 &reftable_be_write_options(refs)->opts,
 				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
@@ -2012,7 +2036,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
 	if (ret)
 		goto done;
 	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
-				 &refs->write_options,
+				 &reftable_be_write_options(refs)->opts,
 				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
@@ -2378,7 +2402,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
 	arg.stack = be->stack;
 
 	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
-				 &refs->write_options,
+				 &reftable_be_write_options(refs)->opts,
 				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 done:
@@ -2451,7 +2475,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
 	arg.stack = be->stack;
 
 	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
-				 &refs->write_options,
+				 &reftable_be_write_options(refs)->opts,
 				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
 
 	assert(ret != REFTABLE_API_ERROR);
@@ -2574,7 +2598,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
 		goto done;
 
 	ret = reftable_stack_new_addition(&add, be->stack,
-					  &refs->write_options,
+					  &reftable_be_write_options(refs)->opts,
 					  REFTABLE_STACK_NEW_ADDITION_RELOAD);
 	if (ret < 0)
 		goto done;
diff --git a/t/t0613-reftable-write-options.sh b/t/t0613-reftable-write-options.sh
index 26b716c75f..a65960d048 100755
--- a/t/t0613-reftable-write-options.sh
+++ b/t/t0613-reftable-write-options.sh
@@ -278,4 +278,23 @@ test_expect_success 'object index can be disabled' '
 	)
 '
 
+test_expect_success 'write options can be set up via onbranch condition' '
+	test_config_global core.logAllRefUpdates false &&
+	test_when_finished "rm -rf repo" &&
+	init_repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		test_commit B &&
+		cat >.git/include <<-\EOF &&
+		[reftable]
+			blockSize = 123
+		EOF
+		git config includeIf.onbranch:master.path "$(pwd)/.git/include" &&
+		git refs optimize &&
+		test-tool dump-reftable -b .git/reftable/*.ref >stats &&
+		test_grep "block_size: 123" stats
+	)
+'
+
 test_done
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1015f335e3..b8c3be6631 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -178,6 +178,18 @@ test_expect_success '--no-create-reflog overrides core.logAllRefUpdates=always'
 	test_must_fail git reflog exists $outside
 '
 
+test_expect_success 'core.logAllRefUpdates can be set up via onbranch condition' '
+	test_when_finished "git update-ref -d $outside" &&
+	test_when_finished "rm -f .git/include" &&
+	cat >.git/include <<-\EOF &&
+	[core]
+		logAllRefUpdates = always
+	EOF
+	test_config includeIf.onbranch:main.path "$(pwd)/.git/include" &&
+	git update-ref $outside $A &&
+	git reflog exists $outside
+'
+
 test_expect_success "create $m (by HEAD)" '
 	git update-ref HEAD $A &&
 	test $A = $(git show-ref -s --verify $m)
-- 
2.55.0.rc1.745.g43192e7977.dirty
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help