Re: [PATCH] format-patch: unleak "-v <num>"

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Sun, Jan 15, 2023 at 12:03:39AM -0800, Junio C Hamano wrote:
>
>> The "subject_prefix" member of "struct revision" usually is always
>> set to a borrowed string (either a string literal like "PATCH" that
>> appear in the program text as a hardcoded default, or the value of
>> "format.subjectprefix") and is never freed when the containing
>> revision structure is released.  The "-v <num>" codepath however
>> violates this rule and stores a pointer to an allocated string to
>> this member, relinquishing the responsibility to free it when it is
>> done using the revision structure, leading to a small one-time leak.
>> 
>> Instead, keep track of the string it allocates to let the revision
>> structure borrow, and clean it up when it is done.
>
> FWIW, this looks obviously correct to me.
>
> The word "unleak" in the subject made me think about UNLEAK(), so this
> is a small tangent. This is exactly the kind of case that I designed
> UNLEAK() for, because the solution really is "while you are assigning to
> X, also keep a copy of the pointer in Y to be freed later".

Yup.  I was originally planning to use UNLEAK(), but it felt ugly to
UNLEAK(rev.subject_prefix), as it stores borrowed pointer sometimes
and owned pointer some other times, which is the exact reason why I
started looking for a clean way to plug this leak.  So I ended up
with declaring that the member should only store a borrowed pointer.

> And UNLEAK() is just "keep a copy of the pointer in Y to know that we
> _could_ free it later". And of course "do nothing if we are not
> leak-detecting". But since we seem to be moving away from UNLEAK(), and
> since it would not even save any lines here, I'm perfectly happy with
> this solution.

The first sentence needs to be rephrased, as it does not make much
sense to have something usually be X and always be X at the same
time (I'd just remove "always" from there).

Thanks.