{"thread":{"id":"14668","subject":"[PATCH 1/2 resend] bisect: test merge base if good rev is not an ancestor of bad rev","startedAt":"2008-07-25T03:34:52Z","lastAt":"2008-07-25T03:34:52Z","messageCount":1,"participants":["Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"84829","messageId":"20080725053452.d0a79095.chriscool@tuxfamily.org","threadId":"14668","inReplyTo":null,"subject":"[PATCH 1/2 resend] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-25T03:34:52Z","receivedAt":"2008-07-25T03:34:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Before this patch, \"git bisect\", when it was given some good revs that\nare not ancestor of the bad rev, didn't check if the merge bases were\ngood. \"git bisect\" just supposed that the user knew what he was doing,\nand that, when he said the revs were good, he knew that it meant that\nall the revs in the history leading to the good revs were also\nconsidered good.\n\nBut in pratice, the user may not know that a good rev is not an\nancestor of the bad rev, or he may not know/remember that all revs\nleading to the good rev will be considered good. So he may give a good\nrev that is a sibling, instead of an ancestor, of the bad rev, when in\nfact there can be one rev becoming good in the branch of the good rev\n(because the bug was already fixed there for example) instead of one\nrev becoming bad in the branch of the bad rev.\n\nFor example, if there is the following history:\n\nA-B-C-D\n   \\E-F\n\nand we launch \"git bisect start D F\" then only C and D would have been\nconsidered as possible first bad commit before this patch. This may be\nwrong because A, B and E may be bad too if the bug exists everywhere\nexcept in F that fixes it.\n\nThe purpose of this patch is to detect when \"git bisect\" is passed\nsome good revs that are not ancestor of the bad rev, and then to first\nask the user to test the merge bases between the good and bad revs.\n\nIf the merge bases are good then all is fine, we can continue\nbisecting. Otherwise, if one merge base is bad, it means that the\nassumption that all revs leading to the good one are good too is\nwrong and we error out. In the case where one merge base is skipped we\nissue a warning and then continue bisecting anyway.\n\nThese checks will also catch the case where good and bad have been\nmistaken. This means that we can remove the check that was done latter\non the output of \"git rev-list --bisect-vars\".\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-bisect.sh               |  134 +++++++++++++++++++++++++++++++++++--------\n t/t6030-bisect-porcelain.sh |   90 +++++++++++++++++++++++++++++\n 2 files changed, 199 insertions(+), 25 deletions(-)\n\n\tThis is a resend series with the following changes since the previous\n\tseries:\n\n\t- \"for ...; do\" has been replaced with \"for\" and \"do\" on 2 different\n\tlines (in patch 1/2)\n\t- a line with \"unset GIT_TRACE\" has been added to \"t/t6030\" (in patch\n\t2/2)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 3cac20d..dfe36e5 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -242,33 +242,18 @@ bisect_auto_next() {\n \tbisect_next_check && bisect_next || :\n }\n \n-eval_rev_list() {\n-\t_eval=\"$1\"\n-\n-\teval $_eval\n-\tres=$?\n-\n-\tif [ $res -ne 0 ]; then\n-\t\techo >&2 \"'git rev-list --bisect-vars' failed:\"\n-\t\techo >&2 \"maybe you mistake good and bad revs?\"\n-\t\texit $res\n-\tfi\n-\n-\treturn $res\n-}\n-\n filter_skipped() {\n \t_eval=\"$1\"\n \t_skip=\"$2\"\n \n \tif [ -z \"$_skip\" ]; then\n-\t\teval_rev_list \"$_eval\"\n+\t\teval \"$_eval\"\n \t\treturn\n \tfi\n \n \t# Let's parse the output of:\n \t# \"git rev-list --bisect-vars --bisect-all ...\"\n-\teval_rev_list \"$_eval\" | while read hash line\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@@ -331,20 +316,120 @@ exit_if_skipped_commits () {\n \tfi\n }\n \n+checkout() {\n+\t_rev=\"$1\"\n+\t_msg=\"$2\"\n+\techo \"Bisecting: $_msg\"\n+\tgit checkout -q \"$_rev\" || exit\n+\tgit show-branch \"$_rev\"\n+}\n+\n+is_among() {\n+\t_rev=\"$1\"\n+\t_list=\"$2\"\n+\tcase \"$_list\" in *$_rev*) return 0 ;; esac\n+\treturn 1\n+}\n+\n+is_merge_base_ok() {\n+\tgrep \"^$1 $2 ok$\" \"$GIT_DIR/BISECT_MERGE_BASES\" >/dev/null 2>&1\n+}\n+\n+mark_merge_base_ok() {\n+\techo \"$1 $2 ok\" >> \"$GIT_DIR/BISECT_MERGE_BASES\"\n+}\n+\n+is_testing_merge_base() {\n+\tgrep \"^testing $1$\" \"$GIT_DIR/BISECT_MERGE_BASES\" >/dev/null 2>&1\n+}\n+\n+mark_testing_merge_base() {\n+\techo \"testing $1\" >> \"$GIT_DIR/BISECT_MERGE_BASES\"\n+}\n+\n+handle_bad_merge_base() {\n+\t_badmb=\"$1\"\n+\t_g=\"$2\"\n+\tif is_testing_merge_base \"$_badmb\"; then\n+\t\tcat >&2 <<EOF\n+The merge base $_badmb is bad.\n+This means the bug has been fixed between $_badmb and $_g.\n+EOF\n+\t\texit 3\n+\telse\n+\t\tcat >&2 <<EOF\n+Some good revs are not ancestor of the bad rev.\n+git bisect cannot work properly in this case.\n+Maybe you mistake good and bad revs?\n+EOF\n+\t\texit 1\n+\tfi\n+}\n+\n+handle_skipped_merge_base() {\n+\t_bad=\"$1\"\n+\t_g=\"$2\"\n+\t_mb=\"$3\"\n+\tcat >&2 <<EOF\n+Warning: the merge base between $_bad and $_g must be skipped.\n+So we cannot be sure the first bad commit is between $_mb and $_bad.\n+We continue anyway.\n+EOF\n+}\n+\n+check_merge_bases() {\n+\t_bad=\"$1\"\n+\t_good=\"$2\"\n+\t_skip=\"$3\"\n+\tfor _g in $_good\n+\tdo\n+\t\tis_merge_base_ok \"$_bad\" \"$_g\" && continue\n+\t\tfor _mb in $(git merge-base --all $_g $_bad)\n+\t\tdo\n+\t\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n+\t\t\t\tcontinue\n+\t\t\telif test \"$_mb\" = \"$_bad\"; then\n+\t\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n+\t\t\telif is_among \"$_mb\" \"$_skip\"; then\n+\t\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n+\t\t\telse\n+\t\t\t\tmark_testing_merge_base \"$_mb\"\n+\t\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n+\t\t\t\tcheckout_done=1\n+\t\t\t\treturn\n+\t\t\tfi\n+\t\tdone\n+\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n+\tdone\n+}\n+\n+check_good_are_ancestors_of_bad() {\n+\t_bad=\"$1\"\n+\t_good=$(echo $2 | sed -e 's/\\^//g')\n+\t_skip=\"$3\"\n+\t_side=$(git rev-list $_good ^$_bad)\n+\ttest -n \"$_side\" && check_merge_bases \"$_bad\" \"$_good\" \"$_skip\"\n+}\n+\n bisect_next() {\n \tcase \"$#\" in 0) ;; *) usage ;; esac\n \tbisect_autostart\n \tbisect_next_check good\n \n+\t# Get bad, good and skipped revs\n+\tbad=$(git rev-parse --verify refs/bisect/bad) &&\n+\tgood=$(git for-each-ref --format='^%(objectname)' \\\n+\t\t\"refs/bisect/good-*\" | tr '\\012' ' ') &&\n \tskip=$(git for-each-ref --format='%(objectname)' \\\n-\t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') || exit\n+\t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') &&\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+\n+\t# Get bisection information\n \tBISECT_OPT=''\n \ttest -n \"$skip\" && BISECT_OPT='--bisect-all'\n-\n-\tbad=$(git rev-parse --verify refs/bisect/bad) &&\n-\tgood=$(git for-each-ref --format='^%(objectname)' \\\n-\t\t\"refs/bisect/good-*\" | tr '\\012' ' ') &&\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@@ -365,9 +450,7 @@ bisect_next() {\n \t# commit is also a \"skip\" commit (see above).\n \texit_if_skipped_commits \"$bisect_rev\"\n \n-\techo \"Bisecting: $bisect_nr revisions left to test after this\"\n-\tgit checkout -q \"$bisect_rev\" || exit\n-\tgit show-branch \"$bisect_rev\"\n+\tcheckout \"$bisect_rev\" \"$bisect_nr revisions left to test after this\"\n }\n \n bisect_visualize() {\n@@ -414,6 +497,7 @@ bisect_clean_state() {\n \tdo\n \t\tgit update-ref -d $ref $hash || exit\n \tdone\n+\trm -f \"$GIT_DIR/BISECT_MERGE_BASES\" &&\n \trm -f \"$GIT_DIR/BISECT_LOG\" &&\n \trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n \trm -f \"$GIT_DIR/BISECT_RUN\" &&\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 0626544..503229b 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -350,6 +350,96 @@ test_expect_success 'bisect does not create a \"bisect\" branch' '\n \tgit branch -D bisect\n '\n \n+# This creates a \"side\" branch to test \"siblings\" cases.\n+#\n+# H1-H2-H3-H4-H5-H6-H7  <--other\n+#            \\\n+#             S5-S6-S7  <--side\n+#\n+test_expect_success 'side branch creation' '\n+\tgit bisect reset &&\n+\tgit checkout -b side $HASH4 &&\n+\tadd_line_into_file \"5(side): first line on a side branch\" hello2 &&\n+\tSIDE_HASH5=$(git rev-parse --verify HEAD) &&\n+\tadd_line_into_file \"6(side): second line on a side branch\" hello2 &&\n+\tSIDE_HASH6=$(git rev-parse --verify HEAD) &&\n+\tadd_line_into_file \"7(side): third line on a side branch\" hello2 &&\n+\tSIDE_HASH7=$(git rev-parse --verify HEAD)\n+'\n+\n+test_expect_success 'good merge base when good and bad are siblings' '\n+\tgit bisect start \"$HASH7\" \"$SIDE_HASH7\" > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\tgit bisect good > my_bisect_log.txt &&\n+\ttest_must_fail grep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH6 my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+test_expect_success 'skipped merge base when good and bad are siblings' '\n+\tgit bisect start \"$SIDE_HASH7\" \"$HASH7\" > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\tgit bisect skip > my_bisect_log.txt 2>&1 &&\n+\tgrep \"Warning\" my_bisect_log.txt &&\n+\tgrep $SIDE_HASH6 my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+\n+test_expect_success 'bad merge base when good and bad are siblings' '\n+\tgit bisect start \"$HASH7\" HEAD > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\ttest_must_fail git bisect bad > my_bisect_log.txt 2>&1 &&\n+\tgrep \"merge base $HASH4 is bad\" my_bisect_log.txt &&\n+\tgrep \"fixed between $HASH4 and $SIDE_HASH7\" my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+\n+# This creates a few more commits (A and B) to test \"siblings\" cases\n+# when a good and a bad rev have many merge bases.\n+#\n+# We should have the following:\n+#\n+# H1-H2-H3-H4-H5-H6-H7\n+#            \\  \\     \\\n+#             S5-A     \\\n+#              \\        \\\n+#               S6-S7----B\n+#\n+# And there A and B have 2 merge bases (S5 and H5) that should be\n+# reported by \"git merge-base --all A B\".\n+#\n+test_expect_success 'many merge bases creation' '\n+\tgit checkout \"$SIDE_HASH5\" &&\n+\tgit merge -m \"merge HASH5 and SIDE_HASH5\" \"$HASH5\" &&\n+\tA_HASH=$(git rev-parse --verify HEAD) &&\n+\tgit checkout side &&\n+\tgit merge -m \"merge HASH7 and SIDE_HASH7\" \"$HASH7\" &&\n+\tB_HASH=$(git rev-parse --verify HEAD) &&\n+\tgit merge-base --all \"$A_HASH\" \"$B_HASH\" > merge_bases.txt &&\n+\ttest $(wc -l < merge_bases.txt) = \"2\" &&\n+\tgrep \"$HASH5\" merge_bases.txt &&\n+\tgrep \"$SIDE_HASH5\" merge_bases.txt\n+'\n+\n+test_expect_success 'good merge bases when good and bad are siblings' '\n+\tgit bisect start \"$B_HASH\" \"$A_HASH\" > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgit bisect good > my_bisect_log2.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log2.txt &&\n+\t{\n+\t\t{\n+\t\t\tgrep \"$SIDE_HASH5\" my_bisect_log.txt &&\n+\t\t\tgrep \"$HASH5\" my_bisect_log2.txt\n+\t\t} || {\n+\t\t\tgrep \"$SIDE_HASH5\" my_bisect_log2.txt &&\n+\t\t\tgrep \"$HASH5\" my_bisect_log.txt\n+\t\t}\n+\t} &&\n+\tgit bisect reset\n+'\n+\n #\n #\n test_done\n-- \n1.6.0.rc0.2.gf038\n"}]}