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

[Christian Couder <chriscool@tuxfamily.org>]

Le vendredi 11 juillet 2008, Christian Couder a écrit :
> Le jeudi 10 juillet 2008, Junio C Hamano a écrit :
> > Christian Couder <chriscool@tuxfamily.org> writes:
> > > Yeah, in that case...
> > >
> > >> The whole idea of "bisect" relies on that idea, that any ancestor of
> > >> a good commit is good.  Otherwise you'd have to check the commits
> > >> one by one, not in a bisecting manner.
> >
> > Didn't we already discuss this at length?
>
> Yes, the thread is there:
>
> http://thread.gmane.org/gmane.comp.version-control.git/86951
>
> > > No, you just need to check that the merge bases between the bad rev
> > > on one side and each good rev on the other side are good too. And if
> > > that is the case, then you can be sure that bisection will point to a
> > > first bad commit.
> > >
> > > So the choice is between a simple and fast but not fully reliable
> > > bisect, or a more complex and slower but fully reliable bisect.
> >
> > I have not looked at your implementation, but I do think:
> >
> >  - The current one is not "fully reliable"; the user needs to know what
> >    he is doing.  You might call it "prone to user errors".
>
> I agree.
>
> >  - "Test this merge-base before going forward, please" will add
> > typically only one round of check (if you have more merge bases between
> > good and bad, you need to test all of them are good to be sure), so it
> > is not "slower nor more complex".
>
> By "slower" I meant that it would need more rounds of check on average.
> By "more complex" I meant that more code is needed.
>
> And I think you are right, all the merge bases need to be tested so I
> will send a patch on top of the patch discussed here.

Ok, here is an untested patch that should check all merge bases. I don't 
have time right now to add tests and a good commit message but I will do 
that when I come back from vacancy in about one week. So please consider it 
as just a RFC.

Thanks,
Christian.

-----8<------------

[PATCH] bisect: check all merge bases instead of only one


Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-bisect.sh |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 50f912f..ca16609 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -375,7 +375,6 @@ Warning: the merge base between $_bad and $_g must be 
skipped.
 So we cannot be sure the first bad commit is between $_mb and $_bad.
 We continue anyway.
 EOF
-	mark_merge_base_ok "$_bad" "$_g"
 }
 
 check_merge_bases() {
@@ -384,19 +383,21 @@ check_merge_bases() {
 	_skip="$3"
 	for _g in $_good; do
 		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
 }
 
-- 
1.5.6.2.221.gf54e0.dirty