[PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`

[Karthik Nayak <karthik.188@gmail.com>]

In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,
2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so
that callers of `reftable_stack_init_addition()` can also reload the
stack if there was a concurrent update made before the lock was
obtained.

Then 16684b6fae (refs/reftable: always reload stacks when creating
lock, 2025-08-12) updated all of the remaining call-sites to propagate
this flag to ensure that we always reload the stack whenever there was a
concurrent update.

As all calls to `reftable_stack_init_addition()` inevitably propagate
the flag, it is safe to remove the flag and its associated code and make
the reloading of the stack the default flow. This makes it easier to
follow the flow and simplifies the logic.

The only exceptions are:

  1. Unit tests, where we explicitly do not propagate the flag. These
     tests are now modified with the new status quo.

  2. `reftable_stack_clean_locked()`, which was propagating 0 to
     `reftable_stack_new_addition()` but was then manually reloading the
     stack after. Here the new flow will achieve the same, while also
     allowing us to remove the manual reload.

This also makes two checks for 'REFTABLE_OUTDATED_ERROR' redundant, so
remove them also.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/reftable-backend.c         | 18 ++++-------
 reftable/reftable-stack.h       | 17 ++--------
 reftable/stack.c                | 37 ++++++----------------
 t/unit-tests/u-reftable-stack.c | 69 ++++++++++++++++++-----------------------
 4 files changed, 49 insertions(+), 92 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 028f0211af..5c87fd2d68 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1003,8 +1003,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
 		struct reftable_addition *addition;
 
 		ret = reftable_stack_new_addition(&addition, be->stack,
-						  &reftable_be_write_options(refs)->opts,
-						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
+						  &reftable_be_write_options(refs)->opts);
 		if (ret) {
 			if (ret == REFTABLE_LOCK_ERROR)
 				strbuf_addstr(err, "cannot lock references");
@@ -2010,8 +2009,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,
-				 &reftable_be_write_options(refs)->opts,
-				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
+				 &reftable_be_write_options(refs)->opts);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2041,8 +2039,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,
-				 &reftable_be_write_options(refs)->opts,
-				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
+				 &reftable_be_write_options(refs)->opts);
 
 done:
 	assert(ret != REFTABLE_API_ERROR);
@@ -2424,8 +2421,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,
-				 &reftable_be_write_options(refs)->opts,
-				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
+				 &reftable_be_write_options(refs)->opts);
 
 done:
 	return ret;
@@ -2499,8 +2495,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,
-				 &reftable_be_write_options(refs)->opts,
-				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
+				 &reftable_be_write_options(refs)->opts);
 
 	assert(ret != REFTABLE_API_ERROR);
 	return ret;
@@ -2622,8 +2617,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
 		goto done;
 
 	ret = reftable_stack_new_addition(&add, be->stack,
-					  &reftable_be_write_options(refs)->opts,
-					  REFTABLE_STACK_NEW_ADDITION_RELOAD);
+					  &reftable_be_write_options(refs)->opts);
 	if (ret < 0)
 		goto done;
 
diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
index 5d22d84e80..5d224f8079 100644
--- a/reftable/reftable-stack.h
+++ b/reftable/reftable-stack.h
@@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);
 /* holds a transaction to add tables at the top of a stack. */
 struct reftable_addition;
 
-enum {
-	/*
-	 * Reload the stack when the stack is out-of-date after locking it.
-	 */
-	REFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),
-};
-
 /*
  * returns a new transaction to add reftables to the given stack. As a side
- * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*
- * flags.
+ * effect, the ref database is locked.
  */
 int reftable_stack_new_addition(struct reftable_addition **dest,
 				struct reftable_stack *st,
-				const struct reftable_write_options *opts,
-				unsigned int flags);
+				const struct reftable_write_options *opts);
 
 /* Adds a reftable to transaction. */
 int reftable_addition_add(struct reftable_addition *add,
@@ -93,14 +84,12 @@ void reftable_addition_destroy(struct reftable_addition *add);
 /*
  * Add a new table to the stack. The write_table function must call
  * reftable_writer_set_limits, add refs and return an error value.
- * The flags are passed through to `reftable_stack_new_addition()`.
  */
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write_table)(struct reftable_writer *wr,
 					  void *write_arg),
 		       void *write_arg,
-		       const struct reftable_write_options *opts,
-		       unsigned flags);
+		       const struct reftable_write_options *opts);
 
 struct reftable_iterator;
 
diff --git a/reftable/stack.c b/reftable/stack.c
index 308f9578f0..540f5e77ac 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -659,8 +659,7 @@ static void reftable_addition_close(struct reftable_addition *add)
 
 static int reftable_stack_init_addition(struct reftable_addition *add,
 					struct reftable_stack *st,
-					const struct reftable_write_options *opts,
-					unsigned int flags)
+					const struct reftable_write_options *opts)
 {
 	struct reftable_buf lock_file_name = REFTABLE_BUF_INIT;
 	int err;
@@ -686,15 +685,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 	err = stack_uptodate(st);
 	if (err < 0)
 		goto done;
-	if (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {
+	if (err > 0) {
 		err = reftable_stack_reload_maybe_reuse(add->stack, 1);
 		if (err)
 			goto done;
 	}
-	if (err > 0) {
-		err = REFTABLE_OUTDATED_ERROR;
-		goto done;
-	}
 
 	add->next_update_index = reftable_stack_next_update_index(st);
 done:
@@ -708,13 +703,12 @@ static int stack_try_add(struct reftable_stack *st,
 			 int (*write_table)(struct reftable_writer *wr,
 					    void *arg),
 			 void *arg,
-			 const struct reftable_write_options *opts,
-			 unsigned flags)
+			 const struct reftable_write_options *opts)
 {
 	struct reftable_addition add;
 	int err;
 
-	err = reftable_stack_init_addition(&add, st, opts, flags);
+	err = reftable_stack_init_addition(&add, st, opts);
 	if (err < 0)
 		goto done;
 
@@ -731,17 +725,10 @@ static int stack_try_add(struct reftable_stack *st,
 int reftable_stack_add(struct reftable_stack *st,
 		       int (*write)(struct reftable_writer *wr, void *arg),
 		       void *arg,
-		       const struct reftable_write_options *opts,
-		       unsigned flags)
+		       const struct reftable_write_options *opts)
 {
-	int err = stack_try_add(st, write, arg, opts, flags);
+	int err = stack_try_add(st, write, arg, opts);
 	if (err < 0) {
-		if (err == REFTABLE_OUTDATED_ERROR) {
-			/* Ignore error return, we want to propagate
-			   REFTABLE_OUTDATED_ERROR.
-			*/
-			reftable_stack_reload(st);
-		}
 		return err;
 	}
 
@@ -843,8 +830,7 @@ int reftable_addition_commit(struct reftable_addition *add)
 
 int reftable_stack_new_addition(struct reftable_addition **dest,
 				struct reftable_stack *st,
-				const struct reftable_write_options *opts,
-				unsigned int flags)
+				const struct reftable_write_options *opts)
 {
 	int err;
 
@@ -852,7 +838,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
 	if (!*dest)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 
-	err = reftable_stack_init_addition(*dest, st, opts, flags);
+	err = reftable_stack_init_addition(*dest, st, opts);
 	if (err) {
 		reftable_free(*dest);
 		*dest = NULL;
@@ -1840,12 +1826,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)
 int reftable_stack_clean(struct reftable_stack *st)
 {
 	struct reftable_addition *add = NULL;
-	int err = reftable_stack_new_addition(&add, st, NULL, 0);
-	if (err < 0) {
-		goto done;
-	}
-
-	err = reftable_stack_reload(st);
+	int err = reftable_stack_new_addition(&add, st, NULL);
 	if (err < 0) {
 		goto done;
 	}
diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c
index e6c1635940..c6254190e6 100644
--- a/t/unit-tests/u-reftable-stack.c
+++ b/t/unit-tests/u-reftable-stack.c
@@ -127,7 +127,7 @@ static void write_n_ref_tables(struct reftable_stack *st,
 		cl_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);
 
 		cl_assert_equal_i(reftable_stack_add(st,
-						     &write_test_ref, &ref, &opts, 0), 0);
+						     &write_test_ref, &ref, &opts), 0);
 	}
 }
 
@@ -168,7 +168,7 @@ void test_reftable_stack__add_one(void)
 	err = reftable_new_stack(&st, dir, NULL);
 	cl_assert(!err);
 
-	err = reftable_stack_add(st, write_test_ref, &ref, &opts, 0);
+	err = reftable_stack_add(st, write_test_ref, &ref, &opts);
 	cl_assert(!err);
 
 	err = reftable_stack_read_ref(st, ref.refname, &dest);
@@ -231,12 +231,9 @@ void test_reftable_stack__uptodate(void)
 	cl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);
 	cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st1, write_test_ref,
-					     &ref1, NULL, 0), 0);
+					     &ref1, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st2, write_test_ref,
-					     &ref2, NULL, 0), REFTABLE_OUTDATED_ERROR);
-	cl_assert_equal_i(reftable_stack_reload(st2), 0);
-	cl_assert_equal_i(reftable_stack_add(st2, write_test_ref,
-					     &ref2, NULL, 0), 0);
+					     &ref2, NULL), 0);
 	reftable_stack_destroy(st1);
 	reftable_stack_destroy(st2);
 	clear_dir(dir);
@@ -260,7 +257,7 @@ void test_reftable_stack__transaction_api(void)
 
 	reftable_addition_destroy(add);
 
-	cl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL, 0), 0);
+	cl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL), 0);
 	cl_assert_equal_i(reftable_addition_add(add, write_test_ref,
 						&ref), 0);
 	cl_assert_equal_i(reftable_addition_commit(add), 0);
@@ -301,21 +298,17 @@ void test_reftable_stack__transaction_with_reload(void)
 
 	cl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);
 	cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);
-	cl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL, 0), 0);
+	cl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL), 0);
 	cl_assert_equal_i(reftable_addition_add(add, write_test_ref,
 						&refs[0]), 0);
 	cl_assert_equal_i(reftable_addition_commit(add), 0);
 	reftable_addition_destroy(add);
 
 	/*
-	 * The second stack is now outdated, which we should notice. We do not
-	 * create the addition and lock the stack by default, but allow the
-	 * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.
+	 * The second stack is now outdated, but it should automatically reload it
+	 * with the newer updates.
 	 */
-	cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL, 0),
-						      REFTABLE_OUTDATED_ERROR);
-	cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL,
-						      REFTABLE_STACK_NEW_ADDITION_RELOAD), 0);
+	cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL), 0);
 	cl_assert_equal_i(reftable_addition_add(add, write_test_ref,
 						&refs[1]), 0);
 	cl_assert_equal_i(reftable_addition_commit(add), 0);
@@ -363,7 +356,7 @@ void test_reftable_stack__transaction_api_performs_auto_compaction(void)
 		 * better control over when exactly auto compaction runs.
 		 */
 		cl_assert_equal_i(reftable_stack_new_addition(&add,
-							      st, &write_opts, 0), 0);
+							      st, &write_opts), 0);
 		cl_assert_equal_i(reftable_addition_add(add,
 							write_test_ref, &ref), 0);
 		cl_assert_equal_i(reftable_addition_commit(add), 0);
@@ -400,7 +393,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void)
 
 	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-					     &ref, NULL, 0), 0);
+					     &ref, NULL), 0);
 	cl_assert_equal_i(st->merged->tables_len, 1);
 	cl_assert_equal_i(st->stats.attempts, 0);
 	cl_assert_equal_i(st->stats.failures, 0);
@@ -418,7 +411,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void)
 	write_file_buf(table_path.buf, "", 0);
 
 	ref.update_index = 2;
-	err = reftable_stack_add(st, write_test_ref, &ref, NULL, 0);
+	err = reftable_stack_add(st, write_test_ref, &ref, NULL);
 	cl_assert(!err);
 	cl_assert_equal_i(st->merged->tables_len, 2);
 	cl_assert_equal_i(st->stats.attempts, 1);
@@ -453,9 +446,9 @@ void test_reftable_stack__update_index_check(void)
 
 	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-					     &ref1, NULL, 0), 0);
+					     &ref1, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-					     &ref2, NULL, 0), REFTABLE_API_ERROR);
+					     &ref2, NULL), REFTABLE_API_ERROR);
 	reftable_stack_destroy(st);
 	clear_dir(dir);
 }
@@ -469,7 +462,7 @@ void test_reftable_stack__lock_failure(void)
 	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
 	for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--)
 		cl_assert_equal_i(reftable_stack_add(st, write_error,
-						     &i, NULL, 0), i);
+						     &i, NULL), i);
 
 	reftable_stack_destroy(st);
 	clear_dir(dir);
@@ -513,7 +506,7 @@ void test_reftable_stack__add(void)
 
 	for (i = 0; i < N; i++)
 		cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-						     &refs[i], &opts, 0), 0);
+						     &refs[i], &opts), 0);
 
 	for (i = 0; i < N; i++) {
 		struct write_log_arg arg = {
@@ -521,7 +514,7 @@ void test_reftable_stack__add(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 		cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-						     &arg, &opts, 0), 0);
+						     &arg, &opts), 0);
 	}
 
 	cl_assert_equal_i(reftable_stack_compact_all(st, &opts, NULL), 0);
@@ -604,7 +597,7 @@ void test_reftable_stack__iterator(void)
 
 	for (i = 0; i < N; i++)
 		cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-						     &refs[i], NULL, 0), 0);
+						     &refs[i], NULL), 0);
 
 	for (i = 0; i < N; i++) {
 		struct write_log_arg arg = {
@@ -613,7 +606,7 @@ void test_reftable_stack__iterator(void)
 		};
 
 		cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-						     &arg, NULL, 0), 0);
+						     &arg, NULL), 0);
 	}
 
 	reftable_stack_init_ref_iterator(st, &it);
@@ -685,11 +678,11 @@ void test_reftable_stack__log_normalize(void)
 
 	input.value.update.message = (char *) "one\ntwo";
 	cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-					     &arg, NULL, 0), REFTABLE_API_ERROR);
+					     &arg, NULL), REFTABLE_API_ERROR);
 
 	input.value.update.message = (char *) "one";
 	cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-					     &arg, NULL, 0), 0);
+					     &arg, NULL), 0);
 	cl_assert_equal_i(reftable_stack_read_log(st, input.refname,
 						  &dest), 0);
 	cl_assert_equal_s(dest.value.update.message, "one\n");
@@ -697,7 +690,7 @@ void test_reftable_stack__log_normalize(void)
 	input.value.update.message = (char *) "two\n";
 	arg.update_index = 2;
 	cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-					     &arg, NULL, 0), 0);
+					     &arg, NULL), 0);
 	cl_assert_equal_i(reftable_stack_read_log(st, input.refname,
 						  &dest), 0);
 	cl_assert_equal_s(dest.value.update.message, "two\n");
@@ -747,7 +740,7 @@ void test_reftable_stack__tombstone(void)
 	}
 	for (i = 0; i < N; i++)
 		cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-						     &refs[i], NULL, 0), 0);
+						     &refs[i], NULL), 0);
 
 	for (i = 0; i < N; i++) {
 		struct write_log_arg arg = {
@@ -755,7 +748,7 @@ void test_reftable_stack__tombstone(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 		cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-						     &arg, NULL, 0), 0);
+						     &arg, NULL), 0);
 	}
 
 	cl_assert_equal_i(reftable_stack_read_ref(st, "branch",
@@ -801,7 +794,7 @@ void test_reftable_stack__hash_id(void)
 
 	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-					     &ref, NULL, 0), 0);
+					     &ref, NULL), 0);
 
 	/* can't read it with the wrong hash ID. */
 	cl_assert_equal_i(reftable_new_stack(&st32, dir,
@@ -869,7 +862,7 @@ void test_reftable_stack__reflog_expire(void)
 			.update_index = reftable_stack_next_update_index(st),
 		};
 		cl_assert_equal_i(reftable_stack_add(st, write_test_log,
-						     &arg, NULL, 0), 0);
+						     &arg, NULL), 0);
 	}
 
 	cl_assert_equal_i(reftable_stack_compact_all(st, NULL, NULL), 0);
@@ -908,7 +901,7 @@ void test_reftable_stack__empty_add(void)
 
 	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
 	cl_assert_equal_i(reftable_stack_add(st, write_nothing,
-					     NULL, NULL, 0), 0);
+					     NULL, NULL), 0);
 	cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);
 	clear_dir(dir);
 	reftable_stack_destroy(st);
@@ -947,7 +940,7 @@ void test_reftable_stack__auto_compaction(void)
 		};
 		snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, write_test_ref, &ref, &opts, 0);
+		err = reftable_stack_add(st, write_test_ref, &ref, &opts);
 		cl_assert(!err);
 
 		err = reftable_stack_auto_compact(st, &opts);
@@ -983,7 +976,7 @@ void test_reftable_stack__auto_compaction_factor(void)
 		};
 		xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i);
 
-		err = reftable_stack_add(st, &write_test_ref, &ref, &opts, 0);
+		err = reftable_stack_add(st, &write_test_ref, &ref, &opts);
 		cl_assert(!err);
 
 		cl_assert(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));
@@ -1064,7 +1057,7 @@ void test_reftable_stack__add_performs_auto_compaction(void)
 		ref.refname = buf;
 
 		cl_assert_equal_i(reftable_stack_add(st, write_test_ref,
-						     &ref, &write_opts, 0), 0);
+						     &ref, &write_opts), 0);
 
 		/*
 		 * The stack length should grow continuously for all runs where
@@ -1303,7 +1296,7 @@ void test_reftable_stack__invalid_limit_updates(void)
 
 	reftable_addition_destroy(add);
 
-	cl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts, 0), 0);
+	cl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts), 0);
 
 	/*
 	 * write_limits_after_ref also updates the update indexes after adding

-- 
2.55.GIT