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

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

[Christian Couder]

Before this patch, 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 remove a spurious space where the
"exit_if_skipped_commits" function is defined.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-bisect.sh               |    4 ++--
 t/t6030-bisect-porcelain.sh |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 85db4ba..105bc6a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -309,7 +309,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
 				;;
 
@@ -342,7 +342,7 @@ filter_skipped() {
 	done
 }
 
-exit_if_skipped_commits () {
+exit_if_skipped_commits() {
 	_tried=$1
 	if expr "$_tried" : ".*[|].*" > /dev/null ; then
 		echo "There are only 'skip'ped commit left to test."
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index dd7eac8..b5da16f 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 &&
+		test_must_fail grep $HASH1 my_bisect_log.txt &&
+		test_must_fail 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.20.g8c5b.dirty

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Before this patch, 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:

Everybody knows that the problem description that comes at the beginning
of the commit log message talks about the state of the code before the
patch is applied.

Try reading the first sentence without "Before this patch, ".  It still
makes perfect sense and more importantly, it is much more readable.

> 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.

I am a bit worried why this "would have failed" was not noticed.

> 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.

Thanks.  Fixes before the 1.6.2 release are very much welcomed.

> And while at it, we also remove a spurious space where the
> "exit_if_skipped_commits" function is defined.

Looking at:

$ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- '*.sh' |
  sed -e 's/^[^ (]*/X/' | sort | uniq -c

and then doing the same for only git-bisect.sh, i.e.

$ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- 'git-bisect.sh' |
  sed -e 's/^[^ (]*/X/' | sort | uniq -c

you will notice that git-bisect is an odd-man out that uses 3 "func () {"
and 13 "func() {".  Majority of functions have one SP after the function
name.

If we were to standardize on one style, we should consistently have SP
there.

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index dd7eac8..b5da16f 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

An unpatched "git-bisect" seems to say "32a594a3 was both good and bad" in
its my_bisect_log.txt .  This makes me suspect that we are forgetting to
check return status after we eval the output from filter_skipped function.
Shouldn't the function should string its commands together with "&&" to
protect it from a breakage like this?

Also, VARS, FOUND and TRIED are not initialized anywhere.  We should
protect ourselves from environment variables the user may have with these
names before starting bisect to break the logic of this code.

Back to the test script.

> +		grep "first bad commit could be any of" my_bisect_log.txt &&
> +		test_must_fail grep $HASH1 my_bisect_log.txt &&
> +		test_must_fail grep $HASH2 my_bisect_log.txt &&

These two are easier to read with ! not test_must_fail; we do not expect
grep to be buggy and dump core ;-)

So perhaps the big loop should be better written like this...

filter_skipped() {
	...
	# Let's parse the output of:
	# "git rev-list --bisect-vars --bisect-all ..."
	eval_rev_list "$_eval" | {
		VARS= FOUND= TRIED=
		while read hash 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,*,*)
				;;
			*) 
				# An unexpected input
				echo "die 'filter_skipped error'"
				echo >&2 \
					"VARS: '$VARS' " \
					"FOUND: '$FOUND' " \
					"TRIED: '$TRIED' " \
					"hash: '$hash' " \
					"line: '$line'"
				;;
			esac
		done
		echo ':'
	}
}

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

[Christian Couder]

Le jeudi 26 février 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Before this patch, 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:
>
> Everybody knows that the problem description that comes at the beginning
> of the commit log message talks about the state of the code before the
> patch is applied.
>
> Try reading the first sentence without "Before this patch, ".  It still
> makes perfect sense and more importantly, it is much more readable.

Ok, I will drop that.

> > 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.
>
> I am a bit worried why this "would have failed" was not noticed.

Perhaps because people do not often use "skip" and "bad" on the same commit.
There is an eval error printed on STDERR when this happens.

> > 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.
>
> Thanks.  Fixes before the 1.6.2 release are very much welcomed.
>
> > And while at it, we also remove a spurious space where the
> > "exit_if_skipped_commits" function is defined.
>
> Looking at:
>
> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- '*.sh' |
>   sed -e 's/^[^ (]*/X/' | sort | uniq -c
>
> and then doing the same for only git-bisect.sh, i.e.
>
> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- 'git-bisect.sh' |
>   sed -e 's/^[^ (]*/X/' | sort | uniq -c
>
> you will notice that git-bisect is an odd-man out that uses 3 "func () {"
> and 13 "func() {".  Majority of functions have one SP after the function
> name.
>
> If we were to standardize on one style, we should consistently have SP
> there.

