Re: [PATCH] commit: fix "author_ident" leak

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

On Thu, May 12 2022, Junio C Hamano wrote:

> Since 4c28e4ada03 (commit: die before asking to edit the log
> message, 2010-12-20), we have been "leaking" the "author_ident" when
> prepare_to_commit() fails.  Instead of returning from right there,
> introduce an exit status variable and jump to the clean-up label
> at the end.
>
> Instead of explicitly releasing the resource with strbuf_release(),
> mark the variable with UNLEAK() at the end, together with two other
> variables that are already marked as such.  If this were in a
> utility function that is called number of times, but these are
> different, we should explicitly release resources that grow
> proportionally to the size of the problem being solved, but
> cmd_commit() is like main() and there is no point in spending extra
> cycles to release individual pieces of resource at the end, just
> before process exit will clean everything for us for free anyway.
>
> This fixes a leak demonstrated by e.g. "t3505-cherry-pick-empty.sh",
> but unfortunately we cannot mark it or other affected tests as passing
> now with "TEST_PASSES_SANITIZE_LEAK=true" as we'll need to fix many
> other memory leaks before doing so.
>
> Incidentally there are two tests that always passes the leak checker
> with or without this change.  Mark them as such.
>
> This is based on an earlier patch by Ævar, but takes a different
> approach that is more maintainable.

We've talked about UNLEAK() v.s. strbuf_release() elsewhere, so let's
leave that aside. I know your preferences in that area.

But even accounting for that, I don't see what the "more maintainable"
here refers to. The approach I suggested would s/UNLEAK/strbuf_release/
in the 4th hunk, but otherwise be equivalent.

Surely both are about as easy to maintain. To the extent that there's
any difference at all I'd think the strbuf_release() would pull ahead,
as it's guaranteed to do the right thing with all of our memory analysis
tooling (some of which will have a noop UNLEAK()).

Just a small question, I see this is in "next" already, and I'm fine
with this change either way.