From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 05 Nov 2025 14:11:53 GMT
Subject: Re: [PATCH v2 2/5] reftable/stack: add function to check if optimization is required
Message-ID: <CAOLa=ZRD_zNCnGf3ibU=X04vC8WjxzRVAyg+OwPr1Hf12kSGgA@mail.gmail.com>
In-Reply-To: <xmqqcy5xpmrw.fsf@gitster.g>

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.

Karthik