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

[Justin Tobler <jltobler@gmail.com>]

On 26/08/19 03:19PM, Karthik Nayak wrote:
> 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.

Ok, if all call sites already wire this flag, then we probably don't
need if anymore.

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

Makes sense.

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

I assume this means we no longer need to test for the case where we
don't reload.

>   2. `reftable_stack_clean_locked()`, which was propagating 0 to

Did you mean `reftable_stack_clean()`?

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

Out of curiousity, was this call site just forgotten previously? Or was
there any reason a manual reload was useful?

> This also makes two checks for 'REFTABLE_OUTDATED_ERROR' redundant, so
> remove them also.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
[snip]
> 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),
> -};

The flag is dropped now that it is the only behavior.

>  /*
>   * 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);

Signatures updated. Ok.

[snip]
> 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;
> -	}

`reftable_stack_init_addition()` now reload unconditionally. Looks good.

>  	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);
> -		}

Since we always reload now, the REFTABLE_OUTDATED_ERROR is no longer a
possibility and doesn't need to be handled anymore.

[snip]
> 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);

We no longer need to check for REFTABLE_OUTDATED_ERROR since the stack
is always reloaded now. Makes sense.

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

Same here.

The rest of this patch is just updating call sites and looks good.

-Justin