Re: [PATCH v2 07/27] bisect: fix various cases where we leak commit list items

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 20, 2024 at 11:32:31AM +0100, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > There are various cases where we leak commit list items because we
> > evict items from the list, but don't free them. Plug those.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  bisect.c                    | 35 +++++++++++++++++++++++++++--------
> >  t/t6030-bisect-porcelain.sh |  1 +
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/bisect.c b/bisect.c
> > index 12efcff2e3c1836ab6e63d36e4d42269fbcaeaab..0e804086cbf6a02d42bda98b62fb86daf099b82d 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -440,11 +440,19 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
> >  			free_commit_list(list->next);
> >  			best = list;
> >  			best->next = NULL;
> > +		} else {
> > +			for (p = list; p != best; p = next) {
> > +				next = p->next;
> > +				free(p);
> > +			}
> 
> This makes the code do:
> 
>     if (!something) {
>         // ...
>     } else {
>         // ...
>     }
> 
> I find it odd reading code like that. Would you mind swapping them
> around? Or is there a reason we type it like this, because I see this
> also being done like this around line 393?

I mostly did it this way round to minimize the diff.

> More on the functional side of this code, I'd like to understand better
> what's happening here.
> It's clear to me when `best` is NULL we want to free the whole
> commit_list. And when `best` is set with FIND_BISECTION_ALL flag set, we
> want to free the commit_list up to `best`. But when the
> FIND_BISECTION_ALL flag is not set, we want to do the opposite, free
> from `best->next` till the end? But what has this to do with
> FIND_BISECTION_ALL? 

Good question.

Typically, `find_bisection()` will only return a single commit, which is
the next bisection point in the range of bisected commit. This is done
by approximation, where we want to find a commit that is roughly half
way between the current good and bad commits. So the result should be a
single commit, only, and thus we free all other commit items except for
the commit that we have determined to be the bisection point.

When `FIND_BISECTION_ALL` is set this changes: instead of returning a
single commit, we return all commits in the bisected range of commits,
but tag them with the distance to the included and excluded commits. So
the result is a list of commits, but that list may have been filtered
down from the list of commits passed into `do_find_bisection()`.

So when do we filter items? This happens in `best_bisection_sorted()`,
where we skip over commits which are treesame. But for all I can see we
never end up skipping over the head of the list, we may only exclude
times from its tail, and we do know to release the tail alright.

Which means... that the added code is effectively a no-op because `best`
will always be pointing to `list`. Removing the code and re-running with
the leak checker enabled confirms that there are no new leaks. I'll drop
it, no idea what I saw here.

Patrick