[PATCH 7/7] bisect: use "rev-list --bisect-skip" and remove "filter_skipped" function

[PATCH 7/7] bisect: use "rev-list --bisect-skip" and remove "filter_skipped" function

[Christian Couder]

Use the new "--bisect-skip" option of "git rev-list" that should be
faster and safer instead of the old "filter_skipped" shell function.

As the output is a little bit different we have to change the code
that interpret the results. But these changes improve code clarity.

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

diff --git a/git-bisect.sh b/git-bisect.sh
index e313bde..31e7cff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -279,76 +279,13 @@ bisect_auto_next() {
 	bisect_next_check && bisect_next || :
 }
 
-filter_skipped() {
+eval_and_string_together() {
 	_eval="$1"
-	_skip="$2"
-
-	if [ -z "$_skip" ]; then
-		eval "$_eval" | {
-			while read line
-			do
-				echo "$line &&"
-			done
-			echo ':'
-		}
-		return
-	fi
 
-	# Let's parse the output of:
-	# "git rev-list --bisect-vars --bisect-all ..."
 	eval "$_eval" | {
-		VARS= FOUND= TRIED=
-		while read hash line
+		while read line
 		do
-			case "$VARS,$FOUND,$TRIED,$hash" in
-			1,*,*,*)
-				# "bisect_foo=bar" read from rev-list output.
-				echo "$hash &&"
-				;;
-			,*,*,---*)
-				# Separator
-				;;
-			,,,bisect_rev*)
-				# We had nothing to search.
-				echo "bisect_rev= &&"
-				VARS=1
-				;;
-			,,*,bisect_rev*)
-				# We did not find a good bisect rev.
-				# This should happen only if the "bad"
-				# commit is also a "skip" commit.
-				echo "bisect_rev='$TRIED' &&"
-				VARS=1
-				;;
-			,,*,*)
-				# We are searching.
-				TRIED="${TRIED:+$TRIED|}$hash"
-				case "$_skip" in
-				*$hash*) ;;
-				*)
-					echo "bisect_rev=$hash &&"
-					echo "bisect_tried='$TRIED' &&"
-					FOUND=1
-					;;
-				esac
-				;;
-			,1,*,bisect_rev*)
-				# We have already found a rev to be tested.
-				VARS=1
-				;;
-			,1,*,*)
-				;;
-			*)
-				# Unexpected input
-				echo "die 'filter_skipped error'"
-				die "filter_skipped error " \
-				    "VARS: '$VARS' " \
-				    "FOUND: '$FOUND' " \
-				    "TRIED: '$TRIED' " \
-				    "hash: '$hash' " \
-				    "line: '$line'"
-				;;
-			esac
+			echo "$line &&"
 		done
 		echo ':'
 	}
@@ -356,10 +293,12 @@ filter_skipped() {
 
 exit_if_skipped_commits () {
 	_tried=$1
-	if expr "$_tried" : ".*[|].*" > /dev/null ; then
+	_bad=$2
+	if test -n "$_tried" ; then
 		echo "There are only 'skip'ped commit left to test."
 		echo "The first bad commit could be any of:"
 		echo "$_tried" | tr '[|]' '[\012]'
+		test -n "$_bad" && echo "$_bad"
 		echo "We cannot bisect more!"
 		exit 2
 	fi
@@ -490,28 +429,26 @@ bisect_next() {
 	test "$?" -eq "1" && return
 
 	# Get bisection information
-	BISECT_OPT=''
-	test -n "$skip" && BISECT_OPT='--bisect-all'
-	eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
+	skip=$(echo $skip | tr ' ' ',')
+	eval="git rev-list --bisect-skip=$skip $good $bad --" &&
 	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
-	eval=$(filter_skipped "$eval" "$skip") &&
+	eval=$(eval_and_string_together "$eval") &&
 	eval "$eval" || exit
 
 	if [ -z "$bisect_rev" ]; then
+		# We should exit here only if the "bad"
+		# commit is also a "skip" commit (see above).
+		exit_if_skipped_commits "$bisect_tried"
 		echo "$bad was both good and bad"
 		exit 1
 	fi
 	if [ "$bisect_rev" = "$bad" ]; then
-		exit_if_skipped_commits "$bisect_tried"
+		exit_if_skipped_commits "$bisect_tried" "$bad"
 		echo "$bisect_rev is first bad commit"
 		git diff-tree --pretty $bisect_rev
 		exit 0
 	fi
 
-	# We should exit here only if the "bad"
-	# commit is also a "skip" commit (see above).
-	exit_if_skipped_commits "$bisect_rev"
-
 	bisect_checkout "$bisect_rev" "$bisect_nr revisions left to test after this (roughly $bisect_steps steps)"
 }
 
-- 
1.6.2.83.g012a16.dirty

Re: [PATCH 7/7] bisect: use "rev-list --bisect-skip" and remove "filter_skipped" function

[Junio C Hamano]

These (except for the first one which I do not think belongs to the
series) look more or less straightforward changes.

One worrysome thing that the series introduces is that you used to hold
all the skipped ones in a shell variable $skip and fed it internally to
the filter_skipped shell function, but now you give them from the command
line.  When you have tons of skipped commits, this can easily bust the
command line length limit on some system, while the shell may still be Ok
with such a large string variable.

Because the operations in rev-list related to bisect are very closely tied
to the implementation of the bisect Porcelain anyway, I am wondering if it
makes more sense to just feed the name of the toplevel refname hierarchy,
i.e. "refs/bisect", as a rev-list parameter and let rev-list enumerate
what is skipped, which commits are good and which ones are bad.

Re: [PATCH 7/7] bisect: use "rev-list --bisect-skip" and remove "filter_skipped" function

[Christian Couder]

Le jeudi 12 mars 2009, Junio C Hamano a écrit :
> These (except for the first one which I do not think belongs to the
> series) look more or less straightforward changes.

I added the first one to the series because I use strbuf_split with the new 
behavior in the series.

> One worrysome thing that the series introduces is that you used to hold
> all the skipped ones in a shell variable $skip and fed it internally to
> the filter_skipped shell function, but now you give them from the command
> line.  When you have tons of skipped commits, this can easily bust the
> command line length limit on some system, while the shell may still be Ok
> with such a large string variable.

Yeah. I will send a patch (8/7) that makes "git rev-list" read the skipped 
commits from stdin when "--bisect-skip" is passed, and that changes 
git-bisect.sh accordingly. If this behavior is ok, then I can reorder the 
series if you want.

> Because the operations in rev-list related to bisect are very closely
> tied to the implementation of the bisect Porcelain anyway, I am wondering
> if it makes more sense to just feed the name of the toplevel refname
> hierarchy, i.e. "refs/bisect", as a rev-list parameter and let rev-list
> enumerate what is skipped, which commits are good and which ones are bad.

Yeah that could be an improvement, but then the enumeration will happen once 
in git-bisect.sh and once in "git rev-list", instead of just once in 
git-bisect.sh. And we also have to deal with the other command line 
arguments passed to "git rev-list" on top of the enumerated commits from 
the refname hierachy, so I don't think that would be very consistent.

But perhaps we could create a new "git rev-bisect" builtin plumbing command 
that could do that. And later we could improve this new command so that it 
checks merge bases (like what is done in bisect_next in git-bisect.sh) and 
so that it also checks out the source code of the new revision to be tested 
(like what is done in bisect_checkout in git-bisect.sh).

Thanks,
Christian.