Re: [PATCH v3] bisect: create 'bisect_flags' parameter in find_bisection()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Harald,

On Sun, 15 Apr 2018, Harald Nordgren wrote:

> Make it possible to implement bisecting only on first parents or on
> merge commits by passing flags to find_bisection(), instead of just
> a 'find_all' boolean.
> 
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>

Very nice. Just two little suggestions:

> diff --git a/bisect.c b/bisect.c
> index a579b50884..19dac7491d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -254,7 +254,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>   */
>  static struct commit_list *do_find_bisection(struct commit_list *list,
>  					     int nr, int *weights,
> -					     int find_all)
> +					     unsigned int bisect_flags)

For flags, we frequently seem to use the type `unsigned` (which is the
same as `unsigned int`, just shorter).

>  {
>  	int n, counted;
>  	struct commit_list *p;
> @@ -313,7 +313,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		clear_distance(list);
>  
>  		/* Does it happen to be at exactly half-way? */
> -		if (!find_all && halfway(p, nr))
> +		if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))

Rather than expanding the short & sweet `find_all` to `(bisect_flags &
BISECT_FIND_ALL)` in three places in this function, I would rather just
define this local variable in the beginning:

	unsigned int find_all = bisect_flags & BISECT_FIND_ALL;

This would also reduce the size of the diff.

All in all, very nice!

Ciao,
Johannes

P.S.: I fully agree with Junio and eagerly await what comes next ;-)