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

[Justin Tobler <jltobler@gmail.com>]

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

That's fair. From my understanding, `stack_segements_for_compaction()`
populates a segment which defines the range of tables that should be
compacted to restore the geometric sequence. Since we want to ultimately
know whether compaction needs to occur, my thought process was we could
maybe have a single function ("check_compaction_needed()") that
effectively returns a boolean and maybe be able to reuse that. I don't
think it matters much though and as you mention we also want to consider
`use_heuristics`.

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

That's for the clarification. So without --auto, instead of following a
geometric sequence, a different maintenance strategy is used and we
compact all the tables into one. Makes sense.

-Justin