From: Johannes Schindelin Date: Wed, 18 Apr 2018 20:31:26 GMT Subject: Re: [PATCH v3] bisect: create 'bisect_flags' parameter in find_bisection() Message-ID: In-Reply-To: <20180415134925.6203-1-haraldnordgren@gmail.com> 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 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 ;-)