Re: [PATCH 2/2] format-patch: free elements of rev.ref_message_ids list

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

Jeff King <peff@peff.net> writes:

> When we are showing multiple patches with format-patch, we have to
> repeatedly overwrite the rev.message_id field. We take care to avoid
> leaking the old value by either freeing it, or adding it to
> ref_message_ids, a string list of ids to reference in subsequent
> messages.
>
> But unfortunately we do leak the value via that string list. We try
> to clear the string list, courtesy of 89f45cf4eb (format-patch: don't
> leak "extra_headers" or "ref_message_ids", 2022-04-13). But since it was
> initialized as "nodup", the string list doesn't realize it owns the
> strings, and it leaks them.
>
> We have two options here:
>
>   1. Continue to init with "nodup", but then tweak the value of
>      ref_message_ids.strdup_strings just before clearing.
>
>   2. Init with "dup", but use "append_nodup" when transferring ownership
>      of strings to the list. Clearing just works.
>
> I picked the second here, as I think it calls attention to the tricky
> part (transferring ownership via the nodup call).

This is pretty much what I had in mind wheh I wrote the log message
for the follow-up "Yikes, for now let's mark the test no longer
sanitizer clean" patch.  Very pleased to see a clean-up I punted
materialize while I was looking the other way ;-)

> There's one other related fix we have to do, though. We also insert the
> result of clean_message_id() into the list. This _sometimes_ allocates
> and sometimes does not, depending on whether we have to remove cruft
> from the end of the string. Let's teach it to consistently return an
> allocated string, so that the caller knows it must be freed.

Yes!

Thanks.