Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jul 01, 2026 at 04:13:58AM -0400, Jeff King wrote:
> On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:
> 
> > >   make SANITIZE=leak
> > >   cd t
> > >   GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
> > > 
> > > which yields many entries like:
> > > 
> > >   ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> > >   Direct leak of 200 byte(s) in 1 object(s) allocated from:
> > >       #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> > >       #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> > >       #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> > >       #3 0x55cd45255473 in init_topo_walk revision.c:3845
> > >       #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> > >       #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> > >       #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
> > 
> > Interesting. Makes me wonder whether we should modify linux-TEST-vars to
> > also run with the leak checker enabled. Ideally we'd of course just do
> > this for all jobs, but the overhead is probably way too high... yes,
> > doing a simple benchmark shows a ~3x hit.
> > 
> > So this is definitely nothing we want to do for all jobs. But for the
> > linux-TEST-vars job it might make sense, as it exercises a bunch of
> > non-default code paths.
> 
> We already run a special leak job for linux-reftables. Why not turn that
> job into "leaks plus reftables plus test-vars"? The only downside would
> be potentially hiding leaks found by linux-reftables-leaks if the
> test-vars features force us into a difference code path. But looking at
> the list, it doesn't seem likely to me. None of them is particularly
> ref-related.
> 
> In fact, I kind of wonder if we could fold linux-reftables into the
> test-vars job completely.

linux-reftable or linux-reftable-leaks? I think it would certainly make
sense to drop one of these and merge it into linux-TEST-vars. The
linux-reftable job doesn't provide any benefit over its -leak variant,
so that would be the candidate I'd personally merge.

> > One thing worth noting: there are still six test suites that are failing
> > with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
> > failures are because of the missing call to `repo_unuse_commit_buffer()`
> > in git-history(1), which we already noted elsewhere.
> > 
> > All of the remaining leaks in t0095, t4013 and t4211 seem to be related
> > to bloom filters.
> 
> I sent some patches to fix the bloom-filter cases.

I saw them already, thanks for your work here!

Patrick