Ok, I dropped the related change in the series I will send just after this 
email.

> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> > index dd7eac8..b5da16f 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
>
> An unpatched "git-bisect" seems to say "32a594a3 was both good and bad"
> in its my_bisect_log.txt .

Yes, but there is also an error printed on STDERR.

> This makes me suspect that we are forgetting 
> to check return status after we eval the output from filter_skipped
> function. Shouldn't the function should string its commands together with
> "&&" to protect it from a breakage like this?

Right, that would be an improvement. I put it in another patch though, 
because it is not really needed to fix a breakage.

> Also, VARS, FOUND and TRIED are not initialized anywhere.  We should
> protect ourselves from environment variables the user may have with these
> names before starting bisect to break the logic of this code.

Yeah I noticed that, I put that change in the first patch.

> Back to the test script.
>
> > +		grep "first bad commit could be any of" my_bisect_log.txt &&
> > +		test_must_fail grep $HASH1 my_bisect_log.txt &&
> > +		test_must_fail grep $HASH2 my_bisect_log.txt &&
>
> These two are easier to read with ! not test_must_fail; we do not expect
> grep to be buggy and dump core ;-)

Ok, there is now "!" in the first patch.

> So perhaps the big loop should be better written like this...

Yeah, it will look more or less like that.

Thanks,
Christian.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> This makes me suspect that we are forgetting 
>> to check return status after we eval the output from filter_skipped
>> function. Shouldn't the function should string its commands together with
>> "&&" to protect it from a breakage like this?
>
> Right, that would be an improvement. I put it in another patch though, 
> because it is not really needed to fix a breakage.

Sorry, but I have to disagree.

Look at the caller of filter_skipped in bisect_next():

	eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
	eval=$(filter_skipped "$eval" "$skip") &&
	eval "$eval" || exit

	if [ -z "$bisect_rev" ]; then
		echo "$bad was both good and bad"
		exit 1
	fi

The eval "$eval" in the middle, if failed properly upon seeing the quote
bug, would have exited there, because "|| exit" there is all about
catching a broken eval string.  It was not effective.

You were lucky in this case that bisect_rev happens to be empty because
the bug happened to be at the place where bisect_rev was assigned to.  But
with a random other breakage in the filter_skipped, you would not have
been so lucky.

I think it is an integral part of the bugfix to string the commands
filter_skipped outputs with &&, so that an error while executing an
earlier command in the output sequence is not masked by execution of other
commands in the output.

Here is what I am thinking about queueing for 1.6.2; it may be necessary
to apply it to 1.6.1.X (or 1.6.0.X) and merge the fix upwards.

By deepinging case/esac one level without indenting the case arms deeper,
it happens to re-indent the case/esac construct correctly.  case arms
should align with case/esac, and comments on each case arm should come
inside it.

-- >8 --

From: Christian Couder <chriscool@tuxfamily.org>
Date: Fri, 27 Feb 2009 07:31:22 +0100
Subject: [PATCH] bisect: fix quoting TRIED revs when "bad" commit is also "skip"ped

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.  The caller was not stopping
properly because the scriptlet this function returned to be evaled
was not strung together with && and because of this, an error in
an earlier part of the output was simply ignored.

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 <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-bisect.sh               |   70 ++++++++++++++++++++++++------------------
 t/t6030-bisect-porcelain.sh |   25 +++++++++++++++
 2 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 85db4ba..f863c88 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -290,56 +290,66 @@ filter_skipped() {
 
 	# Let's parse the output of:
 	# "git rev-list --bisect-vars --bisect-all ..."
-	eval "$_eval" | while read hash line
-	do
-		case "$VARS,$FOUND,$TRIED,$hash" in
-			# We display some vars.
-			1,*,*,*) echo "$hash $line" ;;
+	eval "$_eval" | {
+		VARS= FOUND= TRIED=
+		while read hash line
+		do
+			case "$VARS,$FOUND,$TRIED,$hash" in
+			1,*,*,*)
+				# "bisect_foo=bar" read from rev-list output.
+				echo "$hash &&" ;;
 
-			# Split line.
-			,*,*,---*) ;;
+			,*,*,---*)
+				# Separator
+				;;
 
-			# We had nothing to search.
 			,,,bisect_rev*)
-				echo "bisect_rev="
+				# We had nothing to search.
+				echo "bisect_rev= &&"
 				VARS=1
 				;;
 
-			# We did not find a good bisect rev.
-			# This should happen only if the "bad"
-			# commit is also a "skip" commit.
 			,,*,bisect_rev*)
