Re: [PATCH v3 0/3] automatically skip away from broken commits

[Christian Couder <chriscool@tuxfamily.org>]

Le Saturday 06 June 2009, Junio C Hamano a écrit :
> Here is a fairly unscientific test to see how these three patches improve
> things.  It involves two test scripts:
>
>  - ./+runme is a "bisect run" script.  It says "tests OK" for all the
>    commits given, except that it says "untestable" for all commits that
>    are on the side branch of a merge that we first test.  I think this
>    reflects what happens in the real world setting very well, in that a
>    merge brings in a topic whose breakge is irrelevant to the bug we are
>    hunting.
>
>  - ./+runall is run in one git tree with two parameters (bad commit and
>    good commit); it assumes that
>
>    - ./+runme script above is in /var/tmp/+runme;
>
>    - a version of git without these three patches is installed with
>      prefix=/var/tmp/cc-original;
>
>    - a version of git with these three patches is installed with
>      prefix=/var/tmp/cc-updated;
>
>    and runs the bisection using both versions of git.
>
> The results are not that great; these three patches do not give clear win
> as we hoped:
>
>     $ linux-2.6/master; /var/tmp/+runall v2.6.30-rc8 v2.6.30-rc1
>     Total 2681 commits...
>     Marked 254 commits as untestable
>     Took 14 rounds with cc-updated
>     Marked 254 commits as untestable
>     Took 13 rounds with cc-original
>     $ linux-2.6/master; /var/tmp/+runall v2.6.30-rc8 v2.6.29
>     Total 12917 commits...
>     Marked 141 commits as untestable
>     Took 15 rounds with cc-updated
>     Marked 141 commits as untestable
>     Took 15 rounds with cc-original
>     $ linux-2.6/master; /var/tmp/+runall v2.6.30-rc1 v2.6.29
>     Total 10236 commits...
>     Marked 7749 commits as untestable
>     Took 15 rounds with cc-updated
>     Marked 7749 commits as untestable
>     Took 14 rounds with cc-original
>
> I think this shows that the "skip ratio" heuristics based on the distance
> in the "goodness scale" space does not help in avoiding commits that are
> close in topological space.  There may be cases where the version with
> patch gives fewer rounds especially when the history is very linear, but
> I was mostly interested in the number of commits at least in the
> thousands, which I think is what we should optimize things for, not a toy
> history of linear 100 commits.

I get the same results as yours, and I think that in these tests cases "git 
bisect" was not stuck with having only untestable commits with the 
highest "goodness" values. So in these cases the original behavior does 
quite well and that's why the updated behavior can't do better.

> Here are the test scripts you can use to reproduce the results.
>
> diff --git a/+runall b/+runall
> new file mode 100755
> index 0000000..291d000
> --- /dev/null
> +++ b/+runall
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +BAD=${1?BAD}
> +GOOD=${2?GOOD}
> +TOTAL=$(git rev-list $GOOD..$BAD | wc -l)
> +
> +echo Total $TOTAL commits...
> +
> +for git in cc-updated cc-original
> +do
> +	logfile=/var/tmp/log-$git-$BAD-$GOOD
> +	(
> +		PATH=/var/tmp/$git/bin:$PATH
> +		export PATH
> +		rm -f .git/UNTESTABLE
> +		git bisect reset
> +		git bisect start $BAD $GOOD
> +		git bisect run /var/tmp/+runme
> +		git bisect reset
> +	) >$logfile 2>&1
> +	grep "^Marked " $logfile
> +	echo Took $(grep -c "Bisecting:" $logfile) rounds with $git
> +done
> diff --git a/+runme b/+runme
> new file mode 100755
> index 0000000..7b77338
> --- /dev/null
> +++ b/+runme
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +# Pretend that the first merged branch were untestable
> +
> +THIS=$(git rev-parse HEAD)
> +
> +if ! test -f .git/UNTESTABLE
> +then
> +	if MERGED=$(git rev-parse HEAD^2 2>/dev/null)
> +	then
> +		git rev-list HEAD^ ^$MERGED >.git/UNTESTABLE
> +		echo Marked $(wc -l <.git/UNTESTABLE) commits as untestable
> +		exit 125
> +	else
> +		exit 0
> +	fi
> +fi
> +
> +if grep "$THIS" .git/UNTESTABLE >/dev/null
> +then
> +	exit 125
> +fi
> +exit 0

Thanks.

I renamed your +runme to +runme-good and created a +runme-bad script with 
the following changes:

$ diff -u +runme-good +runme-bad
--- +runme-good 2009-06-07 03:53:08.000000000 +0200
+++ +runme-bad  2009-06-07 08:11:33.000000000 +0200
@@ -12,7 +12,7 @@
                echo Marked $(wc -l <.git/UNTESTABLE) commits as untestable
                exit 125
        else
-               exit 0
+               exit 1
        fi
 fi

@@ -20,4 +20,4 @@
 then
        exit 125
 fi
-exit 0
+exit 1

So that the +runme-bad says "tests KO" (instead of "tests OK" for 
+runme-good) for all the commits given, except that it says "untestable" 
for all commits that are on the side branch of a merge that we first test.

Then I created the "+runme -> +runme-bad" symlink and tried your examples:

$ /var/tmp/+runall v2.6.30-rc8 v2.6.30-rc1
Total 2681 commits...
Marked 254 commits as untestable
Took 11 rounds with cc-updated
Marked 254 commits as untestable
Took 12 rounds with cc-original

$ /var/tmp/+runall v2.6.30-rc8 v2.6.29
Total 12917 commits...
Marked 141 commits as untestable
Took 15 rounds with cc-updated
Marked 141 commits as untestable
Took 15 rounds with cc-original

$ /var/tmp/+runall v2.6.30-rc1 v2.6.29
Total 10236 commits...
Marked 1777 commits as untestable
Took 9 rounds with cc-updated
Marked 1777 commits as untestable
Took 492 rounds with cc-original

The last one is very interesting. It seems that the original implementation 
got stuck while the updated one did a great job...

Best regards,
Christian.