Re: [PATCH v3 8/8] refs/reftable: always reload stacks when creating lock

> When creating a new addition via either `reftable_stack_new_addition()`
> or its convenince wrapper `reftable_stack_add()` we:
> 
>   1. Create the "tables.list.lock" file.
> 
>   2. Verify that the current version of the "tables.list" file is
>      up-to-date.
> 
>   3. Write the new table records if so.
> 
> By default, the second step would cause us to bail out if we see that
> there has been a concurrent write to the stack that made our in-memory
> copy of the stack out-of-date. This is a safety mechanism to not write
> records to the stack based on outdated information.
> 
> The downside though is that concurrent writes may now cause us to bail
> out, which is not a good user experience. In addition, this isn't even
> necessary for us, as Git knows to perform all checks for the old state
> of references under the lock. (Well, in all except one case: when we
> expire the reflog we first create the log iterator before we create the
> lock, but this ordering is fixed as part of this commit.)
> 
> Consequently, most writers pass the `REFTABLE_STACK_NEW_ADDITION_RELOAD`
> flag. The effect of this flag is that we reload the stack after having
> acquired the lock in case the stack is out-of-date. This plugs the race
> with concurrent writers, but we continue performing the verifications of
> the expected old state to catch actual conflicts in the references we
> are about to write.
> 
> Adapt the remaining callsites that don't yet pass this flag to do so.
> While at it, drop a needless manual reload.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 3f0deab338..66d25411f1 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1006,10 +1006,6 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,
>  	if (!arg) {
>  		struct reftable_addition *addition;
>  
> -		ret = reftable_stack_reload(be->stack);
> -		if (ret)
> -			return ret;

> -
>  		ret = reftable_stack_new_addition(&addition, be->stack,
>  						  REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  		if (ret) {
> @@ -1960,7 +1956,8 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,
>  	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
>  	if (ret)
>  		goto done;
> -	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> +	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	assert(ret != REFTABLE_API_ERROR);
> @@ -1989,7 +1986,8 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,
>  	ret = backend_for(&arg.be, refs, newrefname, &newrefname, 1);
>  	if (ret)
>  		goto done;
> -	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, 0);
> +	ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	assert(ret != REFTABLE_API_ERROR);
> @@ -2360,7 +2358,8 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,
>  		goto done;
>  	arg.stack = be->stack;
>  
> -	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, 0);
> +	ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  done:
>  	return ret;
> @@ -2431,7 +2430,8 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,
>  		return ret;
>  	arg.stack = be->stack;
>  
> -	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, 0);
> +	ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,
> +				 REFTABLE_STACK_NEW_ADDITION_RELOAD);
>  
>  	assert(ret != REFTABLE_API_ERROR);
>  	return ret;
> @@ -2552,15 +2552,16 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_stack_init_log_iterator(be->stack, &it);
> +	ret = reftable_stack_new_addition(&add, be->stack,
> +					  REFTABLE_STACK_NEW_ADDITION_RELOAD);

>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_iterator_seek_log(&it, refname);
> +	ret = reftable_stack_init_log_iterator(be->stack, &it);
>  	if (ret < 0)
>  		goto done;
>  
> -	ret = reftable_stack_new_addition(&add, be->stack, 0);
> +	ret = reftable_iterator_seek_log(&it, refname);
>  	if (ret < 0)
>  		goto done;
>  
> 
> -- 
> 2.51.0.rc1.163.g2494970778.dirty
>