* [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
@ 2007-09-15 10:59 Christian Couder
2007-09-16 8:47 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Christian Couder @ 2007-09-15 10:59 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
This factorises some code and make a big function smaller.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
2007-09-15 10:59 [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection Christian Couder
@ 2007-09-16 8:47 ` Junio C Hamano
2007-09-17 3:19 ` Christian Couder
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2007-09-16 8:47 UTC (permalink / raw)
To: Christian Couder; +Cc: git
Christian Couder <chriscool@tuxfamily.org> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
2007-09-16 8:47 ` Junio C Hamano
@ 2007-09-17 3:19 ` Christian Couder
0 siblings, 0 replies; 4+ messages in thread
From: Christian Couder @ 2007-09-17 3:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
@ 2007-09-17 3:28 Christian Couder
0 siblings, 0 replies; 4+ messages in thread
From: Christian Couder @ 2007-09-17 3:28 UTC (permalink / raw)
To: Junio Hamano; +Cc: git
This factorises some code and make a big function smaller.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-rev-list.c | 90 +++++++++++++++++++++++++++------------------------
1 files changed, 48 insertions(+), 42 deletions(-)
This patch series is a resend with the changes Junio asked for.
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index ac551d5..2dae287 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -268,39 +268,12 @@ static void show_list(const char *debug, int counted, int nr,
* unknown. After running count_distance() first, they will get zero
* or positive distance.
*/
-
-static struct commit_list *find_bisection(struct commit_list *list,
- int *reaches, int *all)
+static struct commit_list *do_find_bisection(struct commit_list *list,
+ int nr, int *weights)
{
- int n, nr, on_list, counted, distance;
- struct commit_list *p, *best, *next, *last;
- int *weights;
-
- show_list("bisection 2 entry", 0, 0, list);
-
- /*
- * Count the number of total and tree-changing items on the
- * list, while reversing the list.
- */
- for (nr = on_list = 0, last = NULL, p = list;
- p;
- p = next) {
- unsigned flags = p->item->object.flags;
+ int n, counted, distance;
+ struct commit_list *p, *best;
- next = p->next;
- if (flags & UNINTERESTING)
- continue;
- p->next = last;
- last = p;
- if (!revs.prune_fn || (flags & TREECHANGE))
- nr++;
- on_list++;
- }
- list = last;
- show_list("bisection 2 sorted", 0, nr, list);
-
- *all = nr;
- weights = xcalloc(on_list, sizeof(*weights));
counted = 0;
for (n = 0, p = list; p; p = p->next) {
@@ -357,12 +330,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 +369,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 +390,53 @@ static struct commit_list *find_bisection(struct commit_list *list,
if (distance > counted) {
best = p;
counted = distance;
- *reaches = weight(p);
}
}
+ return best;
+}
+
+static struct commit_list *find_bisection(struct commit_list *list,
+ int *reaches, int *all)
+{
+ int nr, on_list;
+ struct commit_list *p, *best, *next, *last;
+ int *weights;
+
+ show_list("bisection 2 entry", 0, 0, list);
+
+ /*
+ * Count the number of total and tree-changing items on the
+ * list, while reversing the list.
+ */
+ for (nr = on_list = 0, last = NULL, p = list;
+ p;
+ p = next) {
+ unsigned flags = p->item->object.flags;
+
+ next = p->next;
+ if (flags & UNINTERESTING)
+ continue;
+ p->next = last;
+ last = p;
+ if (!revs.prune_fn || (flags & TREECHANGE))
+ nr++;
+ on_list++;
+ }
+ list = last;
+ show_list("bisection 2 sorted", 0, nr, 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;
}
--
1.5.3.1.59.g93705
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-09-17 3:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-09-15 10:59 [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection Christian Couder
2007-09-16 8:47 ` Junio C Hamano
2007-09-17 3:19 ` Christian Couder
-- strict thread matches above, loose matches on Subject: below --
2007-09-17 3:28 Christian Couder
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).