Re: [PATCH 0/7] pack-revindex: enable on-disk reverse indexes by default

[Derrick Stolee <derrickstolee@github.com>]

On 4/10/2023 6:53 PM, Taylor Blau wrote:
> In the vast majority of cases, this trade-off favors the on-disk ".rev"
> files. But in certain cases, the in-memory variant performs more
> favorably. Since these cases are narrow, and performance is machine- and
> repository-dependent, this series also introduces a new configuration
> option to disable reading ".rev" files in the third commit.

I agree that the performance trade-off indicates that having the .rev
files is preferred. It makes operations that _can_ be very fast as fast
as possible (inspecting a small number of objects is much faster because
we don't generate the in-memory index in O(N) time) and you create a knob
for disabling it in the case that we are already doing something that
inspects almost-all objects.
 
> The series is structured as follows:
> 
>   - A couple of cleanup patches to plug a leak in stage_tmp_packfiles().
>   - Three patches to enable `pack.readReverseIndex`.
>   - Another patch to change the default of `pack.writeReverseIndex` from
>     "false" to "true".
>   - A final patch to enable the test suite to be run in a mode that does
>     not use on-disk ".rev" files.

This was an easy series to read. I applied the patches locally and poked
around in the resulting code as I went along. This led to a couple places
where I recommend a few changes, including a new patch that wires
repository pointers through a few more method layers.

Thanks,
-Stolee