From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 16 Sep 2007 08:47:13 GMT
Subject: Re: [PATCH 1/3] rev-list --bisect: Move finding bisection into do_find_bisection.
Message-ID: <7vk5qr575a.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20070915125957.0899841b.chriscool@tuxfamily.org>

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.