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. Thanks.
