[PATCH 0/2] small leak fix in format-patch

[PATCH 0/2] small leak fix in format-patch

[Jeff King]

This fixes a leak I found while discussing an unrelated leak in another
thread[1]. As a bonus, this fixes some minor recent breakage of
leak-reporting when running the test suite under prove. The patches can
be split into separate topics if we want.

  [1/2]: t: move LSan errors from stdout to stderr
  [2/2]: format-patch: fix leak of rev_info in prepare_bases()

 builtin/log.c | 1 +
 t/test-lib.sh | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

-Peff

[1] https://lore.kernel.org/git/20260630055026.GE2495216@coredump.intra.peff.net/

[PATCH 1/2] t: move LSan errors from stdout to stderr

[Jeff King]

When we find LSan errors, we dump them via "say_color", which goes to
stdout. This is mostly harmless, since stdout and stderr tend to go to
the same place (either the user's terminal, or to the ".out" file with
--verbose-log).

But when running under a TAP harness like prove, they are split and
stdout is interpreted as TAP output. Historically even this was fine, as
the extra lines on stdout would be ignored. But since 389c83025d (t: let
prove fail when parsing invalid TAP output, 2026-06-04) we instruct the
TAP reader to complain, and a leaking test will result in complaints
like this (this is a real leak which we have yet to fix):

  $ GIT_TEST_COMMIT_GRAPH=1 make SANITIZE=leak test
  [...]
  Test Summary Report
  -------------------
  t4014-format-patch.sh (Wstat: 256 (exited 1) Tests: 226 Failed: 30)
    Failed tests:  197-226
    Non-zero exit status: 1
    Parse errors: Unknown TAP token: ""
                  Unknown TAP token: "================================================================="
                  Unknown TAP token: "==git==3693658==ERROR: LeakSanitizer: detected memory leaks"
                  Unknown TAP token: ""
                  Unknown TAP token: "Direct leak of 200 byte(s) in 1 object(s) allocated from:"
  Displayed the first 5 of 1531 TAP syntax errors.
  Re-run prove with the -p option to see them all.

You still see the failing tests, so it's mostly just an annoyance. We
can fix it by redirecting to stderr (actually descriptor 4, which is our
verbose-respecting variant). I confirmed manually that the output still
appears with --verbose-log, and even with a single-test "-i
--verbose-only=197" going to the terminal.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/test-lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index ceefb99bff..d390c53ec1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1217,14 +1217,14 @@ check_test_results_san_file_ () {
 	then
 		return
 	fi &&
-	say_color error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
+	say_color >&4 error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
 
 	if test "$test_failure" = 0
 	then
-		say "Our logs revealed a memory leak, exit non-zero!" &&
+		say >&4 "Our logs revealed a memory leak, exit non-zero!" &&
 		invert_exit_code=t
 	else
-		say "Our logs revealed a memory leak..."
+		say >&4 "Our logs revealed a memory leak..."
 	fi
 }
 
-- 
2.55.0.346.g83d0ea82e4

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

[Jeff King]

In prepare_bases() we do a custom revision walk, separate from the main
format-patch walk. After we finish, we fail to call release_revisions(),
possibly leaking its contents.

We failed to notice it so far because the revision machinery doesn't
always allocate. But at least one case can trigger the leak: if a commit
graph is present, then the topo-walk allocates revs.topo_walk_info and
some associated data structures. You can see it in the test suite by
running:

  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

The un-released rev_info has been there since the code was added in
fa2ab86d18 (format-patch: add '--base' option to record base tree info,
2016-04-26), but back then we didn't even have a way to release rev_info
resources! The actual leak probably started around f0d9cc4196
(revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's
hard to bisect because there were so many other unrelated leaks back
then.

So I'm not sure exactly when the leak started beyond "long ago", but it
is easy-ish to find now (since we've plugged all those other leaks) and
the solution is clear.

I didn't add a new test since we can demonstrate it with the existing
ones, but it does require tweaking a test variable. We might consider
ways to get more automatic leak-checking coverage there, but I think it
should be done outside of this fix.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/log.c b/builtin/log.c
index d027ce1e0b..350b35c556 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,
 		bases->nr_patch_id++;
 	}
 	clear_commit_base(&commit_base);
+	release_revisions(&revs);
 }
 
 static void print_bases(struct base_tree_info *bases, FILE *file)
