Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.

Christian Couder [off-list ref] 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.

Le dimanche 16 septembre 2007, Junio C Hamano a écrit :

quoted

+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.