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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 3, 2025, 15:05 UTC
Message-ID: <CAOLa=ZQa21A+fF=ukZMmx3zu1DrMFU-EcZGrZConS-L16+ih1A@mail.gmail.com>
In-Reply-To: <7gjrsjgi32akawqwcamzil2rblqelfvgmrxmgef5ssrslntmc6@43cra6zhledc>

Justin Tobler <jltobler@gmail.com> writes:

[snip]

Show 9 quoted lines

>>
>>  	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?