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@xxxxxxxxxxxxxx/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[1] 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[2], 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[3], 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-20210919T075619Z-avarab@xxxxxxxxx/ 2. https://github.com/git/git/compare/master...avar:avar/tests-post-add-sanitize-leak-test-mode-fix-leaks 3. https://lore.kernel.org/git/87y2bi0vvl.fsf@xxxxxxxxxxxxxxxxxxx/
