Re: [PATCH 1/3] bisect: fix memory leak and document `find_bisection()`

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 2, 2017, 10:47 UTC
Message-ID: <CAN0heSoBeG_8zLwS-71_ZVBF-HsVGKY1W1QiTSQ_ybwWCPauog@mail.gmail.com>
In-Reply-To: <xmqqr2thqnzw.fsf@gitster.mtv.corp.google.com>

On 2 November 2017 at 05:54, Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Martin Ågren <martin.agren@gmail.com> writes:
>
>> `find_bisection()` rebuilds the commit list it is given by reversing it
>> and skipping uninteresting commits. The uninteresting list entries are
>> leaked. Free them to fix the leak.
>>
>> While we're here and understand what's going on, document the function.
>> In particular, make sure to document that the original list should not
>> be examined by the caller.
>
> Good.  Thanks.
>
> I notice that this has only two callers and both of them do
>
>         revs.commits = find_bisection(revs.commits, ...);
>
> I wonder if updating its calling convention to
>
>         (void) find_bisection(&revs.commits, ...);
>
> makes sense.  This is obviously outside the scope of this patch.

I think it would. I considered it briefly but punted, though I don't really understand why now. In hindsight, it would have saved me some work, since I wouldn't have had to carefully document that the original list mustn't be examined. I'll let this simmer for a few days in case other comments turn up, then do a v2 with a preparatory patch that switches the calling convention as you suggested.

Thanks.