Patrick Steinhardt <ps@xxxxxx> writes:
> Whenever we commit a new table to the reftable stack we will end up
> invoking auto-compaction of the stack to keep the total number of tables
> at bay. This auto-compaction may fail though in case at least one of the
> tables which we are about to compact is locked. This is indicated by the
> compaction function returning a positive value. We do not handle this
We no longer return a positive value, do we?
> case though, and thus bubble that return value up the calling chain,
> which will ultimately cause a failure.
>
> Fix this bug by handling positive return values.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/stack.c | 13 ++++++++++++-
> t/t0610-reftable-basics.sh | 20 ++++++++++++++++++++
> 2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index ba15c48ddd..dd5160d751 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -680,8 +680,19 @@ int reftable_addition_commit(struct reftable_addition *add)
> if (err)
> goto done;
>
> - if (!add->stack->disable_auto_compact)
> + if (!add->stack->disable_auto_compact) {
> + /*
> + * Auto-compact the stack to keep the number of tables in
> + * control. Note that we explicitly ignore locking issues which
> + * may indicate that a concurrent process is already trying to
> + * compact tables. This is fine, so we simply ignore that error
> + * condition.
> + */
>
Nit: The last two sentences are somewhat the same, it'd be perhaps nicer
to explain why "this is fine".
> err = reftable_stack_auto_compact(add->stack);
> + if (err < 0 && err != REFTABLE_LOCK_ERROR)
> + goto done;
> + err = 0;
> + }
>
> done:
> reftable_addition_close(add);
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index 686781192e..5f2f9baa9b 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -340,6 +340,26 @@ test_expect_success 'ref transaction: empty transaction in empty repo' '
> EOF
> '
>
> +test_expect_success 'ref transaction: fails gracefully when auto compaction fails' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + test_commit A &&
> + for i in $(test_seq 10)
> + do
> + git branch branch-$i &&
> + for table in .git/reftable/*.ref
> + do
> + touch "$table.lock" || exit 1
> + done ||
> + exit 1
> + done &&
> + test_line_count = 13 .git/reftable/tables.list
> + )
> +'
I'm not sure about the testing setup for reftables, but maybe if we
moved this to the unit tests, we could've also checked the
`reftable_stack_compaction_stats(st)->failures` value. This is fine, but
it doesn't really tell us if the compaction was even attempted.
Attachment:
signature.asc
Description: PGP signature
