# [PATCH] Avoid warning when bisecting a merge 5 messages from 2008-09-04 to 2008-09-05. Participants: Gustaf Hendeby, Christian Couder, Junio C Hamano. Thread: https://gitlist.dev/t/15375 ## Gustaf Hendeby, 2008-09-04 21:02 Subject: [PATCH] Avoid warning when bisecting a merge Message-ID: <1220562150-19962-1-git-send-email-hendeby@isy.liu.se> URL: https://gitlist.dev/e/1220562150-19962-1-git-send-email-hendeby%40isy.liu.se ``` 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 --- 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 ``` ## Christian Couder, 2008-09-05 06:14 Subject: Re: [PATCH] Avoid warning when bisecting a merge Message-ID: <200809050814.36937.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/200809050814.36937.chriscool%40tuxfamily.org In-Reply-To: <1220562150-19962-1-git-send-email-hendeby@isy.liu.se> ``` 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 Acked-by: Christian Couder 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 ``` ## Junio C Hamano, 2008-09-05 06:29 Subject: Re: [PATCH] Avoid warning when bisecting a merge Message-ID: <7vhc8vxg04.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhc8vxg04.fsf%40gitster.siamese.dyndns.org In-Reply-To: <200809050814.36937.chriscool@tuxfamily.org> ``` Christian Couder 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 > > Acked-by: Christian Couder 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='' ``` ## Gustaf Hendeby, 2008-09-05 07:18 Subject: Re: [PATCH] Avoid warning when bisecting a merge Message-ID: <48C0DD32.5010008@isy.liu.se> URL: https://gitlist.dev/e/48C0DD32.5010008%40isy.liu.se In-Reply-To: <7vhc8vxg04.fsf@gitster.siamese.dyndns.org> ``` On 09/05/2008 08:29 AM, Junio C Hamano wrote: > Christian Couder 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 >> Acked-by: Christian Couder > > 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='' > > ``` ## Junio C Hamano, 2008-09-05 08:31 Subject: Re: [PATCH] Avoid warning when bisecting a merge Message-ID: <7v3akfvvsp.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3akfvvsp.fsf%40gitster.siamese.dyndns.org In-Reply-To: <48C0DD32.5010008@isy.liu.se> ``` Gustaf Hendeby 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. ```