From: Justin Tobler <jltobler@gmail.com>
Date: Mon, 03 Nov 2025 18:03:27 GMT
Subject: Re: [PATCH 1/5] reftable/stack: return stack segments directly
Message-ID: <2nr5ig2cg5bc2zvtinm4p2fxssuim5kb4bsflrx3xnos2pwkk3@tya7zuj4pgg6>
In-Reply-To: <CAOLa=ZQa21A+fF=ukZMmx3zu1DrMFU-EcZGrZConS-L16+ih1A@mail.gmail.com>

On 25/11/03 07:05AM, Karthik Nayak wrote:
> 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?

That's fair. I was thinking that we could just make
`stack_segments_for_compaction()` responsible for the boolean check of
whether compaction is required or not. It's probably not worth
overloading the return value though.

-Justin