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:

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

>  int reftable_stack_auto_compact(struct reftable_stack *st)
>  {
>  	struct segment seg;
> diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c
> index a8b91812e8..b8110cdeee 100644
> --- a/t/unit-tests/u-reftable-stack.c
> +++ b/t/unit-tests/u-reftable-stack.c
> @@ -1067,6 +1067,7 @@ void test_reftable_stack__add_performs_auto_compaction(void)
>  			.value_type = REFTABLE_REF_SYMREF,
>  			.value.symref = (char *) "master",
>  		};
> +		bool required = false;
>  		char buf[128];
>  
>  		/*
> @@ -1087,10 +1088,17 @@ void test_reftable_stack__add_performs_auto_compaction(void)
>  		 * auto compaction is disabled. When enabled, we should merge
>  		 * all tables in the stack.
>  		 */
> -		if (i != n)
> +		cl_assert_equal_i(reftable_stack_compaction_required(st, true, &required), 0);
> +		if (i != n) {
>  			cl_assert_equal_i(st->merged->tables_len, i + 1);
> -		else
> +			if (i < 1)
> +				cl_assert_equal_b(required, false);
> +			else
> +				cl_assert_equal_b(required, true);
> +		} else {
>  			cl_assert_equal_i(st->merged->tables_len, 1);
> +			cl_assert_equal_b(required, false);
> +		}
>  	}
>  
>  	reftable_stack_destroy(st);