-- 
2.55.0.346.g83d0ea82e4

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

[Patrick Steinhardt]

On Tue, Jun 30, 2026 at 02:43:01AM -0400, Jeff King wrote:
> In prepare_bases() we do a custom revision walk, separate from the main
> format-patch walk. After we finish, we fail to call release_revisions(),
> possibly leaking its contents.
> 
> We failed to notice it so far because the revision machinery doesn't
> always allocate. But at least one case can trigger the leak: if a commit
> graph is present, then the topo-walk allocates revs.topo_walk_info and
> some associated data structures. You can see it in the test suite by
> running:
> 
>   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.

> The un-released rev_info has been there since the code was added in
> fa2ab86d18 (format-patch: add '--base' option to record base tree info,
> 2016-04-26), but back then we didn't even have a way to release rev_info
> resources! The actual leak probably started around f0d9cc4196
> (revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's
> hard to bisect because there were so many other unrelated leaks back
> then.
> 
> So I'm not sure exactly when the leak started beyond "long ago", but it
> is easy-ish to find now (since we've plugged all those other leaks) and
> the solution is clear.
> 
> I didn't add a new test since we can demonstrate it with the existing
> ones, but it does require tweaking a test variable. We might consider
> ways to get more automatic leak-checking coverage there, but I think it
> should be done outside of this fix.

Yeah, agreed.

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.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/log.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index d027ce1e0b..350b35c556 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,
>  		bases->nr_patch_id++;
>  	}
>  	clear_commit_base(&commit_base);
> +	release_revisions(&revs);
>  }

The fix looks sensible to me. We always initialize `revs` before we take
this exit path here, and there is no other early return that we'd have
to adjust.

Thanks!

Patrick

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

[Jeff King]

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.

> 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.

Building with OPENSSL_SHA1_UNSAFE turns up more. The core issue is that
recent versions of openssl require an allocation to open a sha1 context,
and we free it in git_hash_final(). So code paths that abort mid-hash
will leak the allocation, and we need a git_hash_discard().

It comes up mostly with csum-file.[ch], since that's where we use the
unsafe variant.

If you further build with OPENSSL_SHA1 (using it for _all_ hash
computations), there are a few more cases. It's hard to care too much
since that isn't a recommended build (and we've even discussed dropping
support for non-dc sha1 totally). But sha256 has the same issue, so
we'll want to fix it eventually (I didn't try leak-checking the
linux-sha256 build, but I expect it would complain a lot).

I have some patches but they need a bit of polish. In particular I think
we'll have to tweak the hash.h #define mess to expose a "discard"
primitive from each implementation (otherwise we have to finalize the
hash to discard, which is a little inefficient). I didn't quite have the
stomach for that tonight.

-Peff

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

[Patrick Steinhardt]

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

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

[Jeff King]

On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:

> > 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.

Both. Fold linux-reftable into linux-TEST-vars, and then drop
linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.

-Peff

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

[Patrick Steinhardt]

On Wed, Jul 01, 2026 at 04:47:33AM -0400, Jeff King wrote:
> On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:
> 
> > > 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.
> 
> Both. Fold linux-reftable into linux-TEST-vars, and then drop
> linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.

Hm, okay. I guess that should be fine. Do we also want to do a similar
thing for macOS and create a macos-TEST-vars job that exercises all of
this?

Also, while at it... I really think that job name is just plain awful.
While at it, we might rename it to something more sensible like
"linux-changed-defaults".

Patrick

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

[Jeff King]

On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:

> > > 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.
> > 
> > Both. Fold linux-reftable into linux-TEST-vars, and then drop
> > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
> 
> Hm, okay. I guess that should be fine. Do we also want to do a similar
> thing for macOS and create a macos-TEST-vars job that exercises all of
> this?

