Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 13, 2008, 13:14 UTC
Message-ID: <alpine.DEB.1.00.0807131504130.4816@eeepc-johanness>
In-Reply-To: <200807130837.40961.chriscool@tuxfamily.org>

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

Show 31 quoted lines

>  		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