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

[Toon Claes <toon@iotcl.com>]

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?

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? 

--
Toon