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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> The reftable backend performs auto-compaction as part of its regular
>>> flow, which is required to keep the number of tables part of a stack at
>>> bay. This allows it to stay optimized.
>>
>> Sounds very sensible.
>>
>>> Compaction can also be triggered voluntarily by the user via the 'git
>>> pack-refs' or the 'git refs optimize' command. However, currently there
>>> is no way for the user to check if optimization is required without
>>> actually performing it.
>>
>> Sounds very sensible goal.
>>
>> But where is the existing logic to decide when it needs to
>> auto-compact, performed as part of its regular flow?
>>
>> After reading "the reftable machinery already decides when it needs
>> to compact and does so" plus "but the logic to decide is not made
>> available to users", I would have expected for this patch to extract
>> such an existing logic or otherwise make it available to new callers
>> so that things like "gc --auto" can call it, but the diffstat shows
>> mostly additions, which does not give readers any confidence in the
>> new function that answers "do we need compaction?".  It would give
>> _an_ answer, but there is no clue if the answer it gives is the same
>> answer as the existing logic that decides when to compact as part of
>> the regular operation.
>>
>> I am puzzled.
>>
>>> +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;
>>> +	}
>>> +
>>> +	if (!use_heuristics) {
>>> +		*required = true;
>>> +		return 0;
>>> +	}
>>> +
>>> +	err = stack_segments_for_compaction(st, &seg);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	*required = segment_size(&seg) > 0;
>>> +	return 0;
>>> +}
>>
>> Specifically, where is the above logic come from?  Is it duplicating
>> an existing logic but that code is hard to separate out into this
>> helper?
>>
>
> Good question.
>
> Most of this logic is already part of 'reftable_stack_auto_compact()'.
> We also have another similar function 'reftable_stack_compact_all()'.
> The former is used for compaction based on heuristics and the latter is
> for compacting all tables into one. In the refs subsystem usage of
> heuristics is denoted by the usage of the 'REFS_OPTIMIZE_AUTO' flag.
>
> The function we're introducing allows users to explicitly mention if
> they want to use heuristics or not. This allows us to differentiate
> between the two modes. The result of which is that this uses intertwined
> logic of the two existing functions. Hence we can't extract any code out.
>
> I'll add this information in the commit message.