From: "Ævar Arnfjörð Bjarmason" <firstname.lastname@example.org> To: Andrzej Hunt via GitGitGadget <email@example.com> Cc: firstname.lastname@example.org, Carlo Arenas <email@example.com>, Andrzej Hunt <firstname.lastname@example.org> Subject: Re: [PATCH 0/2] Squash leaks in t0000 Date: Sun, 19 Sep 2021 12:58:16 +0200 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> On Sat, Sep 18 2021, Andrzej Hunt via GitGitGadget wrote: > Carlo points out that t0000 currently doesn't pass with leak-checking > enabled in: > https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71 > > Here's a series that I've sat on for a while, which adds some UNLEAK's to > "fix" this situation - see the individual patches for a justification of why > an UNLEAK seems appropriate. > > ATB, Andrzej > > Andrzej Hunt (2): > log: UNLEAK rev to silence a large number of leaks > log: UNLEAK original pending objects > > builtin/log.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) I sent a re-roll of that series that bypasses the issue by no longer running t0000-basic.sh, so there won't be an immediate need for a fixup series like this. As for these patches & approach, I think that these unleak fixes that just narrowly squash some failure in a specific test aren't worth doing, and are actually counter-productive. We should instead eventually fix the leaks more generally and make the built-ins use those APIs. Maybe our differing approaches there are because we've got different end-goals in mind. My end-goal is three-fold: A. Make git's core APIs nicer, in most cases that we're not freeing memory is a result of a rather messy API that's not quite sure who should be managing its memory. This usually makes using it correctly harder in other ways. B. Make those APIs not leak memory, so we can use them as libraries. C. Have regression tests testing [*B*] Given that, I think that fixing memory leaks in built-in when we're about to exit is completely pointless as a goal in itself. We're about to exit anyway, why care that we're leaking memory? The only reason, I think, is that we're doing it as a proxy to get to a combination of [*A*] and [*B*] above. Once we know that we can run "git log" in various modes without it leaking, it's likely that most or all of the revisions walking API, refname resolution, object lookup etc. isn't leaking. I have a WIP branch that would obsolete this, see the commit at its tip. As shown there you're fixing a leak in cmd_show(), but omit the same leak in its sister functions. At that point we won't need these UNLEAK(), and as a follow-up any concerns about spending too much time in a built-in just to clean up could rather easily be done with something like a GIT_DESTRUCT_LEVEL, i.e. we'd conditionally skip the freeing in some cases. I'm not saying that there's no point in adding UNLEAK() somewhere, but I really don't see it in this case. We didn't *need* to mark t0000-basic.sh as leak-free right away, I just did so because it was the first test, and I naïvely thought it would stay that way while my series cooked. I'd think that when building on top of my SANITIZE=leak series you'd want instead of UNLEAK() to instead label the test as TEST_PASSES_SANITIZE_LEAK=true, but just omit some specific breakages with a use of the "SANITIZE_LEAK" prerequisite. Maybe there's cases where you'd want to use TEST_PASSES_SANITIZE_LEAK=true, but the leak is so deep in the guts of some API that a transitional UNLEAK() is worth it, *and* you can't just mark some other test that mostly tests the command you're interested in with TEST_PASSES_SANITIZE_LEAK=true. But so far I haven't seen such cases, e.g. there's cases where "git tag" leaks in obscure cases, but not in some common cases with some of my preliminary fixes. In that state I can usually find a test that uses "git tag" in some way and mark that as TEST_PASSES_SANITIZE_LEAK=true, instead of sprinkling UNLEAK() in builtin/tag.c just so I can mark the main "git tag" test as passing. 1. 62833https://lore.kernel.org/git/cover-v7-0.2-00000000000-20210919T075619Zemail@example.com/ 2. https://github.com/git/git/compare/master...avar:avar/tests-post-add-sanitize-leak-test-mode-fix-leaks 3. https://firstname.lastname@example.org/
next prev parent reply other threads:[~2021-09-19 12:20 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-18 13:49 Andrzej Hunt via GitGitGadget 2021-09-18 13:49 ` [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks Andrzej Hunt via GitGitGadget 2021-09-18 20:06 ` Carlo Marcelo Arenas Belón 2021-09-19 15:51 ` Andrzej Hunt 2021-09-19 16:13 ` Ævar Arnfjörð Bjarmason 2021-09-19 21:34 ` Carlo Marcelo Arenas Belón 2021-09-20 6:06 ` Eric Sunshine 2021-09-20 21:39 ` Carlo Marcelo Arenas Belón 2021-09-21 3:09 ` Jeff King 2021-09-18 13:49 ` [PATCH 2/2] log: UNLEAK original pending objects Andrzej Hunt via GitGitGadget 2021-09-18 17:28 ` [PATCH 0/2] Squash leaks in t0000 Carlo Arenas 2021-09-19 15:38 ` Andrzej Hunt 2021-09-19 10:58 ` Ævar Arnfjörð Bjarmason [this message] 2021-09-20 17:55 ` Junio C Hamano 2021-09-21 23:01 ` Ævar Arnfjörð Bjarmason
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 0/2] Squash leaks in t0000' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).