On Tue, Apr 02, 2024 at 11:37:10AM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> writes: > > > There are a handful of related test breakages which are found when > > running t/t7700-repack.sh with GIT_TEST_MULTI_PACK_INDEX set to "1" in > > your environment. > > > > Both test failures are the result of something like: > > > > git repack --write-midx --write-bitmap-index [...] && > > > > test_path_is_file $midx && > > test_path_is_file $midx-$(midx_checksum $objdir).bitmap > > > > , where we repack instructing Git to write a new MIDX and corresponding > > MIDX bitamp. > > > > The error occurs when GIT_TEST_MULTI_PACK_INDEX=1 is found in the > > enviornment. This causes Git to write out a second MIDX (after > > processing the builtin's `--write-midx` argument) which is identical to > > the first, but does not request a bitmap (since we did not set the > > GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP variable in the environment). > > Doesn't it sound more like a bug, though? If a command line option > requests something, should we still be honoring a contradicting > instruction given by environment variable(s)? > > But anyway. I have generally considered the `--write-midx` and `GIT_TEST_MULTI_PACK_INDEX` options to be orthogonal to each other. The latter is a developer-oriented option that forces Git to write a MIDX post-repack regardless of the command-line option. It predates the `--write-midx` option by a number of years, and IIUC was introduced to enhance test coverage while the MIDX was being originally developed. I would argue that GIT_TEST_MULTI_PACK_INDEX should be on the list of GIT_TEST_-variables to get rid of as it has served its purpose. > > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh > > index 94f9f4a1da..127efe99f8 100755 > > --- a/t/t7700-repack.sh > > +++ b/t/t7700-repack.sh > > @@ -629,6 +629,7 @@ test_expect_success '--write-midx with preferred bitmap tips' ' > > git log --format="create refs/tags/%s/%s %H" HEAD >refs && > > git update-ref --stdin <refs && > > > > + GIT_TEST_MULTI_PACK_INDEX=0 \ > > git repack --write-midx --write-bitmap-index && > > test_path_is_file $midx && > > test_path_is_file $midx-$(midx_checksum $objdir).bitmap && > > Is it a viable alternative approach to skip this check (and the > other one) when GIT_TEST_MULTI_PACK_INDEX is set (i.e., lazy > prereq). It will give us a better documentation value, e.g., > > test_lazy_prereq FORCED_MIDX ' > # Features that are broken when GIT_TEST_* forces it > # to enable are protexted with this prerequisite. > test "$GIT_TEST_MULTI_PACK_INDEX" = 1; > ' > > test_expect_success !FORCED_MIDX '--write-midx with ...' ' > ... > ' > > With a single comment, we can annotate any future tests that relies > on features working correctly even with GIT_TEST_MULTI_PACK_INDEX. It's possible, but not something that I have seen done elsewhere for this or any of the other GIT_TEST- variables. Like I said, I'd like to get rid of this (and many other) GIT_TEST-related variables, but that is a larger effort than this single patch. Thanks, Taylor
