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.
