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

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

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

Junio C Hamano <gitster@pobox.com> writes:

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

More like we have two functions:
1. compact all tables into one
2. compact based on heuristics

This function oversees logic from both.

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

You're right though, I did manage to extract out the common code and
will send in a new version. Thanks for the push.

- Karthik

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