On Sat, Dec 21, 2024 at 09:51:34AM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > Patrick Steinhardt <ps@xxxxxx> writes: > > > >> In order to compact tables we need at least two tables. Bail out early > >> from `reftable_stack_auto_compact()` in case we have less than two > >> tables. > > > > While that is very true, bailing out on "< 2" would change the > > behaviour. Where there were only one table, earlier we still went > > ahead and exercised compaction code path, but now we no longer do. > > The end result of having just a single table might logically be the > > same with this change, but if we were relying on some side effects > > of exercising the compaction code path, do we know that the rest of > > the code is OK? > > > > That's the kind of questions I would ask, if this were somebody who > > hasn't been deeply involved in the reftable code and came this deep > > in the pre-release period. But since we all know you have been the > > main driver for this effort, we'd take your word for it ;-) > > > > Thanks, will queue. > > And with code inspection, it can trivially seen that this change is > perfectly fine. > > In the original, when "== 1", stack_table_sizes_for_compaction(st) > yields an array with a single element, suggest_compaction_segment() > gives back segment with .start and .end both set to 0, for which > segment_size() returns 0 hence we do not call stack_compact_range(). > The original code happens to do the same when "== 0" (and 0-sized > allocation does not give back NULL). > > So bypassing all of these when "== 1" is a no-op change, an > optimization to avoid allocating a single-element array and then > immediately freeing it. Doing so when "== 0" is a strict > improvement on a platform with malloc that returns NULL for 0-sized > allocation. So bypassing when "< 2" is totally safe and justifyable > change. Indeed. I'll include an explanation in v2 of this patch series. Patrick
