[PATCH 1/2] bisect: fix quoting TRIED revs when "bad" commit is also "skip"ped

Subsystems: the rest

DORMANTno replies

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

[PATCH 1/2] bisect: fix quoting TRIED revs when "bad" commit is also "skip"ped

From: Christian Couder <hidden>
Date: 2016-06-15 22:46:17

When the "bad" commit was also "skip"ped and when more than one
commit was skipped, the "filter_skipped" function would have
printed something like:

bisect_rev=<hash1>|<hash2>

(where <hash1> and <hash2> are hexadecimal sha1 hashes)

and this would have been evaled later as piping "bisect_rev=<hash1>"
into "<hash2>", which would have failed.

So this patch makes the "filter_skipped" function properly quote
what it outputs, so that it will print something like:

bisect_rev='<hash1>|<hash2>'

which will be properly evaled later.

A test case is added to the test suite.

And while at it, we also initialize the VARS, FOUND and TRIED
variables, so that we protect ourselves from environment variables
the user may have with these names.

Signed-off-by: Christian Couder <redacted>
---
 git-bisect.sh               |    6 ++++--
 t/t6030-bisect-porcelain.sh |   25 +++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/git-bisect.sh b/git-bisect.sh
index 85db4ba..a9324b2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -288,6 +288,8 @@ filter_skipped() {
 		return
 	fi
 
+	VARS= FOUND= TRIED=
+
 	# Let's parse the output of:
 	# "git rev-list --bisect-vars --bisect-all ..."
 	eval "$_eval" | while read hash line
@@ -309,7 +311,7 @@ filter_skipped() {
 			# This should happen only if the "bad"
 			# commit is also a "skip" commit.
 			,,*,bisect_rev*)
-				echo "bisect_rev=$TRIED"
+				echo "bisect_rev='$TRIED'"
 				VARS=1
 				;;
 
@@ -320,7 +322,7 @@ filter_skipped() {
 				*$hash*) ;;
 				*)
 					echo "bisect_rev=$hash"
-					echo "bisect_tried=\"$TRIED\""
+					echo "bisect_tried='$TRIED'"
 					FOUND=1
 					;;
 				esac
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index dd7eac8..052a6c9 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell between 2 commits' '
 	fi
 '
 
+# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3
+# and $HASH2 is good,
+# so we should not be able to tell the first bad commit
+# among $HASH3 and $HASH4
+test_expect_success 'bisect skip: with commit both bad and skipped' '
+	git bisect start &&
+	git bisect skip &&
+	git bisect bad &&
+	git bisect good $HASH1 &&
+	git bisect skip &&
+	if git bisect good > my_bisect_log.txt
+	then
+		echo Oops, should have failed.
+		false
+	else
+		test $? -eq 2 &&
+		grep "first bad commit could be any of" my_bisect_log.txt &&
+		! grep $HASH1 my_bisect_log.txt &&
+		! grep $HASH2 my_bisect_log.txt &&
+		grep $HASH3 my_bisect_log.txt &&
+		grep $HASH4 my_bisect_log.txt &&
+		git bisect reset
+	fi
+'
+
 # We want to automatically find the commit that
 # introduced "Another" into hello.
 test_expect_success \
-- 
1.6.2.rc1.257.g6c75

Re: [PATCH 1/2] bisect: fix quoting TRIED revs when "bad" commit is also "skip"ped

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:17

Christian Couder [off-list ref] writes:
quoted hunk
diff --git a/git-bisect.sh b/git-bisect.sh
index 85db4ba..a9324b2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -288,6 +288,8 @@ filter_skipped() {
 		return
 	fi
 
+	VARS= FOUND= TRIED=
+
Did you have any particular reason to move these out of the subshell on
the downstream side of the pipe as I wrote in my response?  That is where
these variables matter.
quoted hunk
 	# Let's parse the output of:
 	# "git rev-list --bisect-vars --bisect-all ..."
 	eval "$_eval" | while read hash line
@@ -309,7 +311,7 @@ filter_skipped() {
Also you seem to have lost my fix to ignore $line for "bisect_foo=value"
lines where $line is always empty.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help