reftable/stack: use error codes when locking fails during compaction

Compaction of a reftable stack may fail gracefully when there is a
concurrent process that writes to the reftable stack and which has thus
locked either the "tables.list" file or one of the tables. This is
expected and can be handled gracefully by some of the callers which
invoke compaction. Thus, to indicate this situation to our callers, we
return a positive return code from `stack_compact_range()` and bubble it
up to the caller.

This kind of error handling is somewhat awkward though as many callers
in the call chain never even think of handling positive return values.
Thus, the result is either that such errors are swallowed by accident,
or that we abort operations with an unhelpful error message.

Make the code more robust by always using negative error codes when
compaction fails, with `REFTABLE_LOCK_ERROR` for the described benign
error case.

Note that only a single callsite knew to handle positive error codes
gracefully in the first place. Subsequent commits will touch up some of
the other sites to handle those errors better.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

1 files changed, 13 insertions, 5 deletions

diff --git a/reftable/stack.c b/reftable/stack.c
index eaa8bb9c99..79856b6565 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -973,7 +973,15 @@ done:
 	return err;
 }
 
-/* <  0: error. 0 == OK, > 0 attempt failed; could retry. */
+/*
+ * Compact all tables in the range `[first, last)` into a single new table.
+ *
+ * This function returns `0` on success or a code `< 0` on failure. When the
+ * stack or any of the tables in the specified range are already locked then
+ * this function returns `REFTABLE_LOCK_ERROR`. This is a benign error that
+ * callers can either ignore, or they may choose to retry compaction after some
+ * amount of time.
+ */
 static int stack_compact_range(struct reftable_stack *st,
 			       size_t first, size_t last,
 			       struct reftable_log_expiry_config *expiry)
@@ -1003,7 +1011,7 @@ static int stack_compact_range(struct reftable_stack *st,
 					LOCK_NO_DEREF);
 	if (err < 0) {
 		if (errno == EEXIST)
-			err = 1;
+			err = REFTABLE_LOCK_ERROR;
 		else
 			err = REFTABLE_IO_ERROR;
 		goto done;
@@ -1025,7 +1033,7 @@ static int stack_compact_range(struct reftable_stack *st,
 						table_name.buf, LOCK_NO_DEREF);
 		if (err < 0) {
 			if (errno == EEXIST)
-				err = 1;
+				err = REFTABLE_LOCK_ERROR;
 			else
 				err = REFTABLE_IO_ERROR;
 			goto done;
@@ -1075,7 +1083,7 @@ static int stack_compact_range(struct reftable_stack *st,
 					LOCK_NO_DEREF);
 	if (err < 0) {
 		if (errno == EEXIST)
-			err = 1;
+			err = REFTABLE_LOCK_ERROR;
 		else
 			err = REFTABLE_IO_ERROR;
 		goto done;
@@ -1187,7 +1195,7 @@ static int stack_compact_range_stats(struct reftable_stack *st,
 				     struct reftable_log_expiry_config *config)
 {
 	int err = stack_compact_range(st, first, last, config);
-	if (err > 0)
+	if (err == REFTABLE_LOCK_ERROR)
 		st->stats.failures++;
 	return err;
 }