[PATCH] Avoid warning when bisecting a merge

[PATCH] Avoid warning when bisecting a merge

[Gustaf Hendeby]

Trying to compare an empty string as a number results in an error,
hence make sure checkout_done is set before using it.

Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>
---

This one should go on top of cc/bisect.

/Gustaf

 git-bisect.sh |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

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
 
-- 
1.6.0.1.320.ga0f13.dirty

Re: [PATCH] Avoid warning when bisecting a merge

[Christian Couder]

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>

Thanks,
Christian.

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.

> ---
>
> This one should go on top of cc/bisect.
>
> /Gustaf
>
>  git-bisect.sh |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> 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

Re: [PATCH] Avoid warning when bisecting a merge

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> 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@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?

>> 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=''

Re: [PATCH] Avoid warning when bisecting a merge

[Gustaf Hendeby]

On 09/05/2008 08:29 AM, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> 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@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?
> 
>> 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".

I can't argue with that.  Junio, your suggestion makes more sense.  I
should have paid more attention to what I was doing.  Sorry about the noise.

/Gustaf

> 
> 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=''
> 
> 

Re: [PATCH] Avoid warning when bisecting a merge

[Junio C Hamano]

Gustaf Hendeby <hendeby@isy.liu.se> writes:

> On 09/05/2008 08:29 AM, Junio C Hamano wrote:
> ...
>> Yup, that might make more sense. In the meantime, I suspect this makes
>> more sense than introducing a new state "0".
>
> I can't argue with that.  Junio, your suggestion makes more sense.  I
> should have paid more attention to what I was doing.  Sorry about the noise.

That's Ok.  Spotting mistakes is an important part of the review process;
there is no need to be afraid of or apologetic about it.