{"thread":{"id":"61254","subject":"[PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","startedAt":"2024-04-02T16:26:41Z","lastAt":"2024-04-02T22:11:02Z","messageCount":7,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492063","messageId":"7e8d435d58eea19d2aae0be366720f5956d29a5d.1712075189.git.me@ttaylorr.com","threadId":"61254","inReplyTo":null,"subject":"[PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-02T16:26:34Z","receivedAt":"2024-04-02T16:26:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"There are a handful of related test breakages which are found when\nrunning t/t7700-repack.sh with GIT_TEST_MULTI_PACK_INDEX set to \"1\" in\nyour environment.\n\nBoth test failures are the result of something like:\n\n git repack --write-midx --write-bitmap-index [...] &&\n\n test_path_is_file $midx &&\n test_path_is_file $midx-$(midx_checksum $objdir).bitmap\n\n, where we repack instructing Git to write a new MIDX and corresponding\nMIDX bitamp.\n\nThe error occurs when GIT_TEST_MULTI_PACK_INDEX=1 is found in the\nenviornment. This causes Git to write out a second MIDX (after\nprocessing the builtin's `--write-midx` argument) which is identical to\nthe first, but does not request a bitmap (since we did not set the\nGIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP variable in the environment).\n\nSince c528e179662 (pack-bitmap: write multi-pack bitmaps, 2021-08-31),\nthe MIDX machinery will drop an existing MIDX bitmap when rewriting an\nidentical MIDX which does not itself request a corresponding bitmap,\nwhich is similar to the way repack itself behaves in the pack-bitmap\ncase.\n\nCorrect these issues (which date back to [1] and [2], respectively) by\nexplicitly setting GIT_TEST_MULTI_PACK_INDEX to zero before running each\ncommand.\n\nIn the future, we should consider removing GIT_TEST_MULTI_PACK_INDEX,\nand in general clean up unused GIT_TEST_-variables. But that is a larger\neffort, and this ensures that we can cleanly run:\n\n $ GIT_TEST_MULTI_PACK_INDEX=1 make test\n\nin the meantime.\n\n[1]: 324efc90d1b (builtin/repack.c: pass `--refs-snapshot` when writing\n bitmaps, 2021-10-01)\n\n[2]: 197443e80ab (repack: don't remove .keep packs with\n `--pack-kept-objects`, 2022-10-17).\n\nSigned-off-by: Taylor Blau \n---\n t/t7700-repack.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 94f9f4a1da..127efe99f8 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -629,6 +629,7 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n \t\tgit log --format=\"create refs/tags/%s/%s %H\" HEAD >refs &&\n \t\tgit update-ref --stdin writes:\n\n> There are a handful of related test breakages which are found when\n> running t/t7700-repack.sh with GIT_TEST_MULTI_PACK_INDEX set to \"1\" in\n> your environment.\n>\n> Both test failures are the result of something like:\n>\n> git repack --write-midx --write-bitmap-index [...] &&\n>\n> test_path_is_file $midx &&\n> test_path_is_file $midx-$(midx_checksum $objdir).bitmap\n>\n> , where we repack instructing Git to write a new MIDX and corresponding\n> MIDX bitamp.\n>\n> The error occurs when GIT_TEST_MULTI_PACK_INDEX=1 is found in the\n> enviornment. This causes Git to write out a second MIDX (after\n> processing the builtin's `--write-midx` argument) which is identical to\n> the first, but does not request a bitmap (since we did not set the\n> GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP variable in the environment).\n\nDoesn't it sound more like a bug, though? If a command line option\nrequests something, should we still be honoring a contradicting\ninstruction given by environment variable(s)?\n\nBut anyway.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 94f9f4a1da..127efe99f8 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -629,6 +629,7 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n> \t\tgit log --format=\"create refs/tags/%s/%s %H\" HEAD >refs &&\n> \t\tgit update-ref --stdin \n> +\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n> \t\tgit repack --write-midx --write-bitmap-index &&\n> \t\ttest_path_is_file $midx &&\n> \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n\nIs it a viable alternative approach to skip this check (and the\nother one) when GIT_TEST_MULTI_PACK_INDEX is set (i.e., lazy\nprereq). It will give us a better documentation value, e.g.,\n\n\ttest_lazy_prereq FORCED_MIDX '\n # Features that are broken when GIT_TEST_* forces it\n # to enable are protexted with this prerequisite.\n\t\ttest \"$GIT_TEST_MULTI_PACK_INDEX\" = 1;\n\t'\n\n\ttest_expect_success !FORCED_MIDX '--write-midx with ...' '\n\t\t...\n\t'\n\nWith a single comment, we can annotate any future tests that relies\non features working correctly even with GIT_TEST_MULTI_PACK_INDEX.\n"},{"id":"492099","messageId":"ZgxSSKGdAicfVhGA@nand.local","threadId":"61254","inReplyTo":"xmqqr0fn4nmx.fsf@gitster.g","subject":"Re: [PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-02T18:45:28Z","receivedAt":"2024-04-02T18:45:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 02, 2024 at 11:37:10AM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > There are a handful of related test breakages which are found when\n> > running t/t7700-repack.sh with GIT_TEST_MULTI_PACK_INDEX set to \"1\" in\n> > your environment.\n> >\n> > Both test failures are the result of something like:\n> >\n> > git repack --write-midx --write-bitmap-index [...] &&\n> >\n> > test_path_is_file $midx &&\n> > test_path_is_file $midx-$(midx_checksum $objdir).bitmap\n> >\n> > , where we repack instructing Git to write a new MIDX and corresponding\n> > MIDX bitamp.\n> >\n> > The error occurs when GIT_TEST_MULTI_PACK_INDEX=1 is found in the\n> > enviornment. This causes Git to write out a second MIDX (after\n> > processing the builtin's `--write-midx` argument) which is identical to\n> > the first, but does not request a bitmap (since we did not set the\n> > GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP variable in the environment).\n>\n> Doesn't it sound more like a bug, though? If a command line option\n> requests something, should we still be honoring a contradicting\n> instruction given by environment variable(s)?\n>\n> But anyway.\n\nI have generally considered the `--write-midx` and\n`GIT_TEST_MULTI_PACK_INDEX` options to be orthogonal to each other. The\nlatter is a developer-oriented option that forces Git to write a MIDX\npost-repack regardless of the command-line option.\n\nIt predates the `--write-midx` option by a number of years, and IIUC was\nintroduced to enhance test coverage while the MIDX was being originally\ndeveloped.\n\nI would argue that GIT_TEST_MULTI_PACK_INDEX should be on the list of\nGIT_TEST_-variables to get rid of as it has served its purpose.\n\n> > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> > index 94f9f4a1da..127efe99f8 100755\n> > --- a/t/t7700-repack.sh\n> > +++ b/t/t7700-repack.sh\n> > @@ -629,6 +629,7 @@ test_expect_success '--write-midx with preferred bitmap tips' '\n> > \t\tgit log --format=\"create refs/tags/%s/%s %H\" HEAD >refs &&\n> > \t\tgit update-ref --stdin >\n> > +\t\tGIT_TEST_MULTI_PACK_INDEX=0 \\\n> > \t\tgit repack --write-midx --write-bitmap-index &&\n> > \t\ttest_path_is_file $midx &&\n> > \t\ttest_path_is_file $midx-$(midx_checksum $objdir).bitmap &&\n>\n> Is it a viable alternative approach to skip this check (and the\n> other one) when GIT_TEST_MULTI_PACK_INDEX is set (i.e., lazy\n> prereq). It will give us a better documentation value, e.g.,\n>\n> \ttest_lazy_prereq FORCED_MIDX '\n> # Features that are broken when GIT_TEST_* forces it\n> # to enable are protexted with this prerequisite.\n> \t\ttest \"$GIT_TEST_MULTI_PACK_INDEX\" = 1;\n> \t'\n>\n> \ttest_expect_success !FORCED_MIDX '--write-midx with ...' '\n> \t\t...\n> \t'\n>\n> With a single comment, we can annotate any future tests that relies\n> on features working correctly even with GIT_TEST_MULTI_PACK_INDEX.\n\nIt's possible, but not something that I have seen done elsewhere for\nthis or any of the other GIT_TEST- variables.\n\nLike I said, I'd like to get rid of this (and many other)\nGIT_TEST-related variables, but that is a larger effort than this single\npatch.\n\nThanks,\nTaylor\n"},{"id":"492100","messageId":"xmqqh6gj4k04.fsf@gitster.g","threadId":"61254","inReplyTo":"ZgxSSKGdAicfVhGA@nand.local","subject":"Re: [PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T19:55:39Z","receivedAt":"2024-04-02T19:55:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> I would argue that GIT_TEST_MULTI_PACK_INDEX should be on the list of\n> GIT_TEST_-variables to get rid of as it has served its purpose.\n>\n> Like I said, I'd like to get rid of this (and many other)\n> GIT_TEST-related variables, but that is a larger effort than this single\n> patch.\nYup, that sounds like a good longer-term goals. While it does not\nsmell like it is consistent with that goal to add more instances of\nuse of it to the test, it may inevitably be a \"few steps backward in\npreparation to jump big forward\", perhaps.\n"},{"id":"492102","messageId":"Zgxk43ZH6Tz4TnaT@nand.local","threadId":"61254","inReplyTo":"xmqqh6gj4k04.fsf@gitster.g","subject":"Re: [PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-02T20:04:51Z","receivedAt":"2024-04-02T20:04:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 02, 2024 at 12:55:39PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > I would argue that GIT_TEST_MULTI_PACK_INDEX should be on the list of\n> > GIT_TEST_-variables to get rid of as it has served its purpose.\n> >\n> > Like I said, I'd like to get rid of this (and many other)\n> > GIT_TEST-related variables, but that is a larger effort than this single\n> > patch.\n>\n> Yup, that sounds like a good longer-term goals. While it does not\n> smell like it is consistent with that goal to add more instances of\n> use of it to the test, it may inevitably be a \"few steps backward in\n> preparation to jump big forward\", perhaps.\n\nYep, exactly.\n\nThanks,\nTaylor\n"},{"id":"492106","messageId":"20240402202405.GC875182@coredump.intra.peff.net","threadId":"61254","inReplyTo":"ZgxSSKGdAicfVhGA@nand.local","subject":"Re: [PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:24:05Z","receivedAt":"2024-04-02T20:24:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 02:45:28PM -0400, Taylor Blau wrote:\n\n> I have generally considered the `--write-midx` and\n> `GIT_TEST_MULTI_PACK_INDEX` options to be orthogonal to each other. The\n> latter is a developer-oriented option that forces Git to write a MIDX\n> post-repack regardless of the command-line option.\n> \n> It predates the `--write-midx` option by a number of years, and IIUC was\n> introduced to enhance test coverage while the MIDX was being originally\n> developed.\n> \n> I would argue that GIT_TEST_MULTI_PACK_INDEX should be on the list of\n> GIT_TEST_-variables to get rid of as it has served its purpose.\n\nHmm. Obviously it is of little value in this explicit --write-midx\ntest, but I thought the main value was just exercising all of the\n_other_ tests with a midx in place. Doesn't that potentially have value\n(just like testing with SPLIT_INDEX, etc, gets more coverage)?\n\nIf it is worth keeping (and I do not really have a strong opinion\nthere), the real issue seems to me that it does not behave like\n--write-midx. That is the source of the problem here, but also makes\ntest runs with it unrealistic, since the command-line option is how\nreal-world users would trigger it.\n\nI.e., I would have expected something like this, so that the variables\ntakes precedence over config but under command-line options:\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 15e4cccc45..4b02d9cb77 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -1197,6 +1197,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tgit_config(repack_config, &cruft_po_args);\n \n+\twrite_midx = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, write_midx);\n+\tif (write_midx)\n+\t\twrite_bitmaps = git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP,\n+\t\t\t\t\t write_bitmaps);\n+\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n@@ -1214,10 +1219,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (!write_midx &&\n \t\t (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n \t\t\twrite_bitmaps = 0;\n-\t} else if (write_bitmaps &&\n-\t\t git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0) &&\n-\t\t git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP, 0)) {\n-\t\twrite_bitmaps = 0;\n \t}\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps > 0 && !write_midx;\n@@ -1515,13 +1516,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (run_update_server_info)\n \t\tupdate_server_info(0);\n \n-\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {\n-\t\tunsigned flags = 0;\n-\t\tif (git_env_bool(GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP, 0))\n-\t\t\tflags |= MIDX_WRITE_BITMAP | MIDX_WRITE_REV_INDEX;\n-\t\twrite_midx_file(get_object_directory(), NULL, NULL, flags);\n-\t}\n-\n cleanup:\n \tstring_list_clear(&names, 1);\n \texisting_packs_release(&existing);\n\nBut it gets weird because some tests (like t7700.2) explicitly ask for\nbitmaps on the command line and want pack bitmaps but _not_ midx\nbitmaps.\n\nSo I dunno. Maybe this is a can of worms that is not worth falling into.\nAfter all, these are not \"real\" environment variables that we expect\nusers to use. I just wonder if the ci runs with them are buying us\nanything for all of the tests outside of t7700.\n\n-Peff\n"},{"id":"492121","messageId":"xmqqcyr72z66.fsf@gitster.g","threadId":"61254","inReplyTo":"20240402202405.GC875182@coredump.intra.peff.net","subject":"Re: [PATCH] t/t7700-repack.sh: fix test breakages with `GIT_TEST_MULTI_PACK_INDEX=1`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T22:10:57Z","receivedAt":"2024-04-02T22:11:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I dunno. Maybe this is a can of worms that is not worth falling into.\n> After all, these are not \"real\" environment variables that we expect\n> users to use. I just wonder if the ci runs with them are buying us\n> anything for all of the tests outside of t7700.\n\nYeah, I agree with the general direction of reducing the role\nGIT_TEST_* environment variables take.\n"}]}