Re: [PATCH] p5332: drop "+" from --stdin-packs input

From: Jeff King <peff@peff.net>
Date: May 1, 2025, 16:03 UTC
Message-ID: <20250501160344.GA1794891@coredump.intra.peff.net>
In-Reply-To: <xmqqv7qw42na.fsf@gitster.g>

On Tue, Apr 22, 2025 at 08:49:45AM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> > 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@ttaylorr.com/
> >
> > 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@peff.net>
> > ---
> >  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