Re: [PATCH] Avoid warning when bisecting a merge
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 5, 2008, 06:29 UTC
- Message-ID
- <7vhc8vxg04.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <200809050814.36937.chriscool@tuxfamily.org>
Christian Couder <chriscool@tuxfamily.org> writes:
Show 9 quoted lines
> 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@isy.liu.se> > > Acked-by: Christian Couder <chriscool@tuxfamily.org>
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?
Show 11 quoted lines
>> 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=''