From: Karthik Nayak Date: Thu, 06 Nov 2025 08:18:51 GMT Subject: Re: [PATCH v2 2/5] reftable/stack: add function to check if optimization is required Message-ID: In-Reply-To: Junio C Hamano writes: > Karthik Nayak writes: > >> Junio C Hamano writes: >> >>> Karthik Nayak 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