From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 13 Oct 2025 09:01:15 GMT
Subject: Re: [PATCH 6/9] reftable/stack: return stack segments directly
Message-ID: <CAOLa=ZSfiD4KTSMyc2WW4oC2zyDO9CHmUWwnws3GhCdbtL4rAA@mail.gmail.com>
In-Reply-To: <aOjse9udPEoWqUaO@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Oct 10, 2025 at 12:27:10PM +0200, Karthik Nayak wrote:
>> The `stack_table_sizes_for_compaction()` function returns individual
>> sizes of each reftable table. This function is only called by
>> `reftable_stack_auto_compact()` to process the table sizes.
>
> Maybe: "to decide which tables need to be compacted, if any."
>

Reads better, will update.

>> Modify the function to directly return the segments, this avoids the
>
> s/this/which/
>

will do.

>> extra step of receiving the sizes only to pass it to
>> `suggest_compaction_segment()`.
>>
>> A future commit will also add functionality for checking if
>
> s/if/whether/
>

This should be okay either ways I believe, but shouldn't matter much,
will change.

>
>> auto-compaction is necessary without performing it. This change allows
>> code re-usability in that context.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  reftable/stack.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/reftable/stack.c b/reftable/stack.c
>> index f91ce50bcd..9d9326ce0e 100644
>> --- a/reftable/stack.c
>> +++ b/reftable/stack.c
>> @@ -1639,29 +1640,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
>>
>>  	REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
>>  	if (!sizes)
>> -		return NULL;
>> +		return REFTABLE_OUT_OF_MEMORY_ERROR;
>>
>>  	for (size_t i = 0; i < st->merged->tables_len; i++)
>>  		sizes[i] = st->tables[i]->size - overhead;
>>
>> -	return sizes;
>> +	*seg = suggest_compaction_segment(sizes, st->merged->tables_len,
>> +					  st->opts.auto_compaction_factor);
>> +	reftable_free(sizes);
>> +
>> +	return 0;
>>  }
>>
>>  int reftable_stack_auto_compact(struct reftable_stack *st)
>>  {
>>  	struct segment seg;
>> -	uint64_t *sizes;
>> +	int err = 0;
>
> The initialization is unnecessary.
>

True, but I always think its nicer to define the base state like this.
This makes it less error prone when/if the code is modified in the
future. But doesn't really matter, so let me remove it.

> Other than that this patch looks good to me.
>
> Patrick

Thanks for the review.