{"thread":{"id":"15375","subject":"[PATCH] Avoid warning when bisecting a merge","startedAt":"2008-09-04T21:02:30Z","lastAt":"2008-09-05T08:31:02Z","messageCount":5,"participants":["Gustaf Hendeby","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89790","messageId":"1220562150-19962-1-git-send-email-hendeby@isy.liu.se","threadId":"15375","inReplyTo":null,"subject":"[PATCH] Avoid warning when bisecting a merge","fromName":"Gustaf Hendeby","fromEmail":"hendeby@isy.liu.se","sentAt":"2008-09-04T21:02:30Z","receivedAt":"2008-09-04T21:02:30Z","isPatch":true,"sender":{"key":"hendeby@isy.liu.se","avatar":"https://avatars.githubusercontent.com/u/730316?v=4"},"body":"Trying to compare an empty string as a number results in an error,\nhence make sure checkout_done is set before using it.\n\nSigned-off-by: Gustaf Hendeby <hendeby@isy.liu.se>\n---\n\nThis one should go on top of cc/bisect.\n\n/Gustaf\n\n git-bisect.sh |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 69a9a56..05d14b3 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -437,6 +437,7 @@ bisect_next() {\n \t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') &&\n \n \t# Maybe some merge bases must be tested first\n+\tcheckout_done=0\n \tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n \ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n \n-- \n1.6.0.1.320.ga0f13.dirty\n"},{"id":"89825","messageId":"200809050814.36937.chriscool@tuxfamily.org","threadId":"15375","inReplyTo":"1220562150-19962-1-git-send-email-hendeby@isy.liu.se","subject":"Re: [PATCH] Avoid warning when bisecting a merge","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-09-05T06:14:36Z","receivedAt":"2008-09-05T06:14:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 4 septembre 2008, Gustaf Hendeby a écrit :\n> Trying to compare an empty string as a number results in an error,\n> hence make sure checkout_done is set before using it.\n\nThis patch seems to work fine. \n\n> Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>\n\nAcked-by: Christian Couder <chriscool@tuxfamily.org>\n\nThanks,\nChristian.\n\nPS: After thinking about it, I wonder if we should remove $checkout_done \nentirely and use the return value from \"check_merge_bases\" \nand \"check_good_are_ancestors_of_bad\" to know if a checkout was done.\n\n> ---\n>\n> This one should go on top of cc/bisect.\n>\n> /Gustaf\n>\n>  git-bisect.sh |    1 +\n>  1 files changed, 1 insertions(+), 0 deletions(-)\n>\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 69a9a56..05d14b3 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -437,6 +437,7 @@ bisect_next() {\n>  \t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') &&\n>\n>  \t# Maybe some merge bases must be tested first\n> +\tcheckout_done=0\n>  \tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n>  \ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n"},{"id":"89827","messageId":"7vhc8vxg04.fsf@gitster.siamese.dyndns.org","threadId":"15375","inReplyTo":"200809050814.36937.chriscool@tuxfamily.org","subject":"Re: [PATCH] Avoid warning when bisecting a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-05T06:29:15Z","receivedAt":"2008-09-05T06:29:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Le jeudi 4 septembre 2008, Gustaf Hendeby a écrit :\n>> Trying to compare an empty string as a number results in an error,\n>> hence make sure checkout_done is set before using it.\n>\n> This patch seems to work fine. \n>\n>> Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>\n>\n> Acked-by: Christian Couder <chriscool@tuxfamily.org>\n\nHave you actually read the patch and thought about it before acking it?\n\nWhy does a variable that says \"have we done checkout?\" have three states?\nCertainly it is not like \"yes, no, dunno\", right?  checkout_done=0 which\nwas added by Gustaf, checkout_done=1 is the state the test checks with\n(presumably set by check_good_are_ancestors_of_bad), and checkout_done=''\nwhich the code does before returning?\n\n>> diff --git a/git-bisect.sh b/git-bisect.sh\n>> index 69a9a56..05d14b3 100755\n>> --- a/git-bisect.sh\n>> +++ b/git-bisect.sh\n>> @@ -437,6 +437,7 @@ bisect_next() {\n>>  \t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') &&\n>>\n>>  \t# Maybe some merge bases must be tested first\n>> +\tcheckout_done=0\n>>  \tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n>>  \ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n\n> PS: After thinking about it, I wonder if we should remove $checkout_done \n> entirely and use the return value from \"check_merge_bases\" \n> and \"check_good_are_ancestors_of_bad\" to know if a checkout was done.\n\nYup, that might make more sense. In the meantime, I suspect this makes\nmore sense than introducing a new state \"0\".\n\ndiff --git c/git-bisect.sh w/git-bisect.sh\nindex 69a9a56..73f01bb 100755\n--- c/git-bisect.sh\n+++ w/git-bisect.sh\n@@ -30,6 +30,7 @@ OPTIONS_SPEC=\n . git-sh-setup\n require_work_tree\n \n+checkout_done=\n _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n \n@@ -418,7 +419,7 @@ check_good_are_ancestors_of_bad() {\n \t_side=$(git rev-list $_good ^$_bad)\n \tif test -n \"$_side\"; then\n \t\tcheck_merge_bases \"$_bad\" \"$_good\" \"$_skip\" || return\n-\t\ttest \"$checkout_done\" -eq \"1\" && return\n+\t\ttest -n \"$checkout_done\" && return\n \tfi\n \n \t: > \"$GIT_DIR/BISECT_ANCESTORS_OK\"\n@@ -438,7 +439,7 @@ bisect_next() {\n \n \t# Maybe some merge bases must be tested first\n \tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n-\ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n+\ttest -n \"$checkout_done\" && checkout_done='' && return\n \n \t# Get bisection information\n \tBISECT_OPT=''\n"},{"id":"89830","messageId":"48C0DD32.5010008@isy.liu.se","threadId":"15375","inReplyTo":"7vhc8vxg04.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Avoid warning when bisecting a merge","fromName":"Gustaf Hendeby","fromEmail":"hendeby@isy.liu.se","sentAt":"2008-09-05T07:18:10Z","receivedAt":"2008-09-05T07:18:10Z","isPatch":true,"sender":{"key":"hendeby@isy.liu.se","avatar":"https://avatars.githubusercontent.com/u/730316?v=4"},"body":"On 09/05/2008 08:29 AM, Junio C Hamano wrote:\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> Le jeudi 4 septembre 2008, Gustaf Hendeby a écrit :\n>>> Trying to compare an empty string as a number results in an error,\n>>> hence make sure checkout_done is set before using it.\n>> This patch seems to work fine. \n>>\n>>> Signed-off-by: Gustaf Hendeby <hendeby@isy.liu.se>\n>> Acked-by: Christian Couder <chriscool@tuxfamily.org>\n> \n> Have you actually read the patch and thought about it before acking it?\n> \n> Why does a variable that says \"have we done checkout?\" have three states?\n> Certainly it is not like \"yes, no, dunno\", right?  checkout_done=0 which\n> was added by Gustaf, checkout_done=1 is the state the test checks with\n> (presumably set by check_good_are_ancestors_of_bad), and checkout_done=''\n> which the code does before returning?\n> \n>> PS: After thinking about it, I wonder if we should remove $checkout_done \n>> entirely and use the return value from \"check_merge_bases\" \n>> and \"check_good_are_ancestors_of_bad\" to know if a checkout was done.\n> \n> Yup, that might make more sense. In the meantime, I suspect this makes\n> more sense than introducing a new state \"0\".\n\nI can't argue with that.  Junio, your suggestion makes more sense.  I\nshould have paid more attention to what I was doing.  Sorry about the noise.\n\n/Gustaf\n\n> \n> diff --git c/git-bisect.sh w/git-bisect.sh\n> index 69a9a56..73f01bb 100755\n> --- c/git-bisect.sh\n> +++ w/git-bisect.sh\n> @@ -30,6 +30,7 @@ OPTIONS_SPEC=\n>  . git-sh-setup\n>  require_work_tree\n>  \n> +checkout_done=\n>  _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n>  _x40=\"$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40\"\n>  \n> @@ -418,7 +419,7 @@ check_good_are_ancestors_of_bad() {\n>  \t_side=$(git rev-list $_good ^$_bad)\n>  \tif test -n \"$_side\"; then\n>  \t\tcheck_merge_bases \"$_bad\" \"$_good\" \"$_skip\" || return\n> -\t\ttest \"$checkout_done\" -eq \"1\" && return\n> +\t\ttest -n \"$checkout_done\" && return\n>  \tfi\n>  \n>  \t: > \"$GIT_DIR/BISECT_ANCESTORS_OK\"\n> @@ -438,7 +439,7 @@ bisect_next() {\n>  \n>  \t# Maybe some merge bases must be tested first\n>  \tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n> -\ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n> +\ttest -n \"$checkout_done\" && checkout_done='' && return\n>  \n>  \t# Get bisection information\n>  \tBISECT_OPT=''\n> \n> \n"},{"id":"89833","messageId":"7v3akfvvsp.fsf@gitster.siamese.dyndns.org","threadId":"15375","inReplyTo":"48C0DD32.5010008@isy.liu.se","subject":"Re: [PATCH] Avoid warning when bisecting a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-09-05T08:31:02Z","receivedAt":"2008-09-05T08:31:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gustaf Hendeby <hendeby@isy.liu.se> writes:\n\n> On 09/05/2008 08:29 AM, Junio C Hamano wrote:\n> ...\n>> Yup, that might make more sense. In the meantime, I suspect this makes\n>> more sense than introducing a new state \"0\".\n>\n> I can't argue with that.  Junio, your suggestion makes more sense.  I\n> should have paid more attention to what I was doing.  Sorry about the noise.\n\nThat's Ok.  Spotting mistakes is an important part of the review process;\nthere is no need to be afraid of or apologetic about it.\n"}]}