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 <[email protected]> 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 ;-)
