{"thread":{"id":"18029","subject":"[PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","startedAt":"2009-02-26T07:29:18Z","lastAt":"2009-02-27T22:34:33Z","messageCount":6,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"106300","messageId":"20090226082918.6adbc565.chriscool@tuxfamily.org","threadId":"18029","inReplyTo":null,"subject":"[PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-02-26T07:29:18Z","receivedAt":"2009-02-26T07:29:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Before this patch, when the \"bad\" commit was also \"skip\"ped and\nwhen more than one commit was skipped, the \"filter_skipped\" function\nwould have printed something like:\n\nbisect_rev=<hash1>|<hash2>\n\n(where <hash1> and <hash2> are hexadecimal sha1 hashes)\n\nand this would have been evaled later as piping \"bisect_rev=<hash1>\"\ninto \"<hash2>\", which would have failed.\n\nSo this patch makes the \"filter_skipped\" function properly quote\nwhat it outputs, so that it will print something like:\n\nbisect_rev=\"<hash1>|<hash2>\"\n\nwhich will be properly evaled later.\n\nA test case is added to the test suite.\n\nAnd while at it, we also remove a spurious space where the\n\"exit_if_skipped_commits\" function is defined.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-bisect.sh               |    4 ++--\n t/t6030-bisect-porcelain.sh |   25 +++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 85db4ba..105bc6a 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -309,7 +309,7 @@ filter_skipped() {\n \t\t\t# This should happen only if the \"bad\"\n \t\t\t# commit is also a \"skip\" commit.\n \t\t\t,,*,bisect_rev*)\n-\t\t\t\techo \"bisect_rev=$TRIED\"\n+\t\t\t\techo \"bisect_rev=\\\"$TRIED\\\"\"\n \t\t\t\tVARS=1\n \t\t\t\t;;\n \n@@ -342,7 +342,7 @@ filter_skipped() {\n \tdone\n }\n \n-exit_if_skipped_commits () {\n+exit_if_skipped_commits() {\n \t_tried=$1\n \tif expr \"$_tried\" : \".*[|].*\" > /dev/null ; then\n \t\techo \"There are only 'skip'ped commit left to test.\"\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex dd7eac8..b5da16f 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell between 2 commits' '\n \tfi\n '\n \n+# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3\n+# and $HASH2 is good,\n+# so we should not be able to tell the first bad commit\n+# among $HASH3 and $HASH4\n+test_expect_success 'bisect skip: with commit both bad and skipped' '\n+\tgit bisect start &&\n+\tgit bisect skip &&\n+\tgit bisect bad &&\n+\tgit bisect good $HASH1 &&\n+\tgit bisect skip &&\n+\tif git bisect good > my_bisect_log.txt\n+\tthen\n+\t\techo Oops, should have failed.\n+\t\tfalse\n+\telse\n+\t\ttest $? -eq 2 &&\n+\t\tgrep \"first bad commit could be any of\" my_bisect_log.txt &&\n+\t\ttest_must_fail grep $HASH1 my_bisect_log.txt &&\n+\t\ttest_must_fail grep $HASH2 my_bisect_log.txt &&\n+\t\tgrep $HASH3 my_bisect_log.txt &&\n+\t\tgrep $HASH4 my_bisect_log.txt &&\n+\t\tgit bisect reset\n+\tfi\n+'\n+\n # We want to automatically find the commit that\n # introduced \"Another\" into hello.\n test_expect_success \\\n-- \n1.6.2.rc1.20.g8c5b.dirty\n"},{"id":"106302","messageId":"7v63ixwpnm.fsf@gitster.siamese.dyndns.org","threadId":"18029","inReplyTo":"20090226082918.6adbc565.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-26T08:47:25Z","receivedAt":"2009-02-26T08:47:25Z","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> Before this patch, when the \"bad\" commit was also \"skip\"ped and\n> when more than one commit was skipped, the \"filter_skipped\" function\n> would have printed something like:\n\nEverybody knows that the problem description that comes at the beginning\nof the commit log message talks about the state of the code before the\npatch is applied.\n\nTry reading the first sentence without \"Before this patch, \".  It still\nmakes perfect sense and more importantly, it is much more readable.\n\n> bisect_rev=<hash1>|<hash2>\n>\n> (where <hash1> and <hash2> are hexadecimal sha1 hashes)\n>\n> and this would have been evaled later as piping \"bisect_rev=<hash1>\"\n> into \"<hash2>\", which would have failed.\n\nI am a bit worried why this \"would have failed\" was not noticed.\n\n> So this patch makes the \"filter_skipped\" function properly quote\n> what it outputs, so that it will print something like:\n>\n> bisect_rev=\"<hash1>|<hash2>\"\n>\n> which will be properly evaled later.\n>\n> A test case is added to the test suite.\n\nThanks.  Fixes before the 1.6.2 release are very much welcomed.\n\n> And while at it, we also remove a spurious space where the\n> \"exit_if_skipped_commits\" function is defined.\n\nLooking at:\n\n$ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- '*.sh' |\n  sed -e 's/^[^ (]*/X/' | sort | uniq -c\n\nand then doing the same for only git-bisect.sh, i.e.\n\n$ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- 'git-bisect.sh' |\n  sed -e 's/^[^ (]*/X/' | sort | uniq -c\n\nyou will notice that git-bisect is an odd-man out that uses 3 \"func () {\"\nand 13 \"func() {\".  Majority of functions have one SP after the function\nname.\n\nIf we were to standardize on one style, we should consistently have SP\nthere.\n\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index dd7eac8..b5da16f 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell between 2 commits' '\n>  \tfi\n>  '\n>  \n> +# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3\n> +# and $HASH2 is good,\n> +# so we should not be able to tell the first bad commit\n> +# among $HASH3 and $HASH4\n> +test_expect_success 'bisect skip: with commit both bad and skipped' '\n> +\tgit bisect start &&\n> +\tgit bisect skip &&\n> +\tgit bisect bad &&\n> +\tgit bisect good $HASH1 &&\n> +\tgit bisect skip &&\n> +\tif git bisect good > my_bisect_log.txt\n\nAn unpatched \"git-bisect\" seems to say \"32a594a3 was both good and bad\" in\nits my_bisect_log.txt .  This makes me suspect that we are forgetting to\ncheck return status after we eval the output from filter_skipped function.\nShouldn't the function should string its commands together with \"&&\" to\nprotect it from a breakage like this?\n\nAlso, VARS, FOUND and TRIED are not initialized anywhere.  We should\nprotect ourselves from environment variables the user may have with these\nnames before starting bisect to break the logic of this code.\n\nBack to the test script.\n\n> +\t\tgrep \"first bad commit could be any of\" my_bisect_log.txt &&\n> +\t\ttest_must_fail grep $HASH1 my_bisect_log.txt &&\n> +\t\ttest_must_fail grep $HASH2 my_bisect_log.txt &&\n\nThese two are easier to read with ! not test_must_fail; we do not expect\ngrep to be buggy and dump core ;-)\n\nSo perhaps the big loop should be better written like this...\n\nfilter_skipped() {\n\t...\n\t# Let's parse the output of:\n\t# \"git rev-list --bisect-vars --bisect-all ...\"\n\teval_rev_list \"$_eval\" | {\n\t\tVARS= FOUND= TRIED=\n\t\twhile read hash line\n\t\tdo\n\t\t\tcase \"$VARS,$FOUND,$TRIED,$hash\" in\n\t\t\t1,*,*,*)\n\t\t\t\t# \"bisect_foo=bar\" read from rev-list output.\n\t\t\t\techo \"$hash &&\"\n\t\t\t\t;;\n\t\t\t,*,*,---*)\n\t\t\t\t# Separator\n\t\t\t\t;;\n\n\t\t\t,,,bisect_rev*)\n\t\t\t\t# We had nothing to search.\n\t\t\t\techo \"bisect_rev= &&\"\n\t\t\t\tVARS=1\n\t\t\t\t;;\n\t\t\t,,*,bisect_rev*)\n\t\t\t\t# We did not find a good bisect rev.\n\t\t\t\t# This should happen only if the \"bad\"\n\t\t\t\t# commit is also a \"skip\" commit.\n\t\t\t\techo \"bisect_rev='$TRIED' &&\"\n\t\t\t\tVARS=1\n\t\t\t\t;;\n\t\t\t,,*,*)\n\t\t\t\t# We are searching.\n\t\t\t\tTRIED=\"${TRIED:+$TRIED|}$hash\"\n\t\t\t\tcase \"$_skip\" in\n\t\t\t\t*$hash*) ;;\n\t\t\t\t*)\n\t\t\t\t\techo \"bisect_rev=$hash &&\"\n\t\t\t\t\techo \"bisect_tried='$TRIED' &&\"\n\t\t\t\t\tFOUND=1\n\t\t\t\t\t;;\n\t\t\t\tesac\n\t\t\t\t;;\n\n\t\t\t,1,*,bisect_rev*)\n\t\t\t\t# We have already found a rev to be tested.\n\t\t\t\tVARS=1\n\t\t\t\t;;\n\t\t\t,1,*,*)\n\t\t\t\t;;\n\t\t\t*) \n\t\t\t\t# An unexpected input\n\t\t\t\techo \"die 'filter_skipped error'\"\n\t\t\t\techo >&2 \\\n\t\t\t\t\t\"VARS: '$VARS' \" \\\n\t\t\t\t\t\"FOUND: '$FOUND' \" \\\n\t\t\t\t\t\"TRIED: '$TRIED' \" \\\n\t\t\t\t\t\"hash: '$hash' \" \\\n\t\t\t\t\t\"line: '$line'\"\n\t\t\t\t;;\n\t\t\tesac\n\t\tdone\n\t\techo ':'\n\t}\n}\n"},{"id":"106396","messageId":"200902270730.56998.chriscool@tuxfamily.org","threadId":"18029","inReplyTo":"7v63ixwpnm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-02-27T06:30:55Z","receivedAt":"2009-02-27T06:30:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 26 février 2009, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > Before this patch, when the \"bad\" commit was also \"skip\"ped and\n> > when more than one commit was skipped, the \"filter_skipped\" function\n> > would have printed something like:\n>\n> Everybody knows that the problem description that comes at the beginning\n> of the commit log message talks about the state of the code before the\n> patch is applied.\n>\n> Try reading the first sentence without \"Before this patch, \".  It still\n> makes perfect sense and more importantly, it is much more readable.\n\nOk, I will drop that.\n\n> > bisect_rev=<hash1>|<hash2>\n> >\n> > (where <hash1> and <hash2> are hexadecimal sha1 hashes)\n> >\n> > and this would have been evaled later as piping \"bisect_rev=<hash1>\"\n> > into \"<hash2>\", which would have failed.\n>\n> I am a bit worried why this \"would have failed\" was not noticed.\n\nPerhaps because people do not often use \"skip\" and \"bad\" on the same commit.\nThere is an eval error printed on STDERR when this happens.\n\n> > So this patch makes the \"filter_skipped\" function properly quote\n> > what it outputs, so that it will print something like:\n> >\n> > bisect_rev=\"<hash1>|<hash2>\"\n> >\n> > which will be properly evaled later.\n> >\n> > A test case is added to the test suite.\n>\n> Thanks.  Fixes before the 1.6.2 release are very much welcomed.\n>\n> > And while at it, we also remove a spurious space where the\n> > \"exit_if_skipped_commits\" function is defined.\n>\n> Looking at:\n>\n> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- '*.sh' |\n>   sed -e 's/^[^ (]*/X/' | sort | uniq -c\n>\n> and then doing the same for only git-bisect.sh, i.e.\n>\n> $ git grep '^[a-z_A-Z][a-z_A-Z0-9]* *() *{' -- 'git-bisect.sh' |\n>   sed -e 's/^[^ (]*/X/' | sort | uniq -c\n>\n> you will notice that git-bisect is an odd-man out that uses 3 \"func () {\"\n> and 13 \"func() {\".  Majority of functions have one SP after the function\n> name.\n>\n> If we were to standardize on one style, we should consistently have SP\n> there.\n\nOk, I dropped the related change in the series I will send just after this \nemail.\n\n> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> > index dd7eac8..b5da16f 100755\n> > --- a/t/t6030-bisect-porcelain.sh\n> > +++ b/t/t6030-bisect-porcelain.sh\n> > @@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell\n> > between 2 commits' ' fi\n> >  '\n> >\n> > +# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3\n> > +# and $HASH2 is good,\n> > +# so we should not be able to tell the first bad commit\n> > +# among $HASH3 and $HASH4\n> > +test_expect_success 'bisect skip: with commit both bad and skipped' '\n> > +\tgit bisect start &&\n> > +\tgit bisect skip &&\n> > +\tgit bisect bad &&\n> > +\tgit bisect good $HASH1 &&\n> > +\tgit bisect skip &&\n> > +\tif git bisect good > my_bisect_log.txt\n>\n> An unpatched \"git-bisect\" seems to say \"32a594a3 was both good and bad\"\n> in its my_bisect_log.txt .\n\nYes, but there is also an error printed on STDERR.\n\n> This makes me suspect that we are forgetting \n> to check return status after we eval the output from filter_skipped\n> function. Shouldn't the function should string its commands together with\n> \"&&\" to protect it from a breakage like this?\n\nRight, that would be an improvement. I put it in another patch though, \nbecause it is not really needed to fix a breakage.\n\n> Also, VARS, FOUND and TRIED are not initialized anywhere.  We should\n> protect ourselves from environment variables the user may have with these\n> names before starting bisect to break the logic of this code.\n\nYeah I noticed that, I put that change in the first patch.\n\n> Back to the test script.\n>\n> > +\t\tgrep \"first bad commit could be any of\" my_bisect_log.txt &&\n> > +\t\ttest_must_fail grep $HASH1 my_bisect_log.txt &&\n> > +\t\ttest_must_fail grep $HASH2 my_bisect_log.txt &&\n>\n> These two are easier to read with ! not test_must_fail; we do not expect\n> grep to be buggy and dump core ;-)\n\nOk, there is now \"!\" in the first patch.\n\n> So perhaps the big loop should be better written like this...\n\nYeah, it will look more or less like that.\n\nThanks,\nChristian.\n"},{"id":"106411","messageId":"7v3ae0mfob.fsf@gitster.siamese.dyndns.org","threadId":"18029","inReplyTo":"200902270730.56998.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T08:45:24Z","receivedAt":"2009-02-27T08:45:24Z","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>> This makes me suspect that we are forgetting \n>> to check return status after we eval the output from filter_skipped\n>> function. Shouldn't the function should string its commands together with\n>> \"&&\" to protect it from a breakage like this?\n>\n> Right, that would be an improvement. I put it in another patch though, \n> because it is not really needed to fix a breakage.\n\nSorry, but I have to disagree.\n\nLook at the caller of filter_skipped in bisect_next():\n\n\teval=\"git rev-list --bisect-vars $BISECT_OPT $good $bad --\" &&\n\teval=\"$eval $(cat \"$GIT_DIR/BISECT_NAMES\")\" &&\n\teval=$(filter_skipped \"$eval\" \"$skip\") &&\n\teval \"$eval\" || exit\n\n\tif [ -z \"$bisect_rev\" ]; then\n\t\techo \"$bad was both good and bad\"\n\t\texit 1\n\tfi\n\nThe eval \"$eval\" in the middle, if failed properly upon seeing the quote\nbug, would have exited there, because \"|| exit\" there is all about\ncatching a broken eval string.  It was not effective.\n\nYou were lucky in this case that bisect_rev happens to be empty because\nthe bug happened to be at the place where bisect_rev was assigned to.  But\nwith a random other breakage in the filter_skipped, you would not have\nbeen so lucky.\n\nI think it is an integral part of the bugfix to string the commands\nfilter_skipped outputs with &&, so that an error while executing an\nearlier command in the output sequence is not masked by execution of other\ncommands in the output.\n\nHere is what I am thinking about queueing for 1.6.2; it may be necessary\nto apply it to 1.6.1.X (or 1.6.0.X) and merge the fix upwards.\n\nBy deepinging case/esac one level without indenting the case arms deeper,\nit happens to re-indent the case/esac construct correctly.  case arms\nshould align with case/esac, and comments on each case arm should come\ninside it.\n\n-- >8 --\n\nFrom: Christian Couder <chriscool@tuxfamily.org>\nDate: Fri, 27 Feb 2009 07:31:22 +0100\nSubject: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped\n\nWhen the \"bad\" commit was also \"skip\"ped and when more than one\ncommit was skipped, the \"filter_skipped\" function would have\nprinted something like:\n\n    bisect_rev=<hash1>|<hash2>\n\n(where <hash1> and <hash2> are hexadecimal sha1 hashes)\n\nand this would have been evaled later as piping \"bisect_rev=<hash1>\"\ninto \"<hash2>\", which would have failed.\n\nSo this patch makes the \"filter_skipped\" function properly quote\nwhat it outputs, so that it will print something like:\n\nbisect_rev='<hash1>|<hash2>'\n\nwhich will be properly evaled later.  The caller was not stopping\nproperly because the scriptlet this function returned to be evaled\nwas not strung together with && and because of this, an error in\nan earlier part of the output was simply ignored.\n\nA test case is added to the test suite.\n\nAnd while at it, we also initialize the VARS, FOUND and TRIED\nvariables, so that we protect ourselves from environment variables\nthe user may have with these names.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-bisect.sh               |   70 ++++++++++++++++++++++++------------------\n t/t6030-bisect-porcelain.sh |   25 +++++++++++++++\n 2 files changed, 65 insertions(+), 30 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 85db4ba..f863c88 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -290,56 +290,66 @@ filter_skipped() {\n \n \t# Let's parse the output of:\n \t# \"git rev-list --bisect-vars --bisect-all ...\"\n-\teval \"$_eval\" | while read hash line\n-\tdo\n-\t\tcase \"$VARS,$FOUND,$TRIED,$hash\" in\n-\t\t\t# We display some vars.\n-\t\t\t1,*,*,*) echo \"$hash $line\" ;;\n+\teval \"$_eval\" | {\n+\t\tVARS= FOUND= TRIED=\n+\t\twhile read hash line\n+\t\tdo\n+\t\t\tcase \"$VARS,$FOUND,$TRIED,$hash\" in\n+\t\t\t1,*,*,*)\n+\t\t\t\t# \"bisect_foo=bar\" read from rev-list output.\n+\t\t\t\techo \"$hash &&\" ;;\n \n-\t\t\t# Split line.\n-\t\t\t,*,*,---*) ;;\n+\t\t\t,*,*,---*)\n+\t\t\t\t# Separator\n+\t\t\t\t;;\n \n-\t\t\t# We had nothing to search.\n \t\t\t,,,bisect_rev*)\n-\t\t\t\techo \"bisect_rev=\"\n+\t\t\t\t# We had nothing to search.\n+\t\t\t\techo \"bisect_rev= &&\"\n \t\t\t\tVARS=1\n \t\t\t\t;;\n \n-\t\t\t# We did not find a good bisect rev.\n-\t\t\t# This should happen only if the \"bad\"\n-\t\t\t# commit is also a \"skip\" commit.\n \t\t\t,,*,bisect_rev*)\n-\t\t\t\techo \"bisect_rev=$TRIED\"\n+\t\t\t\t# We did not find a good bisect rev.\n+\t\t\t\t# This should happen only if the \"bad\"\n+\t\t\t\t# commit is also a \"skip\" commit.\n+\t\t\t\techo \"bisect_rev='$TRIED' &&\"\n \t\t\t\tVARS=1\n \t\t\t\t;;\n \n-\t\t\t# We are searching.\n \t\t\t,,*,*)\n+\t\t\t\t# We are searching.\n \t\t\t\tTRIED=\"${TRIED:+$TRIED|}$hash\"\n \t\t\t\tcase \"$_skip\" in\n \t\t\t\t*$hash*) ;;\n \t\t\t\t*)\n-\t\t\t\t\techo \"bisect_rev=$hash\"\n-\t\t\t\t\techo \"bisect_tried=\\\"$TRIED\\\"\"\n+\t\t\t\t\techo \"bisect_rev=$hash &&\"\n+\t\t\t\t\techo \"bisect_tried='$TRIED' &&\"\n \t\t\t\t\tFOUND=1\n \t\t\t\t\t;;\n \t\t\t\tesac\n \t\t\t\t;;\n \n-\t\t\t# We have already found a rev to be tested.\n-\t\t\t,1,*,bisect_rev*) VARS=1 ;;\n-\t\t\t,1,*,*) ;;\n-\n-\t\t\t# ???\n-\t\t\t*) die \"filter_skipped error \" \\\n-\t\t\t    \"VARS: '$VARS' \" \\\n-\t\t\t    \"FOUND: '$FOUND' \" \\\n-\t\t\t    \"TRIED: '$TRIED' \" \\\n-\t\t\t    \"hash: '$hash' \" \\\n-\t\t\t    \"line: '$line'\"\n-\t\t\t;;\n-\t\tesac\n-\tdone\n+\t\t\t,1,*,bisect_rev*)\n+\t\t\t\t# We have already found a rev to be tested.\n+\t\t\t\tVARS=1\n+\t\t\t\t;;\n+\t\t\t,1,*,*)\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\t# Unexpected input\n+\t\t\t\techo \"die 'filter_skipped error'\"\n+\t\t\t\tdie \"filter_skipped error \" \\\n+\t\t\t\t    \"VARS: '$VARS' \" \\\n+\t\t\t\t    \"FOUND: '$FOUND' \" \\\n+\t\t\t\t    \"TRIED: '$TRIED' \" \\\n+\t\t\t\t    \"hash: '$hash' \" \\\n+\t\t\t\t    \"line: '$line'\"\n+\t\t\t\t;;\n+\t\t\tesac\n+\t\tdone\n+\t\techo ':'\n+\t}\n }\n \n exit_if_skipped_commits () {\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex dd7eac8..052a6c9 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -224,6 +224,31 @@ test_expect_success 'bisect skip: cannot tell between 2 commits' '\n \tfi\n '\n \n+# $HASH1 is good, $HASH4 is both skipped and bad, we skip $HASH3\n+# and $HASH2 is good,\n+# so we should not be able to tell the first bad commit\n+# among $HASH3 and $HASH4\n+test_expect_success 'bisect skip: with commit both bad and skipped' '\n+\tgit bisect start &&\n+\tgit bisect skip &&\n+\tgit bisect bad &&\n+\tgit bisect good $HASH1 &&\n+\tgit bisect skip &&\n+\tif git bisect good > my_bisect_log.txt\n+\tthen\n+\t\techo Oops, should have failed.\n+\t\tfalse\n+\telse\n+\t\ttest $? -eq 2 &&\n+\t\tgrep \"first bad commit could be any of\" my_bisect_log.txt &&\n+\t\t! grep $HASH1 my_bisect_log.txt &&\n+\t\t! grep $HASH2 my_bisect_log.txt &&\n+\t\tgrep $HASH3 my_bisect_log.txt &&\n+\t\tgrep $HASH4 my_bisect_log.txt &&\n+\t\tgit bisect reset\n+\tfi\n+'\n+\n # We want to automatically find the commit that\n # introduced \"Another\" into hello.\n test_expect_success \\\n-- \n1.6.2.rc2.91.gf9a36\n"},{"id":"106481","messageId":"200902272205.10501.chriscool@tuxfamily.org","threadId":"18029","inReplyTo":"7v3ae0mfob.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-02-27T21:05:10Z","receivedAt":"2009-02-27T21:05:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 27 février 2009, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> >> This makes me suspect that we are forgetting\n> >> to check return status after we eval the output from filter_skipped\n> >> function. Shouldn't the function should string its commands together\n> >> with \"&&\" to protect it from a breakage like this?\n> >\n> > Right, that would be an improvement. I put it in another patch though,\n> > because it is not really needed to fix a breakage.\n\nHere I wanted to say that I think it fixes a separate breakage or another \nkind of breakage rather than not a breakage. Sorry.\n\n> Sorry, but I have to disagree.\n>\n> Look at the caller of filter_skipped in bisect_next():\n>\n> \teval=\"git rev-list --bisect-vars $BISECT_OPT $good $bad --\" &&\n> \teval=\"$eval $(cat \"$GIT_DIR/BISECT_NAMES\")\" &&\n> \teval=$(filter_skipped \"$eval\" \"$skip\") &&\n> \teval \"$eval\" || exit\n>\n> \tif [ -z \"$bisect_rev\" ]; then\n> \t\techo \"$bad was both good and bad\"\n> \t\texit 1\n> \tfi\n>\n> The eval \"$eval\" in the middle, if failed properly upon seeing the quote\n> bug, would have exited there, because \"|| exit\" there is all about\n> catching a broken eval string.  It was not effective.\n\nYes, but before I introduced filter_skipped there was:\n\n\teval=\"git rev-list --bisect-vars $good $bad --\" &&\n        eval=\"$eval $(cat \"$GIT_DIR/BISECT_NAMES\")\" &&\n\teval=$(eval \"$eval\") &&\n        eval \"$eval\" || exit\n\nso the output of \"git rev-list --bisect-vars ...\" was evaled, and this \noutput is something like that:\n\n$ git rev-list --bisect-vars HEAD ^HEAD~3\nbisect_rev=c24505030fad7cc2872e0a0fd0f44e05571a0ad8\nbisect_nr=1\nbisect_good=0\nbisect_bad=1\nbisect_all=3\n\nwhere there is no \"&&\" at the end of the commands to string them together.\nSo this breakage already existed before I introduced \"filter_skipped\".\n\n> You were lucky in this case that bisect_rev happens to be empty because\n> the bug happened to be at the place where bisect_rev was assigned to. \n> But with a random other breakage in the filter_skipped, you would not\n> have been so lucky.\n\nYeah, I should have improved on the existing design instead of blindly \nfollowing it. I hope I won't get sued for that ;-)\n\n> I think it is an integral part of the bugfix to string the commands\n> filter_skipped outputs with &&, so that an error while executing an\n> earlier command in the output sequence is not masked by execution of\n> other commands in the output.\n\nSo you should perhaps squash the following hunk to the patch:\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 08e31d6..980d73c 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -284,7 +284,13 @@ filter_skipped() {\n        _skip=\"$2\"\n\n        if [ -z \"$_skip\" ]; then\n-               eval \"$_eval\"\n+               eval \"$_eval\" | {\n+                       while read line\n+                       do\n+                               echo \"$line &&\"\n+                       done\n+                       echo ':'\n+               }\n                return\n        fi\n\nas this will string the commands together when there are no skipped commits \ntoo.\n\n> Here is what I am thinking about queueing for 1.6.2; it may be necessary\n> to apply it to 1.6.1.X (or 1.6.0.X) and merge the fix upwards.\n\nIt looks good to me. But frankly I feel always strange when a patch like \nthis one, where you did most of the code change, get attributed to me. I \nwould have prefered that you added a patch attributed to you on top of mine \nif possible.\n\nBest regards,\nChristian.\n"},{"id":"106486","messageId":"7vmyc7ik5i.fsf@gitster.siamese.dyndns.org","threadId":"18029","inReplyTo":"200902272205.10501.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: fix quoting TRIED revs when \"bad\" commit is also \"skip\"ped","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-27T22:34:33Z","receivedAt":"2009-02-27T22:34:33Z","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> So you should perhaps squash the following hunk to the patch:\n> ...\n> as this will string the commands together when there are no skipped commits \n> too.\n\nOk, good eyes.\n\nEven though this part is directly from rev-list --bisect-vars and is much\nless likely to be malformed than the list of commands filter_skipped\nfunction produces, I agree it is a good discipline to protect it from the\nbreakage the same way.\n\nThanks.\n"}]}