Re: [PATCH v2 1/3] bisect: add parameters to "filter_skipped"

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2009, 06:48 UTC
Message-ID: <7vtz2vp45e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090605041044.8885.91204.chriscool@tuxfamily.org>

Christian Couder <chriscool@tuxfamily.org> writes:

Show 12 quoted lines

> because we will need to get more information from this function in
> some later patches.
>
> The new "int *count" parameter gives the number of commits left after
> the skipped commit have been filtered out.
>
> The new "int *skipped_first" parameter tells us if the first commit
> in the list has been skipped. Note that using this parameter also
> changes the behavior of the function if the first commit is indeed
> skipped. Because we assume that in this case we will want all the
> filtered commits, not just the first one, even if "show_all" is not
> set.

The way you use (*skipped_first == -1) as a marker to mean "we've already checked more than one commit_list so even when we see a one to be skipped, it won't be the first one" is unreadable, especially without explanation. Worse yet, the above paragraph explains what the parameter does, but why is it so special to skip the one that happens to be the first on the input list, especially when one does not know how the list is sorted to begin with.

I understand that the list is sorted by the "goodness" value, i.e. the one that cuts the graph into closer-to-equal halves comes earlier in the list, but still it is unclear why having to skip the best one is so special compared to having to skip say the second best one, especially when you imagine a case where the first two on the list are of equal "goodness" value.