Re: [PATCH 15/22] sequencer: release todo list on error paths

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2024, 16:31 UTC
Message-ID: <xmqqmslnht7o.fsf@gitster.g>
In-Reply-To: <e0685657-369f-4211-8518-cc41aa28dcb9@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 10 quoted lines

> Hi Patrick
>
> On 06/08/2024 10:00, Patrick Steinhardt wrote:
>> We're not releasing the `todo_list` in `sequencer_pick_revisions()` when
>> hitting an error path. Restructure the function to have a common exit
>> path such that we can easily clean up the list and thus plug this memory
>> leak.
>
> This looks good, I've left a couple of small formatting comments below
> if you do end up re-rolling.

Oh, formatting nitpicks, my favourite ;-)

>> @@ -5506,11 +5508,14 @@ int sequencer_pick_revisions(struct repository *r,
>>   				enum object_type type = oid_object_info(r,
>>   									&oid,
>>   									NULL);

Also, if we say

				enum object_type type;

				type = oid_object_info(r, &oid, NULL);

the result is much easier on the eyes usign the same three lines. Yes, initializing while declaring may look nicer and in some cases it may even be necessary, but not this one.

Show 5 quoted lines

>> -				return error(_("%s: can't cherry-pick a %s"),
>> +				res = error(_("%s: can't cherry-pick a %s"),
>>   					name, type_name(type));
>
> This line needs re-indenting to match the changes above.

Thanks.