Re: [PATCH 2/5] reftable/stack: add function to check if optimization is required

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 3, 2025, 15:51 UTC
Message-ID: <CAOLa=ZRzLviMkc8C8617L48NwJPvi7F1Qsozezm9gUQ0_dRU4A@mail.gmail.com>
In-Reply-To: <tdgxvocyp2armupgbti2wnbjphdvidooddbdyrynmdokjgqr3o@tzrbu5lcgipt>

Justin Tobler <jltobler@gmail.com> writes:

Show 17 quoted lines

>> +int reftable_stack_compaction_required(struct reftable_stack *st,
>> +				       bool use_heuristics,
>> +				       bool *required)
>> +{
>> +	struct segment seg;
>> +	int err = 0;
>> +
>> +	if (st->merged->tables_len < 2) {
>> +		*required = false;
>> +		return 0;
>> +	}
>
> Both `reftable_stack_auto_compact()` and `suggest_compaction_segement()`
> already check if the stack has less than two tables. I wonder if we can
> avoid having multiple of these checks by instead having a single one at
> the start of `stack_segements_for_compaction()`?
>

Well we can't for two reasons:
1. We want to perform this check independent of whether `use_heuristics`
   is set or not.
2. Currently `stack_segements_for_compaction()` does one thing only,
   which is stack the segments. I wouldn't want to introduce another
   responsibility to it.

Show 8 quoted lines

>> +	if (!use_heuristics) {
>> +		*required = true;
>> +		return 0;
>> +	}
>
> Is there a reason we would want to skip validating the geometric
> sequence and just assume it compaction is required?
>

This is the difference between running 'git refs optimize' with and without '--auto'. With '--auto' we will use heuristics to do a geometric progression. Without, we simply compact all tables into one.

So we need to support both modes here.

Show 18 quoted lines

>> +
>> +	err = stack_segments_for_compaction(st, &seg);
>> +	if (err)
>> +		return err;
>> +
>> +	*required = segment_size(&seg) > 0;
>
> As mentioned on the previous patch, I wonder if we could just return the
> number of tables in the compaction segment as part of
> `stack_segments_for_compaction()`. A negative value could indicate an
> error. All other values would reflect the number of tables to be
> compacted.
>
> This way callers interested in whether compaction should be performed
> could just do: stack_segments_for_compaction > 0. We could maybe avoid
> having a separate function like we do here and just expose
> `stack_segments_for_compaction()`.
>

We'd still need to expose a new function as `stack_segments_for_compaction()` is still internal details to the reftable backend, which we wouldn't want to expose externally. Users of this function, should only need to know a boolean value wether the backend needs to be optimized or not.