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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

>> +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.

>> +	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.
>> +
>> +	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]