# [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection. 3 messages from 2007-09-15 to 2007-09-17. Participants: Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/9863 ## Christian Couder, 2007-09-15 10:59 Subject: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection. Message-ID: <20070915125957.0899841b.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20070915125957.0899841b.chriscool%40tuxfamily.org ``` This factorises some code and make a big function smaller. Signed-off-by: Christian Couder --- builtin-rev-list.c | 40 +++++++++++++++++++++++++--------------- 1 files changed, 25 insertions(+), 15 deletions(-) diff --git a/builtin-rev-list.c b/builtin-rev-list.c index ac551d5..f32c712 100644 --- a/builtin-rev-list.c +++ b/builtin-rev-list.c @@ -255,6 +255,9 @@ static void show_list(const char *debug, int counted, int nr, } #endif /* DEBUG_BISECT */ +static struct commit_list *do_find_bisection(struct commit_list *list, + int nr, int *weights); + /* * zero or positive weight is the number of interesting commits it can * reach, including itself. Especially, weight = 0 means it does not @@ -272,7 +275,7 @@ static void show_list(const char *debug, int counted, int nr, static struct commit_list *find_bisection(struct commit_list *list, int *reaches, int *all) { - int n, nr, on_list, counted, distance; + int nr, on_list; struct commit_list *p, *best, *next, *last; int *weights; @@ -301,6 +304,25 @@ static struct commit_list *find_bisection(struct commit_list *list, *all = nr; weights = xcalloc(on_list, sizeof(*weights)); + + /* Do the real work of finding bisection commit. */ + best = do_find_bisection(list, nr, weights); + + if (best) + best->next = NULL; + + *reaches = weight(best); + free(weights); + + return best; +} + +static struct commit_list *do_find_bisection(struct commit_list *list, + int nr, int *weights) +{ + int n, counted, distance; + struct commit_list *p, *best; + counted = 0; for (n = 0, p = list; p; p = p->next) { @@ -357,12 +379,8 @@ static struct commit_list *find_bisection(struct commit_list *list, weight_set(p, distance); /* Does it happen to be at exactly half-way? */ - if (halfway(p, distance, nr)) { - p->next = NULL; - *reaches = distance; - free(weights); + if (halfway(p, distance, nr)) return p; - } counted++; } @@ -400,12 +418,8 @@ static struct commit_list *find_bisection(struct commit_list *list, /* Does it happen to be at exactly half-way? */ distance = weight(p); - if (halfway(p, distance, nr)) { - p->next = NULL; - *reaches = distance; - free(weights); + if (halfway(p, distance, nr)) return p; - } } } @@ -425,12 +439,8 @@ static struct commit_list *find_bisection(struct commit_list *list, if (distance > counted) { best = p; counted = distance; - *reaches = weight(p); } } - if (best) - best->next = NULL; - free(weights); return best; } -- 1.5.3.1.20.g2a16-dirty ``` ## Junio C Hamano, 2007-09-16 08:47 Subject: Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection. Message-ID: <7vk5qr575a.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk5qr575a.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20070915125957.0899841b.chriscool@tuxfamily.org> ``` Christian Couder writes: > This factorises some code and make a big function smaller. I think the refactoring itself makes sense, especially where it simplifies the clean-up of weight array in early-return codepath. But I have a couple of comments, though. > +static struct commit_list *do_find_bisection(struct commit_list *list, > + int nr, int *weights); > + > /* > * zero or positive weight is the number of interesting commits it can > * reach, including itself. Especially, weight = 0 means it does not The comment whose top part we can see here talks about the magic values -1 and -2 used while do_find_bisection() after the refactoring does its work, and these magic values are never visible to the calling function. You should move the comment to the top of do_find_bisection() as well. Also this forward declaration is unwarranted. A bottom-up sequence to define do_find_bisection() first, then to define its sole caller find_bisection() next is easier to read at least for me. The latter comment also applies to your other patch. ``` ## Christian Couder, 2007-09-17 03:19 Subject: Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection. Message-ID: <200709170519.19504.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/200709170519.19504.chriscool%40tuxfamily.org In-Reply-To: <7vk5qr575a.fsf@gitster.siamese.dyndns.org> ``` Le dimanche 16 septembre 2007, Junio C Hamano a écrit : > > +static struct commit_list *do_find_bisection(struct commit_list *list, > > + int nr, int *weights); > > + > > /* > > * zero or positive weight is the number of interesting commits it can > > * reach, including itself. Especially, weight = 0 means it does not > > The comment whose top part we can see here talks about the magic > values -1 and -2 used while do_find_bisection() after the > refactoring does its work, and these magic values are never > visible to the calling function. You should move the comment to > the top of do_find_bisection() as well. > > Also this forward declaration is unwarranted. A bottom-up > sequence to define do_find_bisection() first, then to define its > sole caller find_bisection() next is easier to read at least for > me. > > The latter comment also applies to your other patch. All right, I will send new patchs with these changes. Thanks, Christian. ```