Message ID | cover.1681384405.git.ps@pks.im (mailing list archive) |
---|---|
Headers | show |
Series | repack: fix geometric repacking with gitalternates | expand |
On 4/13/2023 7:16 AM, Patrick Steinhardt wrote: > Hi, > > this is the third version of my patch series to fix geometric repacking > with repositories that are connected to an alternate object database. > > Changes compared to v2: > > - I've simplified patch 1 to reset the preferred pack index instead > of moving around some of the checks. This also causes us to print > the warning for missing preferred packfiles again. > > - I've fixed the logic in patch 2 to find the preferred packfile to > not return a packfile that would be rolled up in a geometric > repack. > > - I've added an additional patch to split out preexisting tests for > `--stdin-packs` into their own test file. > > - I've changed the unportable test added for geometric repacking > with `-l` that used stat(1) to instead use our `test-tool chmtime` > helper. > > - I've changed the logic that disables writing bitmaps in git-repack > to cover more cases. It now always kicks in when doing a repack > with `-l` that asks for bitmaps when connected to an alternate > object directory. > > - In general, there's a bunch of small improvements left and right > for the tests I'm adding. I didn't get feedback on your v2 in time, but this v3 is very close. There's just the one test change in patch 9 that I think deserves a re-roll, but otherwise this looks good. Thanks, -Stolee
Patrick Steinhardt <ps@pks.im> writes: > Changes compared to v2: > > - I've simplified patch 1 to reset the preferred pack index instead > of moving around some of the checks. This also causes us to print > the warning for missing preferred packfiles again. > > - I've fixed the logic in patch 2 to find the preferred packfile to > not return a packfile that would be rolled up in a geometric > repack. > > - I've added an additional patch to split out preexisting tests for > `--stdin-packs` into their own test file. > > - I've changed the unportable test added for geometric repacking > with `-l` that used stat(1) to instead use our `test-tool chmtime` > helper. > > - I've changed the logic that disables writing bitmaps in git-repack > to cover more cases. It now always kicks in when doing a repack > with `-l` that asks for bitmaps when connected to an alternate > object directory. > > - In general, there's a bunch of small improvements left and right > for the tests I'm adding. > > Thanks for all the feedback so far. Loss of "stat" is very much appreciated, as the ones added in the previous round were the only hits from $ git grep '^[ ]*stat ' t/ ;# leading SP/HT plus "stat " and I suspect among platforms that are not Windows, not Linux, and not macOS, there may be ones that lack the tool. 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. Thanks. [References] *1* https://github.com/git/git/actions/runs/4694670520/jobs/8323047888#step:5:313 *2* https://github.com/git/git/actions/runs/4694155668
On Thu, Apr 13, 2023 at 07:03:48PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: [snip] > 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