threads / patch / 18280

patch, 7 partsbisect: use "rev-list --bisect-skip" and remove "filter_skipped" function

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

## tl;dr

3 messages between Mar 12, 2009 and Mar 13, 2009. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Christian Couder· Mar 12, 2009, 07:51 UTC · lore

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(-)
Show changes to git-bisect.sh +13 −76
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
Junio C Hamano· Mar 12, 2009, 08:26 UTC · re: Christian Couder · lore

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

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.

Christian Couder· Mar 13, 2009, 05:29 UTC · re: Junio C Hamano · lore

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

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.

Show 6 quoted lines
> 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.

Show 5 quoted lines
> 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.

← back to recent threads