Re: [PATCH v3 00/10] repack: fix geometric repacking with gitalternates

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 14, 2023, 05:42 UTC
Message-ID: <ZDjnvgFyDZb9VZ8u@ncase>
In-Reply-To: <xmqqv8hzusmz.fsf@gitster.g>

On Thu, Apr 13, 2023 at 07:03:48PM -0700, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 8 quoted lines

> With a few fix-ups to the test script I sent to the thread for the
> review of [09/10] squashed in, this topic however seems to cause the
> linux-TEST-vars job fail when queued at the tip of 'seen' (bbfd3bf)
> [*1*].  When 'seen' is rewound by one merge to exclude this topic,
> the CI runs OK (c35f3cd)[*2*].  I dug only far enough to identify
> which topic among 40+ ones was causing the CI breakage.  Perhaps the
> code is operating correctly but the expectation in the test needs
> updating?  I dunno.

The issue is that linux-TEST-vars sets a bunch of environment variables that change the way git-repack(1) operates. Important in this context are GIT_TEST_MULTI_PACK_INDEX and GIT_TEST_MULTI_PACK_INDEX_GENERATE_BITMAP as they change the assumptions that we're doing in our test. What is interesting is that these cause us to write a multi-pack-index and in fact a bitmap, but the warning that we're testing for is not printed. And that's because of the following code snippet:

```
if (write_bitmaps < 0) {
        if (!write_midx &&
            (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))
                write_bitmaps = 0;
} else if (write_bitmaps &&
           git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0) &&
           git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP, 0)) {
        write_bitmaps = 0;
}
```

So if either of those environment variables is set we'll disable generation of bitmaps in git-repack(1) in favor of enabling them in git-multi-pack-index(1). And consequentially we don't see the expected error message.

Anyway. I think what we should do here is to simply override the mechanism with GIT_TEST_MULTI_PACK_INDEX=0. Other tests in this file do the same already, and it does make sense as we explicitly want to test the non-multi-pack-index scenario. And given that we already added a test for the with-multi-pack-index scenario in the same commit we don't really loose any test coverage there.

Patrick