reftable/stack: handle outdated stacks when compacting

Git fork

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Patrick Steinhardt and committed by

Junio C Hamano 7 months ago 54d25de3 178c5885

+26 -6

1 changed file

expand all

unified split

reftable

stack.c

+26 -6

reftable/stack.c

··· 579 579 return err; 580 580 } 581 581 582 - /* -1 = error 583 - 0 = up to date 584 - 1 = changed. */ 582 + /* 583 + * Check whether the given stack is up-to-date with what we have in memory. 584 + * Returns 0 if so, 1 if the stack is out-of-date or a negative error code 585 + * otherwise. 586 + */ 585 587 static int stack_uptodate(struct reftable_stack *st) 586 588 { 587 589 char **names = NULL; ··· 850 852 * control. It is possible that a concurrent writer is already 851 853 * trying to compact parts of the stack, which would lead to a 852 854 * `REFTABLE_LOCK_ERROR` because parts of the stack are locked 853 - * already. This is a benign error though, so we ignore it. 855 + * already. Similarly, the stack may have been rewritten by a 856 + * concurrent writer, which causes `REFTABLE_OUTDATED_ERROR`. 857 + * Both of these errors are benign, so we simply ignore them. 854 858 */ 855 859 err = reftable_stack_auto_compact(add->stack); 856 - if (err < 0 && err != REFTABLE_LOCK_ERROR) 860 + if (err < 0 && err != REFTABLE_LOCK_ERROR && 861 + err != REFTABLE_OUTDATED_ERROR) 857 862 goto done; 858 863 err = 0; 859 864 } ··· 1215 1220 goto done; 1216 1221 } 1217 1222 1223 + /* 1224 + * Check whether the stack is up-to-date. We unfortunately cannot 1225 + * handle the situation gracefully in case it's _not_ up-to-date 1226 + * because the range of tables that the user has requested us to 1227 + * compact may have been changed. So instead we abort. 1228 + * 1229 + * We could in theory improve the situation by having the caller not 1230 + * pass in a range, but instead the list of tables to compact. If so, 1231 + * we could check that relevant tables still exist. But for now it's 1232 + * good enough to just abort. 1233 + */ 1218 1234 err = stack_uptodate(st); 1219 - if (err) 1235 + if (err < 0) 1220 1236 goto done; 1237 + if (err > 0) { 1238 + err = REFTABLE_OUTDATED_ERROR; 1239 + goto done; 1240 + } 1221 1241 1222 1242 /* 1223 1243 * Lock all tables in the user-provided range. This is the slice of our