Skip to content

Commit c9c4e2d

Browse files
chriscoolgitster
chriscool
authored and
gitster
committed
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 <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
1 parent f821d08 commit c9c4e2d

File tree

2 files changed

+58
-14
lines changed

2 files changed

+58
-14
lines changed

‎git-bisect.sh‎

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,25 @@ bisect_write() {
172172
test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
173173
}
174174

175+
is_expected_rev() {
176+
test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
177+
test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
178+
}
179+
180+
mark_expected_rev() {
181+
echo "$1" > "$GIT_DIR/BISECT_EXPECTED_REV"
182+
}
183+
184+
check_expected_revs() {
185+
for _rev in "$@"; do
186+
if ! is_expected_rev "$_rev"; then
187+
rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
188+
rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
189+
return
190+
fi
191+
done
192+
}
193+
175194
bisect_state() {
176195
bisect_autostart
177196
state=$1
@@ -181,7 +200,8 @@ bisect_state() {
181200
1,bad|1,good|1,skip)
182201
rev=$(git rev-parse --verify HEAD) ||
183202
die "Bad rev input: HEAD"
184-
bisect_write "$state" "$rev" ;;
203+
bisect_write "$state" "$rev"
204+
check_expected_revs "$rev" ;;
185205
2,bad|*,good|*,skip)
186206
shift
187207
eval=''
@@ -191,7 +211,8 @@ bisect_state() {
191211
die "Bad rev input: $rev"
192212
eval="$eval bisect_write '$state' '$sha'; "
193213
done
194-
eval "$eval" ;;
214+
eval "$eval"
215+
check_expected_revs "$@" ;;
195216
*,bad)
196217
die "'git bisect bad' can take only one argument." ;;
197218
*)
@@ -321,6 +342,7 @@ bisect_checkout() {
321342
_rev="$1"
322343
_msg="$2"
323344
echo "Bisecting: $_msg"
345+
mark_expected_rev "$_rev"
324346
git checkout -q "$_rev" || exit
325347
git show-branch "$_rev"
326348
}
@@ -332,18 +354,10 @@ is_among() {
332354
return 1
333355
}
334356

335-
is_testing_merge_base() {
336-
grep "^testing $1$" "$GIT_DIR/BISECT_MERGE_BASES" >/dev/null 2>&1
337-
}
338-
339-
mark_testing_merge_base() {
340-
echo "testing $1" >> "$GIT_DIR/BISECT_MERGE_BASES"
341-
}
342-
343357
handle_bad_merge_base() {
344358
_badmb="$1"
345359
_good="$2"
346-
if is_testing_merge_base "$_badmb"; then
360+
if is_expected_rev "$_badmb"; then
347361
cat >&2 <<EOF
348362
The merge base $_badmb is bad.
349363
This means the bug has been fixed between $_badmb and [$_good].
@@ -383,7 +397,6 @@ check_merge_bases() {
383397
elif is_among "$_mb" "$_skip"; then
384398
handle_skipped_merge_base "$_mb" "$_bad" "$_good"
385399
else
386-
mark_testing_merge_base "$_mb"
387400
bisect_checkout "$_mb" "a merge base must be tested"
388401
checkout_done=1
389402
return
@@ -392,6 +405,9 @@ check_merge_bases() {
392405
}
393406

394407
check_good_are_ancestors_of_bad() {
408+
test -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
409+
return
410+
395411
_bad="$1"
396412
_good=$(echo $2 | sed -e 's/\^//g')
397413
_skip="$3"
@@ -401,8 +417,11 @@ check_good_are_ancestors_of_bad() {
401417

402418
_side=$(git rev-list $_good ^$_bad)
403419
if test -n "$_side"; then
404-
check_merge_bases "$_bad" "$_good" "$_skip"
420+
check_merge_bases "$_bad" "$_good" "$_skip" || return
421+
test "$checkout_done" -eq "1" && return
405422
fi
423+
424+
: > "$GIT_DIR/BISECT_ANCESTORS_OK"
406425
}
407426

408427
bisect_next() {
@@ -491,7 +510,8 @@ bisect_clean_state() {
491510
do
492511
git update-ref -d $ref $hash || exit
493512
done
494-
rm -f "$GIT_DIR/BISECT_MERGE_BASES" &&
513+
rm -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
514+
rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
495515
rm -f "$GIT_DIR/BISECT_LOG" &&
496516
rm -f "$GIT_DIR/BISECT_NAMES" &&
497517
rm -f "$GIT_DIR/BISECT_RUN" &&

‎t/t6030-bisect-porcelain.sh‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,30 @@ test_expect_success 'good merge bases when good and bad are siblings' '
440440
git bisect reset
441441
'
442442

443+
check_trace() {
444+
grep "$1" "$GIT_TRACE" | grep "\^$2" | grep "$3" >/dev/null
445+
}
446+
447+
test_expect_success 'optimized merge base checks' '
448+
GIT_TRACE="$(pwd)/trace.log" &&
449+
export GIT_TRACE &&
450+
git bisect start "$HASH7" "$SIDE_HASH7" > my_bisect_log.txt &&
451+
grep "merge base must be tested" my_bisect_log.txt &&
452+
grep "$HASH4" my_bisect_log.txt &&
453+
check_trace "rev-list" "$HASH7" "$SIDE_HASH7" &&
454+
git bisect good > my_bisect_log2.txt &&
455+
test -f ".git/BISECT_ANCESTORS_OK" &&
456+
test "$HASH6" = $(git rev-parse --verify HEAD) &&
457+
: > "$GIT_TRACE" &&
458+
git bisect bad > my_bisect_log3.txt &&
459+
test_must_fail check_trace "rev-list" "$HASH6" "$SIDE_HASH7" &&
460+
git bisect good "$A_HASH" > my_bisect_log4.txt &&
461+
grep "merge base must be tested" my_bisect_log4.txt &&
462+
test_must_fail test -f ".git/BISECT_ANCESTORS_OK" &&
463+
check_trace "rev-list" "$HASH6" "$A_HASH" &&
464+
unset GIT_TRACE
465+
'
466+
443467
#
444468
#
445469
test_done

0 commit comments

Comments
 (0)