On Tue, Apr 22, 2025 at 08:49:45AM -0700, Junio C Hamano wrote: > > The fix is to remove the extra "+" (which also lets us simplify the sed > > invocation a bit, as it is now just stripping the leading directories). > > > > But that leaves the mystery of why it was ever there in the first place. > > The answer is that an earlier iteration of the patch series had a > > concept of "disjoint" packs in the midx. And one of its patches here: > > > > https://lore.kernel.org/git/c52d7e7b27a9add4f58b8334db4fe4498af1c90f.1701198172.git.me@xxxxxxxxxxxx/ > > > > taught read_packs_from_stdin() to treat a leading "+" as marking a > > disjoint pack. But in the second version of the series, which was > > ultimately merged, that disjoint concept went away, and the code to > > parse "+" did likewise. The regular regression tests were adjusted to > > match, but this case in t/perf was forgotten. > > > > Signed-off-by: Jeff King <peff@xxxxxxxx> > > --- > > t/perf/p5332-multi-pack-reuse.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks. I wonder if we had some tools and mechanisms people were > discussing to track changes on changsets, such a mishap could have > been caught more easily. [jc: random folks from that discussion > CC'ed, just in case they are interested]. IMHO it would probably not help that much, because the error was in the other direction. I.e., the issue was that something _didn't_ change between two versions of the series, but should have. In general I think we'd usually rely on tests or compiler analysis (e.g., leftover unused variables) to catch this kind of thing. It's just that hardly anybody actually runs the perf tests. I know there was some discussion about running them regularly in CI, but I'm skeptical that the CPU time / utility tradeoff is very good there. In some sense, this case was the process working as designed. Running the test _did_ catch the problem, but we didn't notice because nobody ran it for a while. So in the interim, nobody was hurt. ;) I'm mostly joking. It is certainly more convenient to catch these things earlier, but latent buggy code that nobody runs might not be that big a worry. -Peff
