Re: [PATCH v3] bisect: create 'bisect_flags' parameter in find_bisection()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Apr 18, 2018, 20:31 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1804182228210.4241@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
- In-Reply-To
- <20180415134925.6203-1-haraldnordgren@gmail.com>
Hi Harald,
On Sun, 15 Apr 2018, Harald Nordgren wrote:
Show 5 quoted lines
> 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:
Show 10 quoted lines
> 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).
Show 9 quoted lines
> {
> 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 ;-)