{"thread":{"id":"63323","subject":"Test failure in p5332-multi-pack-reuse.sh","startedAt":"2025-04-22T02:01:28Z","lastAt":"2025-05-01T16:03:45Z","messageCount":6,"participants":["Philippe Blain","Junio C Hamano","Jeff King","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"516449","messageId":"292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com","threadId":"63323","inReplyTo":null,"subject":"Test failure in p5332-multi-pack-reuse.sh","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2025-04-22T02:01:25Z","receivedAt":"2025-04-22T02:01:28Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Taylor,\n\nI noticed that p5332-multi-pack-reuse.sh, which you added in \nba47d88795 (t/perf: add performance tests for multi-pack reuse,\n2023-12-14) fails early on in the second test (\"setup bitmaps for\n1-pack scenario\"). Since perf tests run with '--immediate', I do not\nknow if further tests in that file also fail. It is reproducible on macOS [1] as \nwell as Linux [2] (I don't know if these logs are public though).\n\nI also tested on Linux on version 2.44.0 which is the first release\nin which this test was added, and it also failed similarily.\n\nSidenote: on GitHub CI, I could not demonstrate the failure on Linux\nbecause all Linux jobs run in containers, and the images we use do \nnot have Git installed, such that actions/checkout@v4 uses the GitHub\nAPI to download the repository instead of cloning it [3]. This leads \ndie_if_build_dir_not_repo from perf-lib.sh to fail with\n\"No $GIT_PERF_REPO defined, and your build directory is not a repo\" [4].\nWe could fix that by installing the 'git' package before the 'actions/checkout'\nstep, but we would need to account for the different package managers of \nthe distros we test on.\n\nCheers,\n\nPhilippe.\n\n[1] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421311#step:4:896\n[2] https://gitlab.com/phil-blain/git/-/jobs/9780586827#L2889\n[3] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421399#step:4:28\n[4] https://github.com/phil-blain/git/actions/runs/14580975799/job/40897421399#step:8:838\n"},{"id":"516450","messageId":"xmqqcyd46dsb.fsf@gitster.g","threadId":"63323","inReplyTo":"292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com","subject":"Re: Test failure in p5332-multi-pack-reuse.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-22T04:06:12Z","receivedAt":"2025-04-22T04:06:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Sidenote: on GitHub CI, I could not demonstrate the failure on Linux\n> because all Linux jobs run in containers, and the images we use do \n> not have Git installed, such that actions/checkout@v4 uses the GitHub\n> API to download the repository instead of cloning it [3]. This leads \n> die_if_build_dir_not_repo from perf-lib.sh to fail with\n> \"No $GIT_PERF_REPO defined, and your build directory is not a repo\" [4].\n> We could fix that by installing the 'git' package before the 'actions/checkout'\n> step, but we would need to account for the different package managers of \n> the distros we test on.\n\nNot limited to this topic, but wouldn't it make more sense to first\nrun install-dependencies (including \"/usr/bin/git\") and then invoke\nthe actions/checkout thing, I have to wonder.  We were bitten by a\nseparate topic due to the same issue quite recently.\n\nThanks.\n"},{"id":"516490","messageId":"20250422111632.GA1855088@coredump.intra.peff.net","threadId":"63323","inReplyTo":"292ae7a3-2aad-1f22-2afe-739ec921d6b7@gmail.com","subject":"[PATCH] p5332: drop \"+\" from --stdin-packs input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-22T11:16:32Z","receivedAt":"2025-04-22T11:16:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 21, 2025 at 10:01:25PM -0400, Philippe Blain wrote:\n\n> I noticed that p5332-multi-pack-reuse.sh, which you added in \n> ba47d88795 (t/perf: add performance tests for multi-pack reuse,\n> 2023-12-14) fails early on in the second test (\"setup bitmaps for\n> 1-pack scenario\"). Since perf tests run with '--immediate', I do not\n> know if further tests in that file also fail. It is reproducible on macOS [1] as \n> well as Linux [2] (I don't know if these logs are public though).\n> \n> I also tested on Linux on version 2.44.0 which is the first release\n> in which this test was added, and it also failed similarily.\n\nI think the patch below is probably the right solution. With it I got\nthe output I'd expect (multi-pack reuse with many packs yields a CPU\nspeedup at the cost of increased size):\n\n  Test                                                            this tree\n  ----------------------------------------------------------------------------------\n  5332.3: clone for 1-pack scenario (single-pack reuse)           6.66(37.73+0.19)\n  5332.4: clone size for 1-pack scenario (single-pack reuse)               117.0M\n  5332.5: clone for 1-pack scenario (multi-pack reuse)            6.89(38.71+0.25)\n  5332.6: clone size for 1-pack scenario (multi-pack reuse)                117.0M\n  5332.9: clone for 10-pack scenario (single-pack reuse)          5.67(35.65+0.37)\n  5332.10: clone size for 10-pack scenario (single-pack reuse)             125.1M\n  5332.11: clone for 10-pack scenario (multi-pack reuse)          2.47(5.71+0.15)\n  5332.12: clone size for 10-pack scenario (multi-pack reuse)              134.3M\n  5332.15: clone for 100-pack scenario (single-pack reuse)        14.50(130.54+0.55)\n  5332.16: clone size for 100-pack scenario (single-pack reuse)            224.2M\n  5332.17: clone for 100-pack scenario (multi-pack reuse)         3.34(3.69+0.18)\n  5332.18: clone size for 100-pack scenario (multi-pack reuse)             307.3M\n\n-- >8 --\nSubject: [PATCH] p5332: drop \"+\" from --stdin-packs input\n\nThis perf script creates a midx by running \"git multi-pack-index write\"\nwith the \"--stdin-packs\" option. We feed that stdin by running \"find\" on\n.git/objects/pack, using sed to strip off everything but the basename.\n\nBut that sed invocation also does something peculiar: it adds a \"+\" to\nthe start of each pack name. This causes the multi-pack-index command to\nbarf. The modified name does not match any pack it knows about, so it\nends up with an empty list of packs to put in the midx. And thus nothing\nmatches the --preferred-pack option we pass, which causes it die().\n\nThe fix is to remove the extra \"+\" (which also lets us simplify the sed\ninvocation a bit, as it is now just stripping the leading directories).\n\nBut that leaves the mystery of why it was ever there in the first place.\nThe answer is that an earlier iteration of the patch series had a\nconcept of \"disjoint\" packs in the midx. And one of its patches here:\n\n  https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/\n\ntaught read_packs_from_stdin() to treat a leading \"+\" as marking a\ndisjoint pack. But in the second version of the series, which was\nultimately merged, that disjoint concept went away, and the code to\nparse \"+\" did likewise. The regular regression tests were adjusted to\nmatch, but this case in t/perf was forgotten.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/perf/p5332-multi-pack-reuse.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/perf/p5332-multi-pack-reuse.sh b/t/perf/p5332-multi-pack-reuse.sh\nindex d1c89a8b7d..0a2525db44 100755\n--- a/t/perf/p5332-multi-pack-reuse.sh\n+++ b/t/perf/p5332-multi-pack-reuse.sh\n@@ -58,7 +58,7 @@ do\n \t'\n \n \ttest_expect_success \"setup bitmaps for $nr_packs-pack scenario\" '\n-\t\tfind $packdir -type f -name \"*.idx\" | sed -e \"s/.*\\/\\(.*\\)$/+\\1/g\" |\n+\t\tfind $packdir -type f -name \"*.idx\" | sed -e \"s/.*\\///\" |\n \t\tgit multi-pack-index write --stdin-packs --bitmap \\\n \t\t\t--preferred-pack=\"$(find_pack $(git rev-parse HEAD))\"\n \t'\n-- \n2.49.0.682.g886cb1c59a\n\n"},{"id":"516512","messageId":"xmqqv7qw42na.fsf@gitster.g","threadId":"63323","inReplyTo":"20250422111632.GA1855088@coredump.intra.peff.net","subject":"Re: [PATCH] p5332: drop \"+\" from --stdin-packs input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-04-22T15:49:45Z","receivedAt":"2025-04-22T15:49:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>\n> -- >8 --\n> Subject: [PATCH] p5332: drop \"+\" from --stdin-packs input\n>\n> This perf script creates a midx by running \"git multi-pack-index write\"\n> with the \"--stdin-packs\" option. We feed that stdin by running \"find\" on\n> .git/objects/pack, using sed to strip off everything but the basename.\n>\n> But that sed invocation also does something peculiar: it adds a \"+\" to\n> the start of each pack name. This causes the multi-pack-index command to\n> barf. The modified name does not match any pack it knows about, so it\n> ends up with an empty list of packs to put in the midx. And thus nothing\n> matches the --preferred-pack option we pass, which causes it die().\n>\n> The fix is to remove the extra \"+\" (which also lets us simplify the sed\n> invocation a bit, as it is now just stripping the leading directories).\n>\n> But that leaves the mystery of why it was ever there in the first place.\n> The answer is that an earlier iteration of the patch series had a\n> concept of \"disjoint\" packs in the midx. And one of its patches here:\n>\n>   https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/\n>\n> taught read_packs_from_stdin() to treat a leading \"+\" as marking a\n> disjoint pack. But in the second version of the series, which was\n> ultimately merged, that disjoint concept went away, and the code to\n> parse \"+\" did likewise. The regular regression tests were adjusted to\n> match, but this case in t/perf was forgotten.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/perf/p5332-multi-pack-reuse.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nThanks.  I wonder if we had some tools and mechanisms people were\ndiscussing to track changes on changsets, such a mishap could have\nbeen caught more easily.  [jc: random folks from that discussion\nCC'ed, just in case they are interested].\n\n>\n> diff --git a/t/perf/p5332-multi-pack-reuse.sh b/t/perf/p5332-multi-pack-reuse.sh\n> index d1c89a8b7d..0a2525db44 100755\n> --- a/t/perf/p5332-multi-pack-reuse.sh\n> +++ b/t/perf/p5332-multi-pack-reuse.sh\n> @@ -58,7 +58,7 @@ do\n>  \t'\n>  \n>  \ttest_expect_success \"setup bitmaps for $nr_packs-pack scenario\" '\n> -\t\tfind $packdir -type f -name \"*.idx\" | sed -e \"s/.*\\/\\(.*\\)$/+\\1/g\" |\n> +\t\tfind $packdir -type f -name \"*.idx\" | sed -e \"s/.*\\///\" |\n>  \t\tgit multi-pack-index write --stdin-packs --bitmap \\\n>  \t\t\t--preferred-pack=\"$(find_pack $(git rev-parse HEAD))\"\n>  \t'\n"},{"id":"516516","messageId":"aAfQwrhuLF7BysyE@nand.local","threadId":"63323","inReplyTo":"20250422111632.GA1855088@coredump.intra.peff.net","subject":"Re: [PATCH] p5332: drop \"+\" from --stdin-packs input","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-04-22T17:24:18Z","receivedAt":"2025-04-22T17:24:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 22, 2025 at 07:16:32AM -0400, Jeff King wrote:\n> ---\n>  t/perf/p5332-multi-pack-reuse.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nMy apologies for the mistake in the first place, but thank you for\ndigging and providing the fix.\n\n  Acked-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"517048","messageId":"20250501160344.GA1794891@coredump.intra.peff.net","threadId":"63323","inReplyTo":"xmqqv7qw42na.fsf@gitster.g","subject":"Re: [PATCH] p5332: drop \"+\" from --stdin-packs input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-01T16:03:44Z","receivedAt":"2025-05-01T16:03:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 22, 2025 at 08:49:45AM -0700, Junio C Hamano wrote:\n\n> > The fix is to remove the extra \"+\" (which also lets us simplify the sed\n> > invocation a bit, as it is now just stripping the leading directories).\n> >\n> > But that leaves the mystery of why it was ever there in the first place.\n> > The answer is that an earlier iteration of the patch series had a\n> > concept of \"disjoint\" packs in the midx. And one of its patches here:\n> >\n> >   https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@ttaylorr.com/\n> >\n> > taught read_packs_from_stdin() to treat a leading \"+\" as marking a\n> > disjoint pack. But in the second version of the series, which was\n> > ultimately merged, that disjoint concept went away, and the code to\n> > parse \"+\" did likewise. The regular regression tests were adjusted to\n> > match, but this case in t/perf was forgotten.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> >  t/perf/p5332-multi-pack-reuse.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> Thanks.  I wonder if we had some tools and mechanisms people were\n> discussing to track changes on changsets, such a mishap could have\n> been caught more easily.  [jc: random folks from that discussion\n> CC'ed, just in case they are interested].\n\nIMHO it would probably not help that much, because the error was in the\nother direction. I.e., the issue was that something _didn't_ change\nbetween two versions of the series, but should have.\n\nIn general I think we'd usually rely on tests or compiler analysis\n(e.g., leftover unused variables) to catch this kind of thing. It's just\nthat hardly anybody actually runs the perf tests. I know there was some\ndiscussion about running them regularly in CI, but I'm skeptical that\nthe CPU time / utility tradeoff is very good there.\n\nIn some sense, this case was the process working as designed. Running\nthe test _did_ catch the problem, but we didn't notice because nobody\nran it for a while. So in the interim, nobody was hurt. ;) I'm mostly\njoking. It is certainly more convenient to catch these things earlier,\nbut latent buggy code that nobody runs might not be that big a worry.\n\n-Peff\n"}]}