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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Feb 27, 2009, 21:05 UTC
Message-ID: <200902272205.10501.chriscool@tuxfamily.org>
In-Reply-To: <7v3ae0mfob.fsf@gitster.siamese.dyndns.org>

Le vendredi 27 février 2009, Junio C Hamano a écrit :

Show 8 quoted lines

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

Show 17 quoted lines

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