Re: [PATCH 0/2] Squash leaks in t0000

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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[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@gmail.com/
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@evledraar.gmail.com/