It could be helpful if we expect the interaction of macOS and those
test-vars to be interesting, but I'm a bit skeptical. Most of them are
about feature selection. So I'm doubtful it would turn up anything
useful. But who knows.

Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
both is useful, as they have different warnings. But have we ever seen a
case where running the tests showed a different result with different
compilers?

I dunno. I guess there is an argument for CI-maximalism; as long as the
jobs run in parallel and they're "just" CPU-minutes. But those minutes
eventually have a cost, and I'm not sure I've gotten useful data from
most of the jobs (i.e., failures that didn't also just happen somewhere
else).

Anyway, that is all a big tangent/rant. Mostly I think it would be fine
to cannibalize linux-reftable into linux-TEST-vars if we want to get
more coverage without increasing the CI cost.

Note that I did find some leaks that would only be hit running
linux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But
that is getting super specific now (even if we ran linux-sha256 with
leak detection, would we want to do it with openssl and not the default
backend)?

> Also, while at it... I really think that job name is just plain awful.
> While at it, we might rename it to something more sensible like
> "linux-changed-defaults".

Yes please. Every time I see the all-caps TEST in the middle I think I'm
having a stroke.

change-defaults is OK but not super descriptive. I might call it
linux-exotic-flags or something. That's not descriptive either, but is a
little more fun.

-Peff

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

[Patrick Steinhardt]

On Thu, Jul 02, 2026 at 04:58:21AM -0400, Jeff King wrote:
> On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:
> 
> > > > 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.
> > > 
> > > Both. Fold linux-reftable into linux-TEST-vars, and then drop
> > > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.
> > 
> > Hm, okay. I guess that should be fine. Do we also want to do a similar
> > thing for macOS and create a macos-TEST-vars job that exercises all of
> > this?
> 
> It could be helpful if we expect the interaction of macOS and those
> test-vars to be interesting, but I'm a bit skeptical. Most of them are
> about feature selection. So I'm doubtful it would turn up anything
> useful. But who knows.
> 
> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
> both is useful, as they have different warnings. But have we ever seen a
> case where running the tests showed a different result with different
> compilers?

Not that I'd know of. As you say, I think it makes sense to use
different compilers in general. But I don't really think we need to have
this as a full "compiler x tests" matrix.

> I dunno. I guess there is an argument for CI-maximalism; as long as the
> jobs run in parallel and they're "just" CPU-minutes. But those minutes
> eventually have a cost, and I'm not sure I've gotten useful data from
> most of the jobs (i.e., failures that didn't also just happen somewhere
> else).

I'm certainly on board with reducing the test matrix a bit. I'm sure
that we can have a cleverer selection of jobs where we both have the
same test coverage as we have right now while running less jobs overall.

> Anyway, that is all a big tangent/rant. Mostly I think it would be fine
> to cannibalize linux-reftable into linux-TEST-vars if we want to get
> more coverage without increasing the CI cost.

You got to start somewhere :)

> Note that I did find some leaks that would only be hit running
> linux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But
> that is getting super specific now (even if we ran linux-sha256 with
> leak detection, would we want to do it with openssl and not the default
> backend)?
> 
> > Also, while at it... I really think that job name is just plain awful.
> > While at it, we might rename it to something more sensible like
> > "linux-changed-defaults".
> 
> Yes please. Every time I see the all-caps TEST in the middle I think I'm
> having a stroke.

Heh :P

> change-defaults is OK but not super descriptive. I might call it
> linux-exotic-flags or something. That's not descriptive either, but is a
> little more fun.

I certainly like it more than my suggestion.

Thanks!

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
>> both is useful, as they have different warnings. But have we ever seen a
>> case where running the tests showed a different result with different
>> compilers?
>
> Not that I'd know of. As you say, I think it makes sense to use
> different compilers in general. But I don't really think we need to have
> this as a full "compiler x tests" matrix.

