Re: [PATCH 1/5] reftable/stack: return stack segments directly

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

[snip]

>>
>>  	if (segment_size(&seg) > 0)
>>  		return stack_compact_range(st, seg.start, seg.end - 1,
>
> Do we expect the errors returned by `stack_segments_for_compaction()` to
> always be negative? If so, I wonder if we should also have it return the
> number of tables in the segment. That way it could also handle the
> followup `segment_size()`.
>

Currently yes, since all 'REFTABLE_<error>' errors return negative
value. But I must say I'm not a fan of combining errors and values
together in a single return. This only creates confusion.

I'm not sure removing `segment_size()` is also a good idea, because it
describes what the check is. Otherwise we're looking at something like:

@@ -1655,11 +1646,10 @@ int reftable_stack_auto_compact(struct
reftable_stack *st)
 	if (st->merged->tables_len < 2)
 		return 0;

-	err = stack_segments_for_compaction(st, &seg);
-	if (err)
+	err_or_stack_size = stack_segments_for_compaction(st, &seg);
+	if (err_or_stack_size < 0)
 		return err;
-
-	if (segment_size(&seg) > 0)
+	else if (err_or_stack_size > 0)
 		return stack_compact_range(st, seg.start, seg.end - 1,
 					   NULL, STACK_COMPACT_RANGE_BEST_EFFORT);

I'm not sure that this would be better? Or am I missing something?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]