-				echo "bisect_rev=$TRIED"
+				# 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.
 			,,*,*)
+				# We are searching.
 				TRIED="${TRIED:+$TRIED|}$hash"
 				case "$_skip" in
 				*$hash*) ;;
 				*)
-					echo "bisect_rev=$hash"
-					echo "bisect_tried=\"$TRIED\""
+					echo "bisect_rev=$hash &&"
+					echo "bisect_tried='$TRIED' &&"
 					FOUND=1
 					;;
 				esac
 				;;
 
-			# We have already found a rev to be tested.
-			,1,*,bisect_rev*) VARS=1 ;;
-			,1,*,*) ;;
-
-			# ???
-			*) die "filter_skipped error " \
-			    "VARS: '$VARS' " \
-			    "FOUND: '$FOUND' " \
-			    "TRIED: '$TRIED' " \
-			    "hash: '$hash' " \
-			    "line: '$line'"
-			;;
-		esac
-	done
+			,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
+		done
+		echo ':'
+	}
 }
 
 exit_if_skipped_commits () {
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.rc2.91.gf9a36

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

[Christian Couder]

Le vendredi 27 février 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> >> This makes me suspect that we are forgetting
> >> to check return status after we eval the output from filter_skipped
> >> function. Shouldn't the function should string its commands together
> >> with "&&" to protect it from a breakage like this?
> >
> > Right, that would be an improvement. I put it in another patch though,
> > because it is not really needed to fix a breakage.

Here I wanted to say that I think it fixes a separate breakage or another 
kind of breakage rather than not a breakage. Sorry.

> Sorry, but I have to disagree.
>
> Look at the caller of filter_skipped in bisect_next():
>
> 	eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
> 	eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
> 	eval=$(filter_skipped "$eval" "$skip") &&
> 	eval "$eval" || exit
>
> 	if [ -z "$bisect_rev" ]; then
> 		echo "$bad was both good and bad"
> 		exit 1
> 	fi
>
> The eval "$eval" in the middle, if failed properly upon seeing the quote
> bug, would have exited there, because "|| exit" there is all about
> catching a broken eval string.  It was not effective.

Yes, but before I introduced filter_skipped there was:

	eval="git rev-list --bisect-vars $good $bad --" &&
        eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
	eval=$(eval "$eval") &&
        eval "$eval" || exit

so the output of "git rev-list --bisect-vars ..." was evaled, and this 
output is something like that:

$ git rev-list --bisect-vars HEAD ^HEAD~3
bisect_rev=c24505030fad7cc2872e0a0fd0f44e05571a0ad8
bisect_nr=1
bisect_good=0
bisect_bad=1
bisect_all=3

where there is no "&&" at the end of the commands to string them together.
So this breakage already existed before I introduced "filter_skipped".

> You were lucky in this case that bisect_rev happens to be empty because
> the bug happened to be at the place where bisect_rev was assigned to. 
> But with a random other breakage in the filter_skipped, you would not
> have been so lucky.

Yeah, I should have improved on the existing design instead of blindly 
following it. I hope I won't get sued for that ;-)

> I think it is an integral part of the bugfix to string the commands
> filter_skipped outputs with &&, so that an error while executing an
> earlier command in the output sequence is not masked by execution of
> other commands in the output.

So you should perhaps squash the following hunk to the patch:

diff --git a/git-bisect.sh b/git-bisect.sh
index 08e31d6..980d73c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -284,7 +284,13 @@ filter_skipped() {
        _skip="$2"

        if [ -z "$_skip" ]; then
-               eval "$_eval"
+               eval "$_eval" | {
+                       while read line
+                       do
+                               echo "$line &&"
+                       done
+                       echo ':'
+               }
                return
        fi

as this will string the commands together when there are no skipped commits 
too.

> Here is what I am thinking about queueing for 1.6.2; it may be necessary
> to apply it to 1.6.1.X (or 1.6.0.X) and merge the fix upwards.

It looks good to me. But frankly I feel always strange when a patch like 
this one, where you did most of the code change, get attributed to me. I 
would have prefered that you added a patch attributed to you on top of mine 
if possible.

Best regards,
Christian.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> So you should perhaps squash the following hunk to the patch:
> ...
> as this will string the commands together when there are no skipped commits 
> too.

Ok, good eyes.

Even though this part is directly from rev-list --bisect-vars and is much
less likely to be malformed than the list of commands filter_skipped
function produces, I agree it is a good discipline to protect it from the
breakage the same way.

Thanks.