Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
> Le jeudi 4 septembre 2008, Gustaf Hendeby a écrit :
>> Trying to compare an empty string as a number results in an error,
>> hence make sure checkout_done is set before using it.
>
> This patch seems to work fine.
>
>> Signed-off-by: Gustaf Hendeby <hendeby@xxxxxxxxxx>
>
> Acked-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
Have you actually read the patch and thought about it before acking it?
Why does a variable that says "have we done checkout?" have three states?
Certainly it is not like "yes, no, dunno", right? checkout_done=0 which
was added by Gustaf, checkout_done=1 is the state the test checks with
(presumably set by check_good_are_ancestors_of_bad), and checkout_done=''
which the code does before returning?
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 69a9a56..05d14b3 100755
>> --- a/git-bisect.sh
>> +++ b/git-bisect.sh
>> @@ -437,6 +437,7 @@ bisect_next() {
>> "refs/bisect/skip-*" | tr '\012' ' ') &&
>>
>> # Maybe some merge bases must be tested first
>> + checkout_done=0
>> check_good_are_ancestors_of_bad "$bad" "$good" "$skip" || exit
>> test "$checkout_done" -eq "1" && checkout_done='' && return
> PS: After thinking about it, I wonder if we should remove $checkout_done
> entirely and use the return value from "check_merge_bases"
> and "check_good_are_ancestors_of_bad" to know if a checkout was done.
Yup, that might make more sense. In the meantime, I suspect this makes
more sense than introducing a new state "0".
diff --git c/git-bisect.sh w/git-bisect.sh
index 69a9a56..73f01bb 100755
--- c/git-bisect.sh
+++ w/git-bisect.sh
@@ -30,6 +30,7 @@ OPTIONS_SPEC=
. git-sh-setup
require_work_tree
+checkout_done=
_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
@@ -418,7 +419,7 @@ check_good_are_ancestors_of_bad() {
_side=$(git rev-list $_good ^$_bad)
if test -n "$_side"; then
check_merge_bases "$_bad" "$_good" "$_skip" || return
- test "$checkout_done" -eq "1" && return
+ test -n "$checkout_done" && return
fi
: > "$GIT_DIR/BISECT_ANCESTORS_OK"
@@ -438,7 +439,7 @@ bisect_next() {
# Maybe some merge bases must be tested first
check_good_are_ancestors_of_bad "$bad" "$good" "$skip" || exit
- test "$checkout_done" -eq "1" && checkout_done='' && return
+ test -n "$checkout_done" && checkout_done='' && return
# Get bisection information
BISECT_OPT=''
--
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
