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

[Christian Couder <chriscool@tuxfamily.org>]

Hi,

Le dimanche 13 juillet 2008, Johannes Schindelin a écrit :
> 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.

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

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

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