From f821d0892173e4e46a71fef4d06995f7a81c9296 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 22 Aug 2008 05:52:22 +0200 Subject: bisect: test merge base if good rev is not an ancestor of bad rev Before this patch, "git bisect", when it was given some good revs that are not ancestor of the bad rev, didn't check if the merge bases were good. "git bisect" just supposed that the user knew what he was doing, and that, when he said the revs were good, he knew that it meant that all the revs in the history leading to the good revs were also considered good. But in pratice, the user may not know that a good rev is not an ancestor of the bad rev, or he may not know/remember that all revs leading to the good rev will be considered good. So he may give a good rev that is a sibling, instead of an ancestor, of the bad rev, when in fact there can be one rev becoming good in the branch of the good rev (because the bug was already fixed there, for example) instead of one rev becoming bad in the branch of the bad rev. For example, if there is the following history: A--B--C--D \ E--F and we launch "git bisect start D F" then only C and D would have been considered as possible first bad commit before this patch. This could invite user errors; F could be the commit that fixes the bug that exists everywhere else. The purpose of this patch is to detect when "git bisect" is passed some good revs that are not ancestors of the bad rev, and then to first ask the user to test the merge bases between the good and bad revs. If the merge bases are good then all is fine, we can continue bisecting. Otherwise, if one merge base is bad, it means that the assumption that all revs leading to the good one are good too is wrong and we error out. In the case where one merge base is skipped we issue a warning and then continue bisecting anyway. These checks will also catch the case where good and bad have been mistaken. This means that we can remove the check that was done latter on the output of "git rev-list --bisect-vars". Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 127 +++++++++++++++++++++++++++++++++++--------- t/t6030-bisect-porcelain.sh | 90 +++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 25 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 97ac600873..b314d47704 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -243,33 +243,18 @@ bisect_auto_next() { bisect_next_check && bisect_next || : } -eval_rev_list() { - _eval="$1" - - eval $_eval - res=$? - - if [ $res -ne 0 ]; then - echo >&2 "'git rev-list --bisect-vars' failed:" - echo >&2 "maybe you mistake good and bad revs?" - exit $res - fi - - return $res -} - filter_skipped() { _eval="$1" _skip="$2" if [ -z "$_skip" ]; then - eval_rev_list "$_eval" + eval "$_eval" return fi # Let's parse the output of: # "git rev-list --bisect-vars --bisect-all ..." - eval_rev_list "$_eval" | while read hash line + eval "$_eval" | while read hash line do case "$VARS,$FOUND,$TRIED,$hash" in # We display some vars. @@ -332,20 +317,113 @@ exit_if_skipped_commits () { fi } +bisect_checkout() { + _rev="$1" + _msg="$2" + echo "Bisecting: $_msg" + git checkout -q "$_rev" || exit + git show-branch "$_rev" +} + +is_among() { + _rev="$1" + _list="$2" + case "$_list" in *$_rev*) return 0 ;; esac + return 1 +} + +is_testing_merge_base() { + grep "^testing $1$" "$GIT_DIR/BISECT_MERGE_BASES" >/dev/null 2>&1 +} + +mark_testing_merge_base() { + echo "testing $1" >> "$GIT_DIR/BISECT_MERGE_BASES" +} + +handle_bad_merge_base() { + _badmb="$1" + _good="$2" + if is_testing_merge_base "$_badmb"; then + cat >&2 <&2 <&2 < my_bisect_log.txt && + grep "merge base must be tested" my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + git bisect good > my_bisect_log.txt && + test_must_fail grep "merge base must be tested" my_bisect_log.txt && + grep $HASH6 my_bisect_log.txt && + git bisect reset +' +test_expect_success 'skipped merge base when good and bad are siblings' ' + git bisect start "$SIDE_HASH7" "$HASH7" > my_bisect_log.txt && + grep "merge base must be tested" my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + git bisect skip > my_bisect_log.txt 2>&1 && + grep "Warning" my_bisect_log.txt && + grep $SIDE_HASH6 my_bisect_log.txt && + git bisect reset +' + +test_expect_success 'bad merge base when good and bad are siblings' ' + git bisect start "$HASH7" HEAD > my_bisect_log.txt && + grep "merge base must be tested" my_bisect_log.txt && + grep $HASH4 my_bisect_log.txt && + test_must_fail git bisect bad > my_bisect_log.txt 2>&1 && + grep "merge base $HASH4 is bad" my_bisect_log.txt && + grep "fixed between $HASH4 and \[$SIDE_HASH7\]" my_bisect_log.txt && + git bisect reset +' + +# This creates a few more commits (A and B) to test "siblings" cases +# when a good and a bad rev have many merge bases. +# +# We should have the following: +# +# H1-H2-H3-H4-H5-H6-H7 +# \ \ \ +# S5-A \ +# \ \ +# S6-S7----B +# +# And there A and B have 2 merge bases (S5 and H5) that should be +# reported by "git merge-base --all A B". +# +test_expect_success 'many merge bases creation' ' + git checkout "$SIDE_HASH5" && + git merge -m "merge HASH5 and SIDE_HASH5" "$HASH5" && + A_HASH=$(git rev-parse --verify HEAD) && + git checkout side && + git merge -m "merge HASH7 and SIDE_HASH7" "$HASH7" && + B_HASH=$(git rev-parse --verify HEAD) && + git merge-base --all "$A_HASH" "$B_HASH" > merge_bases.txt && + test $(wc -l < merge_bases.txt) = "2" && + grep "$HASH5" merge_bases.txt && + grep "$SIDE_HASH5" merge_bases.txt +' + +test_expect_success 'good merge bases when good and bad are siblings' ' + git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt && + grep "merge base must be tested" my_bisect_log.txt && + git bisect good > my_bisect_log2.txt && + grep "merge base must be tested" my_bisect_log2.txt && + { + { + grep "$SIDE_HASH5" my_bisect_log.txt && + grep "$HASH5" my_bisect_log2.txt + } || { + grep "$SIDE_HASH5" my_bisect_log2.txt && + grep "$HASH5" my_bisect_log.txt + } + } && + git bisect reset +' + # # test_done -- cgit v1.2.3 From c9c4e2d5a25bbf637780f9b83e74c2a26fb957f5 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 22 Aug 2008 05:52:29 +0200 Subject: bisect: only check merge bases when needed When one good revision is not an ancestor of the bad revision, the merge bases between the good and the bad revision should be checked to make sure that they are also good revisions. A previous patch takes care of that, but it may check the merge bases more often than really needed. In fact the previous patch did not try to optimize this as much as possible because it is not so simple. So this is the purpose of this patch. One may think that when all the merge bases have been checked then we can save a flag, so that we don't need to check the merge bases again during the bisect process. The problem is that the user may choose to checkout and test something completely different from what the bisect process suggested. In this case we have to check the merge bases again, because there may be new merge bases relevant to the bisect process. That's why, in this patch, when we detect that the user tested something else than what the bisect process suggested, we remove the flag that says that we don't need to check the merge bases again. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 48 ++++++++++++++++++++++++++++++++------------- t/t6030-bisect-porcelain.sh | 24 +++++++++++++++++++++++ 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index b314d47704..69a9a565e0 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -172,6 +172,25 @@ bisect_write() { test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG" } +is_expected_rev() { + test -f "$GIT_DIR/BISECT_EXPECTED_REV" && + test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV") +} + +mark_expected_rev() { + echo "$1" > "$GIT_DIR/BISECT_EXPECTED_REV" +} + +check_expected_revs() { + for _rev in "$@"; do + if ! is_expected_rev "$_rev"; then + rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" + rm -f "$GIT_DIR/BISECT_EXPECTED_REV" + return + fi + done +} + bisect_state() { bisect_autostart state=$1 @@ -181,7 +200,8 @@ bisect_state() { 1,bad|1,good|1,skip) rev=$(git rev-parse --verify HEAD) || die "Bad rev input: HEAD" - bisect_write "$state" "$rev" ;; + bisect_write "$state" "$rev" + check_expected_revs "$rev" ;; 2,bad|*,good|*,skip) shift eval='' @@ -191,7 +211,8 @@ bisect_state() { die "Bad rev input: $rev" eval="$eval bisect_write '$state' '$sha'; " done - eval "$eval" ;; + eval "$eval" + check_expected_revs "$@" ;; *,bad) die "'git bisect bad' can take only one argument." ;; *) @@ -321,6 +342,7 @@ bisect_checkout() { _rev="$1" _msg="$2" echo "Bisecting: $_msg" + mark_expected_rev "$_rev" git checkout -q "$_rev" || exit git show-branch "$_rev" } @@ -332,18 +354,10 @@ is_among() { return 1 } -is_testing_merge_base() { - grep "^testing $1$" "$GIT_DIR/BISECT_MERGE_BASES" >/dev/null 2>&1 -} - -mark_testing_merge_base() { - echo "testing $1" >> "$GIT_DIR/BISECT_MERGE_BASES" -} - handle_bad_merge_base() { _badmb="$1" _good="$2" - if is_testing_merge_base "$_badmb"; then + if is_expected_rev "$_badmb"; then cat >&2 < "$GIT_DIR/BISECT_ANCESTORS_OK" } bisect_next() { @@ -491,7 +510,8 @@ bisect_clean_state() { do git update-ref -d $ref $hash || exit done - rm -f "$GIT_DIR/BISECT_MERGE_BASES" && + rm -f "$GIT_DIR/BISECT_EXPECTED_REV" && + rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" && rm -f "$GIT_DIR/BISECT_LOG" && rm -f "$GIT_DIR/BISECT_NAMES" && rm -f "$GIT_DIR/BISECT_RUN" && diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index a1ce95c5a6..c163114693 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -440,6 +440,30 @@ test_expect_success 'good merge bases when good and bad are siblings' ' git bisect reset ' +check_trace() { + grep "$1" "$GIT_TRACE" | grep "\^$2" | grep "$3" >/dev/null +} + +test_expect_success 'optimized merge base checks' ' + GIT_TRACE="$(pwd)/trace.log" && + export GIT_TRACE && + git bisect start "$HASH7" "$SIDE_HASH7" > my_bisect_log.txt && + grep "merge base must be tested" my_bisect_log.txt && + grep "$HASH4" my_bisect_log.txt && + check_trace "rev-list" "$HASH7" "$SIDE_HASH7" && + git bisect good > my_bisect_log2.txt && + test -f ".git/BISECT_ANCESTORS_OK" && + test "$HASH6" = $(git rev-parse --verify HEAD) && + : > "$GIT_TRACE" && + git bisect bad > my_bisect_log3.txt && + test_must_fail check_trace "rev-list" "$HASH6" "$SIDE_HASH7" && + git bisect good "$A_HASH" > my_bisect_log4.txt && + grep "merge base must be tested" my_bisect_log4.txt && + test_must_fail test -f ".git/BISECT_ANCESTORS_OK" && + check_trace "rev-list" "$HASH6" "$A_HASH" && + unset GIT_TRACE +' + # # test_done -- cgit v1.2.3 From 6a54d976f654ef15ce6db7d52c76677dfdd0de6c Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 6 Sep 2008 07:27:03 +0200 Subject: bisect: remove "checkout_done" variable used when checking merge bases Using return values from the following functions: - check_merge_bases - check_good_are_ancestors_of_bad seems simpler. While at it, let's add some comments to better document the above functions. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 69a9a565e0..79de7017e8 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -384,6 +384,17 @@ We continue anyway. EOF } +# +# "check_merge_bases" checks that merge bases are not "bad". +# +# - If one is "good", that's good, we have nothing to do. +# - If one is "bad", it means the user assumed something wrong +# and we must exit. +# - If one is "skipped", we can't know but we should warn. +# - If we don't know, we should check it out and ask the user to test. +# +# In the last case we will return 1, and otherwise 0. +# check_merge_bases() { _bad="$1" _good="$2" @@ -398,12 +409,20 @@ check_merge_bases() { handle_skipped_merge_base "$_mb" "$_bad" "$_good" else bisect_checkout "$_mb" "a merge base must be tested" - checkout_done=1 - return + return 1 fi done + return 0 } +# +# "check_good_are_ancestors_of_bad" checks that all "good" revs are +# ancestor of the "bad" rev. +# +# If that's not the case, we need to check the merge bases. +# If a merge base must be tested by the user we return 1 and +# otherwise 0. +# check_good_are_ancestors_of_bad() { test -f "$GIT_DIR/BISECT_ANCESTORS_OK" && return @@ -417,11 +436,13 @@ check_good_are_ancestors_of_bad() { _side=$(git rev-list $_good ^$_bad) if test -n "$_side"; then + # Return if a checkout was done check_merge_bases "$_bad" "$_good" "$_skip" || return - test "$checkout_done" -eq "1" && return fi : > "$GIT_DIR/BISECT_ANCESTORS_OK" + + return 0 } bisect_next() { @@ -437,8 +458,9 @@ bisect_next() { "refs/bisect/skip-*" | tr '\012' ' ') && # 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 + check_good_are_ancestors_of_bad "$bad" "$good" "$skip" + # Return now if a checkout has already been done + test "$?" -eq "1" && return # Get bisection information BISECT_OPT='' -- cgit v1.2.3