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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2007, 08:47 UTC
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.

Show 6 quoted lines

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