ab/commit-plug-leaks (was: What's cooking in git.git (Mar 2022, #05; Wed, 23))

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 25, 2022, 13:38 UTC
Message-ID: <220325.86v8w2kurx.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqwngkm4am.fsf@gitster.g>

On Wed, Mar 23 2022, Junio C Hamano wrote:

Show 9 quoted lines

> * ab/commit-plug-leaks (2022-02-16) 2 commits
>  - commit: use strbuf_release() instead of UNLEAK()
>  - commit: fix "author_ident" leak
>
>  Leakfixes in the top-level called-once function.
>
>  Expecting a reroll.
>  I think UNLEAK->strbuf_release() is a regression.
>  source: <cover-0.2-00000000000-20220216T081844Z-avarab@gmail.com>

Re our earlier exchange ending in https://lore.kernel.org/git/xmqqczjbj0nf.fsf@gitster.g/ I still think it makes sense to get rid of the UNLEAK() there.

One reason not mentioned there (but which I do find useful) is that if we actually free the memory then you don't need to build with -DSUPPRESS_ANNOTATED_LEAKS to suppress these, which is useful e.g. when looking at valgrind output, as opposed to SANITIZE=leak where we do add -DSUPPRESS_ANNOTATED_LEAKS.

And since I submitted this topic our number of UNLEAK() in builtin/ is down from 29 to 23 with the queued release_revisions() topic.

But you seem to feel strongly that we should keep these specific UNLEAK() for reasons I don't really understand despite re-reading that exchange. I.e. the cost of doing the actual release is minuscule, and we have a lot of such strbuf_release() in builtin/*.c already.

But if you'd just like to drop this topic I understand, but I think it's ready to advance as is, but either way it's probably good to get it out of the current status