Re: [PATCH v2 17/19] t: retire 'GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP'

From: Jeff King <peff@peff.net>
Date: Aug 1, 2024, 10:46 UTC
Message-ID: <20240801104647.GQ1159276@coredump.intra.peff.net>
In-Reply-To: <23912425bf7c0106ed388f7712e7bb822572fe5d.1721250704.git.me@ttaylorr.com>

On Wed, Jul 17, 2024 at 05:12:47PM -0400, Taylor Blau wrote:

Show 13 quoted lines

> Two years ago, commit ff1e653c8e2 (midx: respect
> 'GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP', 2021-08-31) introduced a new
> environment variable which caused the test suite to write MIDX bitmaps
> after any 'git repack' invocation.
> 
> At the time, this was done to help flush out any bugs with MIDX bitmaps
> that weren't explicitly covered in the t5326-multi-pack-bitmap.sh
> script.
> 
> Two years later, that flag has served us well and is no longer providing
> meaningful coverage, as the script in t5326 has matured substantially
> and covers many more interesting cases than it did back when ff1e653c8e2
> was originally written.

I do think it could be providing some value still, just because other scripts may create unusual setups that will exercise the code in different ways. That said...

Show 7 quoted lines

> Remove the 'GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP' environment variable
> as it is no longer serving a useful purpose. More importantly, removing
> this variable clears the way for us to introduce a new one to help
> similarly flush out bugs related to incremental MIDX chains.
> 
> Because these incremental MIDX chains are (for now) incompatible with
> MIDX bitmaps, we cannot have both.

...if it is one or the other, I think it is better to test the new code.

And I do think that midx bitmap code is less likely to be exercised in interesting ways by random parts of the test suite (versus something like GIT_TEST_DEFAULT_HASH, whose effects are pervasive). So I think this is a good tradeoff.

Show 11 quoted lines

>  builtin/repack.c                  | 12 ++----------
>  ci/run-build-and-tests.sh         |  1 -
>  midx.h                            |  2 --
>  t/README                          |  4 ----
>  t/t0410-partial-clone.sh          |  2 --
>  t/t5310-pack-bitmaps.sh           |  4 ----
>  t/t5319-multi-pack-index.sh       |  3 +--
>  t/t5326-multi-pack-bitmaps.sh     |  3 +--
>  t/t5327-multi-pack-bitmaps-rev.sh |  5 ++---
>  t/t7700-repack.sh                 | 21 +++++++--------------
>  10 files changed, 13 insertions(+), 44 deletions(-)

Patch looks good.

-Peff