Re: [PATCH v3 09/10] pack-revindex: ensure that on-disk reverse indexes are given precedence

From: Jeff King <peff@peff.net>
Date: Jan 29, 2021, 00:53 UTC
Message-ID: <YBNcjmn5Jm/EmHwS@coredump.intra.peff.net>
In-Reply-To: <a66d2f9f7c20eeb813656e66b3ad9d42f2eecf34.1611617820.git.me@ttaylorr.com>

On Mon, Jan 25, 2021 at 06:37:46PM -0500, Taylor Blau wrote:

Show 12 quoted lines

> When an on-disk reverse index exists, there is no need to generate one
> in memory. In fact, doing so can be slow, and require large amounts of
> the heap.
> 
> Let's make sure that we treat the on-disk reverse index with precedence
> (i.e., that when it exists, we don't bother trying to generate an
> equivalent one in memory) by teaching Git how to conditionally die()
> when generating a reverse index in memory.
> 
> Then, add a test to ensure that when (a) an on-disk reverse index
> exists, and (b) when setting GIT_TEST_REV_INDEX_DIE_IN_MEMORY, that we
> do not die, implying that we read from the on-disk one.

I don't love this kind of hackery, as it will have to live forever if we want to keep testing this feature. But I also don't know how else to tell in the regular test suite that we are avoiding the slow path.

Would it be enough to instead add a t/perf test which checks the speed of:

  echo HEAD | git cat-file --batch-check='%(objectsize:disk)'

? I hate relying on the perf suite, because it gets run way less often (and requires a lot of squinting to interpret the results). But it wouldn't require any extra code the binary, as it's observing the actual user-visible thing we care about: speed.

(I guess we care as much or more about peak heap usage, but measuring that is a whole other can of worms).

-Peff