Re: [PATCH v2 2/5] reftable/stack: add function to check if optimization is required
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 4, 2025, 20:26 UTC
- Message-ID
- <xmqqcy5xpmrw.fsf@gitster.g>
- In-Reply-To
- <20251104-562-add-sub-command-to-check-if-maintenance-is-needed-v2-2-303462a9e4ed@gmail.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.
Show 24 quoted lines
> +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?
Show 35 quoted lines
> 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);