Very true.  Different configurations with TEST-vars are great
combination to test, but we are not in the business of hunting bugs
in clang/gcc so we long as they compile (instead of warning "hey,
that construct gives you undefined behaviour"), we shouldn't have to
run the test suite with the same configuration for both.

> I'm certainly on board with reducing the test matrix a bit. I'm sure
> that we can have a cleverer selection of jobs where we both have the
> same test coverage as we have right now while running less jobs overall.

Yeah, and if we can spend the saved cycles for better coverage, that
would be grat.

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

[Jeff King]

On Fri, Jul 03, 2026 at 01:45:15PM -0700, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
> >> both is useful, as they have different warnings. But have we ever seen a
> >> case where running the tests showed a different result with different
> >> compilers?
> >
> > Not that I'd know of. As you say, I think it makes sense to use
> > different compilers in general. But I don't really think we need to have
> > this as a full "compiler x tests" matrix.
> 
> Very true.  Different configurations with TEST-vars are great
> combination to test, but we are not in the business of hunting bugs
> in clang/gcc so we long as they compile (instead of warning "hey,
> that construct gives you undefined behaviour"), we shouldn't have to
> run the test suite with the same configuration for both.

I don't care about finding bugs in clang vs gcc. I'm more concerned with
a case where we have undefined behavior, both compile it fine, but the
bad behavior is revealed in the tests only by one of them.

I can think offhand of only one case where I saw that happen[1]. IIRC it
had to do with integer sizes being passed to a variadic function. But it
also changed behavior within the same compiler using different
optimization levels. So it feels like kind of a scattershot way of
trying to flush out UB, and we are probably better off with UBSan and
friends.

-Peff

[1] I mentioned it in:

      https://lore.kernel.org/git/20251130134625.GA199421@coredump.intra.peff.net/

    but didn't give enough details for it to be useful here. I mention
    it merely as the only anecdote I could call to mind. :)

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

[Patrick Steinhardt]

On Sun, Jul 05, 2026 at 08:34:29PM -0400, Jeff King wrote:
> On Fri, Jul 03, 2026 at 01:45:15PM -0700, Junio C Hamano wrote:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> > 
> > >> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with
> > >> both is useful, as they have different warnings. But have we ever seen a
> > >> case where running the tests showed a different result with different
> > >> compilers?
> > >
> > > Not that I'd know of. As you say, I think it makes sense to use
> > > different compilers in general. But I don't really think we need to have
> > > this as a full "compiler x tests" matrix.
> > 
> > Very true.  Different configurations with TEST-vars are great
> > combination to test, but we are not in the business of hunting bugs
> > in clang/gcc so we long as they compile (instead of warning "hey,
> > that construct gives you undefined behaviour"), we shouldn't have to
> > run the test suite with the same configuration for both.
> 
> I don't care about finding bugs in clang vs gcc. I'm more concerned with
> a case where we have undefined behavior, both compile it fine, but the
> bad behavior is revealed in the tests only by one of them.
> 
> I can think offhand of only one case where I saw that happen[1]. IIRC it
> had to do with integer sizes being passed to a variadic function. But it
> also changed behavior within the same compiler using different
> optimization levels. So it feels like kind of a scattershot way of
> trying to flush out UB, and we are probably better off with UBSan and
> friends.

Yes, I just wanted to say that UBSan is definitely the better way to go
in this context. I have no idea of course whether it would have catched
the mentioned issue, though.

And even so, we'd have at least one job that runs all tests with either
of the compilers, so we'd still notice issues like that.

Patrick

Re: [PATCH 0/2] small leak fix in format-patch

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Jeff King <peff@peff.net> writes:

> This fixes a leak I found while discussing an unrelated leak in another
> thread[1]. As a bonus, this fixes some minor recent breakage of
> leak-reporting when running the test suite under prove. The patches can
> be split into separate topics if we want.

I think you meant to CC the other "Kaartic" :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 0/2] small leak fix in format-patch

[Jeff King]

On Sat, Jul 04, 2026 at 02:13:52PM -0700, Karthik Nayak wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This fixes a leak I found while discussing an unrelated leak in another
> > thread[1]. As a bonus, this fixes some minor recent breakage of
> > leak-reporting when running the test suite under prove. The patches can
> > be split into separate topics if we want.
> 
> I think you meant to CC the other "Kaartic" :)

Yes, I figured that out about halfway through the discussion and hoped
nobody might notice. ;) Sorry to both of you for the confusion.

-Peff