Re: [PATCH v2 07/27] bisect: fix various cases where we leak commit list items
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Nov 20, 2024, 12:41 UTC
- Message-ID
- <Zz3Y9xJ-xQvBNNdm@pks.im>
- In-Reply-To
- <875xoitcu8.fsf@iotcl.com>
On Wed, Nov 20, 2024 at 11:32:31AM +0100, Toon Claes wrote:
Show 36 quoted lines
> 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.
Show 8 quoted lines
> 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