Re: [PATCH v2 6/6] reftable/stack: handle outdated stacks when compacting

> When we compact the reftable stack we first acquire the lock for the
> "tables.list" file and then reload the stack to check that it is still
> up-to-date. This is done by calling `stack_uptodate()`, which knows to
> return zero in case the stack is up-to-date, a positive value if it is
> not and a negative error code on unexpected conditions.

> We don't do proper error checking though, but instead we only check
> whether the returned error code is non-zero. If so, we simply bubble it
> up the calling stack, which means that callers may see an unexpected
> positive value.
> 
> Fix this issue by translating to `REFTABLE_OUTDATED_ERROR` instead.
> Handle this situation in `reftable_addition_commit()`, where we perform
> a best-effort auto-compaction.
> 
> All other callsites of `stack_uptodate()` know to handle a positive
> return value and thus don't need to be fixed.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index f77d7f58e8..effa2fc8cb 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -579,9 +579,11 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,
>  	return err;
>  }
>  
> -/* -1 = error
> - 0 = up to date
> - 1 = changed. */
> +/*
> + * Check whether the given stack is up-to-date with what we have in memory.
> + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code
> + * otherwise.
> + */
>  static int stack_uptodate(struct reftable_stack *st)
>  {
>  	char **names = NULL;
> @@ -849,10 +851,13 @@ int reftable_addition_commit(struct reftable_addition *add)
>  		 * control. It is possible that a concurrent writer is already
>  		 * trying to compact parts of the stack, which would lead to a
>  		 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked
> -		 * already. This is a benign error though, so we ignore it.
> +		 * already. Similarly, the stack may have been rewritten by a
> +		 * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`.
> +		 * Both of these errors are benign, so we simply ignore them.
>  		 */
>  		err = reftable_stack_auto_compact(add->stack);
> -		if (err < 0 && err != REFTABLE_LOCK_ERROR)
> +		if (err < 0 && err != REFTABLE_LOCK_ERROR &&
> +		    err != REFTABLE_OUTDATED_ERROR)
>  			goto done;
>  		err = 0;
>  	}
> @@ -1215,9 +1220,24 @@ static int stack_compact_range(struct reftable_stack *st,
>  		goto done;
>  	}
>  
> +	/*
> +	 * Check whether the stack is up-to-date. We unfortunately cannot
> +	 * handle the situation gracefully in case it's _not_ up-to-date
> +	 * because the range of tables that the user has requested us to
> +	 * compact may have been changed. So instead we abort.
> +	 *
> +	 * We could in theory improve the situation by having the caller not
> +	 * pass in a range, but instead the list of tables to compact. If so,
> +	 * we could check that relevant tables still exist. But for now it's
> +	 * good enough to just abort.
> +	 */
>  	err = stack_uptodate(st);
> -	if (err)
> +	if (err < 0)
>  		goto done;
> +	if (err > 0) {
> +		err = REFTABLE_OUTDATED_ERROR;
> +		goto done;
> +	}

>  
>  	/*
>  	 * Lock all tables in the user-provided range. This is the slice of our
> 
> -- 
> 2.50.1.723.g3e08bea96f.dirty
> 
>