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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Jul 22, 2008, 06:15 UTC
Message-ID: <200807220815.32989.chriscool@tuxfamily.org>
In-Reply-To: <alpine.DEB.1.00.0807131504130.4816@eeepc-johanness>

Hi,

Le dimanche 13 juillet 2008, Johannes Schindelin a écrit :

Show 8 quoted lines

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

I didn't think the first one was "faulty". It just didn't fix everything.

Show 6 quoted lines

> > @@ -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,

There is a reason though. It's because in "bisect_next", variables with those names ("good", "bad" and "skip") are already used, so reusing the same name is not easily possible.

Show 8 quoted lines

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

There are other places in git-bisect.sh where "for" and "do" are in the same line. Perhaps one day I will submit a patch to fix these.

Show 38 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.

In fact, there was such a thing in my patch, search for "$GIT_DIR/BISECT_ANCESTORS_OK". But it was a little bit broken if people didn't test the commit that "git bisect" suggested.

In the next version I will post just after this mail, this is in the 2/2 patch (and hopefully fixed).

> But that can wait until you come back from your vacation...
>
> Have fun,
> Dscho

Thanks, Christian.