{"thread":{"id":"65892","subject":"[PATCH 0/2] small leak fix in format-patch","startedAt":"2026-06-30T06:39:46Z","lastAt":"2026-07-06T05:57:45Z","messageCount":15,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"546733","messageId":"20260630063944.GA3733670@coredump.intra.peff.net","threadId":"65892","inReplyTo":null,"subject":"[PATCH 0/2] small leak fix in format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T06:39:44Z","receivedAt":"2026-06-30T06:39:46Z","isPatch":true,"body":"This fixes a leak I found while discussing an unrelated leak in another\nthread[1]. As a bonus, this fixes some minor recent breakage of\nleak-reporting when running the test suite under prove. The patches can\nbe split into separate topics if we want.\n\n  [1/2]: t: move LSan errors from stdout to stderr\n  [2/2]: format-patch: fix leak of rev_info in prepare_bases()\n\n builtin/log.c | 1 +\n t/test-lib.sh | 6 +++---\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\n-Peff\n\n[1] https://lore.kernel.org/git/20260630055026.GE2495216@coredump.intra.peff.net/\n"},{"id":"546734","messageId":"20260630064159.GA3733961@coredump.intra.peff.net","threadId":"65892","inReplyTo":"20260630063944.GA3733670@coredump.intra.peff.net","subject":"[PATCH 1/2] t: move LSan errors from stdout to stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T06:41:59Z","receivedAt":"2026-06-30T06:42:00Z","isPatch":true,"body":"When we find LSan errors, we dump them via \"say_color\", which goes to\nstdout. This is mostly harmless, since stdout and stderr tend to go to\nthe same place (either the user's terminal, or to the \".out\" file with\n--verbose-log).\n\nBut when running under a TAP harness like prove, they are split and\nstdout is interpreted as TAP output. Historically even this was fine, as\nthe extra lines on stdout would be ignored. But since 389c83025d (t: let\nprove fail when parsing invalid TAP output, 2026-06-04) we instruct the\nTAP reader to complain, and a leaking test will result in complaints\nlike this (this is a real leak which we have yet to fix):\n\n  $ GIT_TEST_COMMIT_GRAPH=1 make SANITIZE=leak test\n  [...]\n  Test Summary Report\n  -------------------\n  t4014-format-patch.sh (Wstat: 256 (exited 1) Tests: 226 Failed: 30)\n    Failed tests:  197-226\n    Non-zero exit status: 1\n    Parse errors: Unknown TAP token: \"\"\n                  Unknown TAP token: \"=================================================================\"\n                  Unknown TAP token: \"==git==3693658==ERROR: LeakSanitizer: detected memory leaks\"\n                  Unknown TAP token: \"\"\n                  Unknown TAP token: \"Direct leak of 200 byte(s) in 1 object(s) allocated from:\"\n  Displayed the first 5 of 1531 TAP syntax errors.\n  Re-run prove with the -p option to see them all.\n\nYou still see the failing tests, so it's mostly just an annoyance. We\ncan fix it by redirecting to stderr (actually descriptor 4, which is our\nverbose-respecting variant). I confirmed manually that the output still\nappears with --verbose-log, and even with a single-test \"-i\n--verbose-only=197\" going to the terminal.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex ceefb99bff..d390c53ec1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1217,14 +1217,14 @@ check_test_results_san_file_ () {\n \tthen\n \t\treturn\n \tfi &&\n-\tsay_color error \"$(cat \"$TEST_RESULTS_SAN_FILE\".*)\" &&\n+\tsay_color >&4 error \"$(cat \"$TEST_RESULTS_SAN_FILE\".*)\" &&\n \n \tif test \"$test_failure\" = 0\n \tthen\n-\t\tsay \"Our logs revealed a memory leak, exit non-zero!\" &&\n+\t\tsay >&4 \"Our logs revealed a memory leak, exit non-zero!\" &&\n \t\tinvert_exit_code=t\n \telse\n-\t\tsay \"Our logs revealed a memory leak...\"\n+\t\tsay >&4 \"Our logs revealed a memory leak...\"\n \tfi\n }\n \n-- \n2.55.0.346.g83d0ea82e4\n\n"},{"id":"546735","messageId":"20260630064301.GB3733961@coredump.intra.peff.net","threadId":"65892","inReplyTo":"20260630063944.GA3733670@coredump.intra.peff.net","subject":"[PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-06-30T06:43:01Z","receivedAt":"2026-06-30T06:43:02Z","isPatch":true,"body":"In prepare_bases() we do a custom revision walk, separate from the main\nformat-patch walk. After we finish, we fail to call release_revisions(),\npossibly leaking its contents.\n\nWe failed to notice it so far because the revision machinery doesn't\nalways allocate. But at least one case can trigger the leak: if a commit\ngraph is present, then the topo-walk allocates revs.topo_walk_info and\nsome associated data structures. You can see it in the test suite by\nrunning:\n\n  make SANITIZE=leak\n  cd t\n  GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh\n\nwhich yields many entries like:\n\n  ==git==3687620==ERROR: LeakSanitizer: detected memory leaks\n  Direct leak of 200 byte(s) in 1 object(s) allocated from:\n      #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74\n      #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55\n      #2 0x55cd452cdd9d in xmalloc wrapper.c:76\n      #3 0x55cd45255473 in init_topo_walk revision.c:3845\n      #4 0x55cd45255bef in prepare_revision_walk revision.c:4017\n      #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872\n      #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439\n\nThe un-released rev_info has been there since the code was added in\nfa2ab86d18 (format-patch: add '--base' option to record base tree info,\n2016-04-26), but back then we didn't even have a way to release rev_info\nresources! The actual leak probably started around f0d9cc4196\n(revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's\nhard to bisect because there were so many other unrelated leaks back\nthen.\n\nSo I'm not sure exactly when the leak started beyond \"long ago\", but it\nis easy-ish to find now (since we've plugged all those other leaks) and\nthe solution is clear.\n\nI didn't add a new test since we can demonstrate it with the existing\nones, but it does require tweaking a test variable. We might consider\nways to get more automatic leak-checking coverage there, but I think it\nshould be done outside of this fix.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/log.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d027ce1e0b..350b35c556 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,\n \t\tbases->nr_patch_id++;\n \t}\n \tclear_commit_base(&commit_base);\n+\trelease_revisions(&revs);\n }\n \n static void print_bases(struct base_tree_info *bases, FILE *file)\n-- \n2.55.0.346.g83d0ea82e4\n"},{"id":"546748","messageId":"akOZy-BygZS8fqPM@pks.im","threadId":"65892","inReplyTo":"20260630064301.GB3733961@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-30T10:26:19Z","receivedAt":"2026-06-30T10:26:29Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 02:43:01AM -0400, Jeff King wrote:\n> In prepare_bases() we do a custom revision walk, separate from the main\n> format-patch walk. After we finish, we fail to call release_revisions(),\n> possibly leaking its contents.\n> \n> We failed to notice it so far because the revision machinery doesn't\n> always allocate. But at least one case can trigger the leak: if a commit\n> graph is present, then the topo-walk allocates revs.topo_walk_info and\n> some associated data structures. You can see it in the test suite by\n> running:\n> \n>   make SANITIZE=leak\n>   cd t\n>   GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh\n> \n> which yields many entries like:\n> \n>   ==git==3687620==ERROR: LeakSanitizer: detected memory leaks\n>   Direct leak of 200 byte(s) in 1 object(s) allocated from:\n>       #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74\n>       #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55\n>       #2 0x55cd452cdd9d in xmalloc wrapper.c:76\n>       #3 0x55cd45255473 in init_topo_walk revision.c:3845\n>       #4 0x55cd45255bef in prepare_revision_walk revision.c:4017\n>       #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872\n>       #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439\n\nInteresting. Makes me wonder whether we should modify linux-TEST-vars to\nalso run with the leak checker enabled. Ideally we'd of course just do\nthis for all jobs, but the overhead is probably way too high... yes,\ndoing a simple benchmark shows a ~3x hit.\n\nSo this is definitely nothing we want to do for all jobs. But for the\nlinux-TEST-vars job it might make sense, as it exercises a bunch of\nnon-default code paths.\n\n> The un-released rev_info has been there since the code was added in\n> fa2ab86d18 (format-patch: add '--base' option to record base tree info,\n> 2016-04-26), but back then we didn't even have a way to release rev_info\n> resources! The actual leak probably started around f0d9cc4196\n> (revision.c: begin refactoring --topo-order logic, 2018-11-01), but it's\n> hard to bisect because there were so many other unrelated leaks back\n> then.\n> \n> So I'm not sure exactly when the leak started beyond \"long ago\", but it\n> is easy-ish to find now (since we've plugged all those other leaks) and\n> the solution is clear.\n> \n> I didn't add a new test since we can demonstrate it with the existing\n> ones, but it does require tweaking a test variable. We might consider\n> ways to get more automatic leak-checking coverage there, but I think it\n> should be done outside of this fix.\n\nYeah, agreed.\n\nOne thing worth noting: there are still six test suites that are failing\nwith this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x\nfailures are because of the missing call to `repo_unuse_commit_buffer()`\nin git-history(1), which we already noted elsewhere.\n\nAll of the remaining leaks in t0095, t4013 and t4211 seem to be related\nto bloom filters.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/log.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index d027ce1e0b..350b35c556 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1888,6 +1888,7 @@ static void prepare_bases(struct base_tree_info *bases,\n>  \t\tbases->nr_patch_id++;\n>  \t}\n>  \tclear_commit_base(&commit_base);\n> +\trelease_revisions(&revs);\n>  }\n\nThe fix looks sensible to me. We always initialize `revs` before we take\nthis exit path here, and there is no other early return that we'd have\nto adjust.\n\nThanks!\n\nPatrick\n"},{"id":"546856","messageId":"20260701081358.GB813310@coredump.intra.peff.net","threadId":"65892","inReplyTo":"akOZy-BygZS8fqPM@pks.im","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-01T08:13:58Z","receivedAt":"2026-07-01T08:13:59Z","isPatch":true,"body":"On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:\n\n> >   make SANITIZE=leak\n> >   cd t\n> >   GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh\n> > \n> > which yields many entries like:\n> > \n> >   ==git==3687620==ERROR: LeakSanitizer: detected memory leaks\n> >   Direct leak of 200 byte(s) in 1 object(s) allocated from:\n> >       #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74\n> >       #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55\n> >       #2 0x55cd452cdd9d in xmalloc wrapper.c:76\n> >       #3 0x55cd45255473 in init_topo_walk revision.c:3845\n> >       #4 0x55cd45255bef in prepare_revision_walk revision.c:4017\n> >       #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872\n> >       #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439\n> \n> Interesting. Makes me wonder whether we should modify linux-TEST-vars to\n> also run with the leak checker enabled. Ideally we'd of course just do\n> this for all jobs, but the overhead is probably way too high... yes,\n> doing a simple benchmark shows a ~3x hit.\n> \n> So this is definitely nothing we want to do for all jobs. But for the\n> linux-TEST-vars job it might make sense, as it exercises a bunch of\n> non-default code paths.\n\nWe already run a special leak job for linux-reftables. Why not turn that\njob into \"leaks plus reftables plus test-vars\"? The only downside would\nbe potentially hiding leaks found by linux-reftables-leaks if the\ntest-vars features force us into a difference code path. But looking at\nthe list, it doesn't seem likely to me. None of them is particularly\nref-related.\n\nIn fact, I kind of wonder if we could fold linux-reftables into the\ntest-vars job completely.\n\n> One thing worth noting: there are still six test suites that are failing\n> with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x\n> failures are because of the missing call to `repo_unuse_commit_buffer()`\n> in git-history(1), which we already noted elsewhere.\n> \n> All of the remaining leaks in t0095, t4013 and t4211 seem to be related\n> to bloom filters.\n\nI sent some patches to fix the bloom-filter cases.\n\nBuilding with OPENSSL_SHA1_UNSAFE turns up more. The core issue is that\nrecent versions of openssl require an allocation to open a sha1 context,\nand we free it in git_hash_final(). So code paths that abort mid-hash\nwill leak the allocation, and we need a git_hash_discard().\n\nIt comes up mostly with csum-file.[ch], since that's where we use the\nunsafe variant.\n\nIf you further build with OPENSSL_SHA1 (using it for _all_ hash\ncomputations), there are a few more cases. It's hard to care too much\nsince that isn't a recommended build (and we've even discussed dropping\nsupport for non-dc sha1 totally). But sha256 has the same issue, so\nwe'll want to fix it eventually (I didn't try leak-checking the\nlinux-sha256 build, but I expect it would complain a lot).\n\nI have some patches but they need a bit of polish. In particular I think\nwe'll have to tweak the hash.h #define mess to expose a \"discard\"\nprimitive from each implementation (otherwise we have to finalize the\nhash to discard, which is a little inefficient). I didn't quite have the\nstomach for that tonight.\n\n-Peff\n"},{"id":"546858","messageId":"akTS_rPV7JaGHKRq@pks.im","threadId":"65892","inReplyTo":"20260701081358.GB813310@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-01T08:42:38Z","receivedAt":"2026-07-01T08:42:43Z","isPatch":true,"body":"On Wed, Jul 01, 2026 at 04:13:58AM -0400, Jeff King wrote:\n> On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:\n> \n> > >   make SANITIZE=leak\n> > >   cd t\n> > >   GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh\n> > > \n> > > which yields many entries like:\n> > > \n> > >   ==git==3687620==ERROR: LeakSanitizer: detected memory leaks\n> > >   Direct leak of 200 byte(s) in 1 object(s) allocated from:\n> > >       #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74\n> > >       #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55\n> > >       #2 0x55cd452cdd9d in xmalloc wrapper.c:76\n> > >       #3 0x55cd45255473 in init_topo_walk revision.c:3845\n> > >       #4 0x55cd45255bef in prepare_revision_walk revision.c:4017\n> > >       #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872\n> > >       #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439\n> > \n> > Interesting. Makes me wonder whether we should modify linux-TEST-vars to\n> > also run with the leak checker enabled. Ideally we'd of course just do\n> > this for all jobs, but the overhead is probably way too high... yes,\n> > doing a simple benchmark shows a ~3x hit.\n> > \n> > So this is definitely nothing we want to do for all jobs. But for the\n> > linux-TEST-vars job it might make sense, as it exercises a bunch of\n> > non-default code paths.\n> \n> We already run a special leak job for linux-reftables. Why not turn that\n> job into \"leaks plus reftables plus test-vars\"? The only downside would\n> be potentially hiding leaks found by linux-reftables-leaks if the\n> test-vars features force us into a difference code path. But looking at\n> the list, it doesn't seem likely to me. None of them is particularly\n> ref-related.\n> \n> In fact, I kind of wonder if we could fold linux-reftables into the\n> test-vars job completely.\n\nlinux-reftable or linux-reftable-leaks? I think it would certainly make\nsense to drop one of these and merge it into linux-TEST-vars. The\nlinux-reftable job doesn't provide any benefit over its -leak variant,\nso that would be the candidate I'd personally merge.\n\n> > One thing worth noting: there are still six test suites that are failing\n> > with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x\n> > failures are because of the missing call to `repo_unuse_commit_buffer()`\n> > in git-history(1), which we already noted elsewhere.\n> > \n> > All of the remaining leaks in t0095, t4013 and t4211 seem to be related\n> > to bloom filters.\n> \n> I sent some patches to fix the bloom-filter cases.\n\nI saw them already, thanks for your work here!\n\nPatrick\n"},{"id":"546861","messageId":"20260701084733.GA814472@coredump.intra.peff.net","threadId":"65892","inReplyTo":"akTS_rPV7JaGHKRq@pks.im","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-01T08:47:33Z","receivedAt":"2026-07-01T08:47:35Z","isPatch":true,"body":"On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:\n\n> > We already run a special leak job for linux-reftables. Why not turn that\n> > job into \"leaks plus reftables plus test-vars\"? The only downside would\n> > be potentially hiding leaks found by linux-reftables-leaks if the\n> > test-vars features force us into a difference code path. But looking at\n> > the list, it doesn't seem likely to me. None of them is particularly\n> > ref-related.\n> > \n> > In fact, I kind of wonder if we could fold linux-reftables into the\n> > test-vars job completely.\n> \n> linux-reftable or linux-reftable-leaks? I think it would certainly make\n> sense to drop one of these and merge it into linux-TEST-vars. The\n> linux-reftable job doesn't provide any benefit over its -leak variant,\n> so that would be the candidate I'd personally merge.\n\nBoth. Fold linux-reftable into linux-TEST-vars, and then drop\nlinux-reftable-leaks in favor of a new linux-TEST-vars-leaks.\n\n-Peff\n"},{"id":"546863","messageId":"akTXYoY7mSQUM33P@pks.im","threadId":"65892","inReplyTo":"20260701084733.GA814472@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-01T09:01:22Z","receivedAt":"2026-07-01T09:01:32Z","isPatch":true,"body":"On Wed, Jul 01, 2026 at 04:47:33AM -0400, Jeff King wrote:\n> On Wed, Jul 01, 2026 at 10:42:38AM +0200, Patrick Steinhardt wrote:\n> \n> > > We already run a special leak job for linux-reftables. Why not turn that\n> > > job into \"leaks plus reftables plus test-vars\"? The only downside would\n> > > be potentially hiding leaks found by linux-reftables-leaks if the\n> > > test-vars features force us into a difference code path. But looking at\n> > > the list, it doesn't seem likely to me. None of them is particularly\n> > > ref-related.\n> > > \n> > > In fact, I kind of wonder if we could fold linux-reftables into the\n> > > test-vars job completely.\n> > \n> > linux-reftable or linux-reftable-leaks? I think it would certainly make\n> > sense to drop one of these and merge it into linux-TEST-vars. The\n> > linux-reftable job doesn't provide any benefit over its -leak variant,\n> > so that would be the candidate I'd personally merge.\n> \n> Both. Fold linux-reftable into linux-TEST-vars, and then drop\n> linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.\n\nHm, okay. I guess that should be fine. Do we also want to do a similar\nthing for macOS and create a macos-TEST-vars job that exercises all of\nthis?\n\nAlso, while at it... I really think that job name is just plain awful.\nWhile at it, we might rename it to something more sensible like\n\"linux-changed-defaults\".\n\nPatrick\n"},{"id":"546971","messageId":"20260702085821.GC481298@coredump.intra.peff.net","threadId":"65892","inReplyTo":"akTXYoY7mSQUM33P@pks.im","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:58:21Z","receivedAt":"2026-07-02T08:58:22Z","isPatch":true,"body":"On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:\n\n> > > linux-reftable or linux-reftable-leaks? I think it would certainly make\n> > > sense to drop one of these and merge it into linux-TEST-vars. The\n> > > linux-reftable job doesn't provide any benefit over its -leak variant,\n> > > so that would be the candidate I'd personally merge.\n> > \n> > Both. Fold linux-reftable into linux-TEST-vars, and then drop\n> > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.\n> \n> Hm, okay. I guess that should be fine. Do we also want to do a similar\n> thing for macOS and create a macos-TEST-vars job that exercises all of\n> this?\n\nIt could be helpful if we expect the interaction of macOS and those\ntest-vars to be interesting, but I'm a bit skeptical. Most of them are\nabout feature selection. So I'm doubtful it would turn up anything\nuseful. But who knows.\n\nLikewise I find the dual clang/gcc jobs to be overkill. Compiling with\nboth is useful, as they have different warnings. But have we ever seen a\ncase where running the tests showed a different result with different\ncompilers?\n\nI dunno. I guess there is an argument for CI-maximalism; as long as the\njobs run in parallel and they're \"just\" CPU-minutes. But those minutes\neventually have a cost, and I'm not sure I've gotten useful data from\nmost of the jobs (i.e., failures that didn't also just happen somewhere\nelse).\n\nAnyway, that is all a big tangent/rant. Mostly I think it would be fine\nto cannibalize linux-reftable into linux-TEST-vars if we want to get\nmore coverage without increasing the CI cost.\n\nNote that I did find some leaks that would only be hit running\nlinux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But\nthat is getting super specific now (even if we ran linux-sha256 with\nleak detection, would we want to do it with openssl and not the default\nbackend)?\n\n> Also, while at it... I really think that job name is just plain awful.\n> While at it, we might rename it to something more sensible like\n> \"linux-changed-defaults\".\n\nYes please. Every time I see the all-caps TEST in the middle I think I'm\nhaving a stroke.\n\nchange-defaults is OK but not super descriptive. I might call it\nlinux-exotic-flags or something. That's not descriptive either, but is a\nlittle more fun.\n\n-Peff\n"},{"id":"546973","messageId":"akY4u02vdBkVqs7m@pks.im","threadId":"65892","inReplyTo":"20260702085821.GC481298@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-02T10:08:59Z","receivedAt":"2026-07-02T10:09:07Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 04:58:21AM -0400, Jeff King wrote:\n> On Wed, Jul 01, 2026 at 11:01:22AM +0200, Patrick Steinhardt wrote:\n> \n> > > > linux-reftable or linux-reftable-leaks? I think it would certainly make\n> > > > sense to drop one of these and merge it into linux-TEST-vars. The\n> > > > linux-reftable job doesn't provide any benefit over its -leak variant,\n> > > > so that would be the candidate I'd personally merge.\n> > > \n> > > Both. Fold linux-reftable into linux-TEST-vars, and then drop\n> > > linux-reftable-leaks in favor of a new linux-TEST-vars-leaks.\n> > \n> > Hm, okay. I guess that should be fine. Do we also want to do a similar\n> > thing for macOS and create a macos-TEST-vars job that exercises all of\n> > this?\n> \n> It could be helpful if we expect the interaction of macOS and those\n> test-vars to be interesting, but I'm a bit skeptical. Most of them are\n> about feature selection. So I'm doubtful it would turn up anything\n> useful. But who knows.\n> \n> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with\n> both is useful, as they have different warnings. But have we ever seen a\n> case where running the tests showed a different result with different\n> compilers?\n\nNot that I'd know of. As you say, I think it makes sense to use\ndifferent compilers in general. But I don't really think we need to have\nthis as a full \"compiler x tests\" matrix.\n\n> I dunno. I guess there is an argument for CI-maximalism; as long as the\n> jobs run in parallel and they're \"just\" CPU-minutes. But those minutes\n> eventually have a cost, and I'm not sure I've gotten useful data from\n> most of the jobs (i.e., failures that didn't also just happen somewhere\n> else).\n\nI'm certainly on board with reducing the test matrix a bit. I'm sure\nthat we can have a cleverer selection of jobs where we both have the\nsame test coverage as we have right now while running less jobs overall.\n\n> Anyway, that is all a big tangent/rant. Mostly I think it would be fine\n> to cannibalize linux-reftable into linux-TEST-vars if we want to get\n> more coverage without increasing the CI cost.\n\nYou got to start somewhere :)\n\n> Note that I did find some leaks that would only be hit running\n> linux-sha256 with a non-standard backend like OPENSSL_SHA256=1.  But\n> that is getting super specific now (even if we ran linux-sha256 with\n> leak detection, would we want to do it with openssl and not the default\n> backend)?\n> \n> > Also, while at it... I really think that job name is just plain awful.\n> > While at it, we might rename it to something more sensible like\n> > \"linux-changed-defaults\".\n> \n> Yes please. Every time I see the all-caps TEST in the middle I think I'm\n> having a stroke.\n\nHeh :P\n\n> change-defaults is OK but not super descriptive. I might call it\n> linux-exotic-flags or something. That's not descriptive either, but is a\n> little more fun.\n\nI certainly like it more than my suggestion.\n\nThanks!\n\nPatrick\n"},{"id":"547121","messageId":"xmqqjyrbhkf8.fsf@gitster.g","threadId":"65892","inReplyTo":"akY4u02vdBkVqs7m@pks.im","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-03T20:45:15Z","receivedAt":"2026-07-03T20:45:18Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with\n>> both is useful, as they have different warnings. But have we ever seen a\n>> case where running the tests showed a different result with different\n>> compilers?\n>\n> Not that I'd know of. As you say, I think it makes sense to use\n> different compilers in general. But I don't really think we need to have\n> this as a full \"compiler x tests\" matrix.\n\nVery true.  Different configurations with TEST-vars are great\ncombination to test, but we are not in the business of hunting bugs\nin clang/gcc so we long as they compile (instead of warning \"hey,\nthat construct gives you undefined behaviour\"), we shouldn't have to\nrun the test suite with the same configuration for both.\n\n> I'm certainly on board with reducing the test matrix a bit. I'm sure\n> that we can have a cleverer selection of jobs where we both have the\n> same test coverage as we have right now while running less jobs overall.\n\nYeah, and if we can spend the saved cycles for better coverage, that\nwould be grat.\n\n"},{"id":"547140","messageId":"CAOLa=ZRX9fGPdtUKXhmJF-zm_+F0zDuXSV+w02ZPEzqXY6n1Fw@mail.gmail.com","threadId":"65892","inReplyTo":"20260630063944.GA3733670@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] small leak fix in format-patch","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-07-04T21:13:52Z","receivedAt":"2026-07-04T21:13:54Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> This fixes a leak I found while discussing an unrelated leak in another\n> thread[1]. As a bonus, this fixes some minor recent breakage of\n> leak-reporting when running the test suite under prove. The patches can\n> be split into separate topics if we want.\n\nI think you meant to CC the other \"Kaartic\" :)\n"},{"id":"547171","messageId":"20260706000140.GB2301945@coredump.intra.peff.net","threadId":"65892","inReplyTo":"CAOLa=ZRX9fGPdtUKXhmJF-zm_+F0zDuXSV+w02ZPEzqXY6n1Fw@mail.gmail.com","subject":"Re: [PATCH 0/2] small leak fix in format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-06T00:01:40Z","receivedAt":"2026-07-06T00:01:41Z","isPatch":true,"body":"On Sat, Jul 04, 2026 at 02:13:52PM -0700, Karthik Nayak wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This fixes a leak I found while discussing an unrelated leak in another\n> > thread[1]. As a bonus, this fixes some minor recent breakage of\n> > leak-reporting when running the test suite under prove. The patches can\n> > be split into separate topics if we want.\n> \n> I think you meant to CC the other \"Kaartic\" :)\n\nYes, I figured that out about halfway through the discussion and hoped\nnobody might notice. ;) Sorry to both of you for the confusion.\n\n-Peff\n"},{"id":"547174","messageId":"20260706003429.GD2301945@coredump.intra.peff.net","threadId":"65892","inReplyTo":"xmqqjyrbhkf8.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-06T00:34:29Z","receivedAt":"2026-07-06T00:34:30Z","isPatch":true,"body":"On Fri, Jul 03, 2026 at 01:45:15PM -0700, Junio C Hamano wrote:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with\n> >> both is useful, as they have different warnings. But have we ever seen a\n> >> case where running the tests showed a different result with different\n> >> compilers?\n> >\n> > Not that I'd know of. As you say, I think it makes sense to use\n> > different compilers in general. But I don't really think we need to have\n> > this as a full \"compiler x tests\" matrix.\n> \n> Very true.  Different configurations with TEST-vars are great\n> combination to test, but we are not in the business of hunting bugs\n> in clang/gcc so we long as they compile (instead of warning \"hey,\n> that construct gives you undefined behaviour\"), we shouldn't have to\n> run the test suite with the same configuration for both.\n\nI don't care about finding bugs in clang vs gcc. I'm more concerned with\na case where we have undefined behavior, both compile it fine, but the\nbad behavior is revealed in the tests only by one of them.\n\nI can think offhand of only one case where I saw that happen[1]. IIRC it\nhad to do with integer sizes being passed to a variadic function. But it\nalso changed behavior within the same compiler using different\noptimization levels. So it feels like kind of a scattershot way of\ntrying to flush out UB, and we are probably better off with UBSan and\nfriends.\n\n-Peff\n\n[1] I mentioned it in:\n\n      https://lore.kernel.org/git/20251130134625.GA199421@coredump.intra.peff.net/\n\n    but didn't give enough details for it to be useful here. I mention\n    it merely as the only anecdote I could call to mind. :)\n"},{"id":"547184","messageId":"aktD0fioUvyebhOY@pks.im","threadId":"65892","inReplyTo":"20260706003429.GD2301945@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-06T05:57:37Z","receivedAt":"2026-07-06T05:57:45Z","isPatch":true,"body":"On Sun, Jul 05, 2026 at 08:34:29PM -0400, Jeff King wrote:\n> On Fri, Jul 03, 2026 at 01:45:15PM -0700, Junio C Hamano wrote:\n> \n> > Patrick Steinhardt <ps@pks.im> writes:\n> > \n> > >> Likewise I find the dual clang/gcc jobs to be overkill. Compiling with\n> > >> both is useful, as they have different warnings. But have we ever seen a\n> > >> case where running the tests showed a different result with different\n> > >> compilers?\n> > >\n> > > Not that I'd know of. As you say, I think it makes sense to use\n> > > different compilers in general. But I don't really think we need to have\n> > > this as a full \"compiler x tests\" matrix.\n> > \n> > Very true.  Different configurations with TEST-vars are great\n> > combination to test, but we are not in the business of hunting bugs\n> > in clang/gcc so we long as they compile (instead of warning \"hey,\n> > that construct gives you undefined behaviour\"), we shouldn't have to\n> > run the test suite with the same configuration for both.\n> \n> I don't care about finding bugs in clang vs gcc. I'm more concerned with\n> a case where we have undefined behavior, both compile it fine, but the\n> bad behavior is revealed in the tests only by one of them.\n> \n> I can think offhand of only one case where I saw that happen[1]. IIRC it\n> had to do with integer sizes being passed to a variadic function. But it\n> also changed behavior within the same compiler using different\n> optimization levels. So it feels like kind of a scattershot way of\n> trying to flush out UB, and we are probably better off with UBSan and\n> friends.\n\nYes, I just wanted to say that UBSan is definitely the better way to go\nin this context. I have no idea of course whether it would have catched\nthe mentioned issue, though.\n\nAnd even so, we'd have at least one job that runs all tests with either\nof the compilers, so we'd still notice issues like that.\n\nPatrick\n"}]}