Hi,
On Sun, 13 Jul 2008, Christian Couder wrote:
> [PATCH] bisect: check all merge bases instead of only one
It would have been so much nicer to squash the two patches into one, as we
generally frown upon "let's submit one patch that we know is faulty, and
then another that fixes it". That's so CVS/SVN.
> @@ -384,19 +383,21 @@ check_merge_bases() {
> _skip="$3"
> for _g in $_good; do
I really wonder if we can be blessed with less ugly variable names. Maybe
some that do not start with an underscore for no apparent reason, and
maybe some that are longer than one letter, so that you have a chance to
understand later what it is supposed to contain. I.e. something like:
for good in $good_revisions
do
(You see that I also broke up the "for" and "do" into two lines, as is
common practice in the rest of Git's shell code.)
> is_merge_base_ok "$_bad" "$_g" && continue
> - _mb=$(git merge-base $_g $_bad)
> - if test "$_mb" = "$_g" || is_among "$_mb" "$_good"; then
> - mark_merge_base_ok "$_bad" "$_g"
> - elif test "$_mb" = "$_bad"; then
> - handle_bad_merge_base "$_bad" "$_g"
> - elif is_among "$_mb" "$_skip"; then
> - handle_skipped_merge_base "$_bad" "$_g" "_mb"
> - else
> - mark_testing_merge_base "$_mb"
> - checkout "$_mb" "a merge base must be tested"
> - checkout_done=1
> - break
> - fi
> + for _mb in $(git merge-base --all $_g $_bad); do
> + if test "$_mb" = "$_g" || is_among "$_mb" "$_good"; then
> + continue
> + elif test "$_mb" = "$_bad"; then
> + handle_bad_merge_base "$_bad" "$_g"
> + elif is_among "$_mb" "$_skip"; then
> + handle_skipped_merge_base "$_bad" "$_g" "_mb"
> + else
> + mark_testing_merge_base "$_mb"
> + checkout "$_mb" "a merge base must be tested"
> + checkout_done=1
> + return
> + fi
> + done
> + mark_merge_base_ok "$_bad" "$_g"
> done
> }
I really wonder if we cannot do better than that, in terms of code
complexity.
For example, I wonder if we should special-case the start, and not just
check everytime if there are unchecked merge bases instead. If there are,
check the first.
But that can wait until you come back from your vacation...
Have fun,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
