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

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

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

You mean you already have two duplicate implementations whose
definition of "when should we compact?" can drift apart over time
(worse, they may already be subtly different), and you are adding
yet another one?

Instead of describing such an insanity in the commit message, can we
refactor to have a single central logic that is used from three
places?

Thanks.