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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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