From: Karthik Nayak Date: Mon, 03 Nov 2025 15:05:51 GMT Subject: Re: [PATCH 1/5] reftable/stack: return stack segments directly Message-ID: In-Reply-To: <7gjrsjgi32akawqwcamzil2rblqelfvgmrxmgef5ssrslntmc6@43cra6zhledc> Justin Tobler 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_' 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?