Le vendredi 27 février 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
