Skip to content

Commit f821d08

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

File tree

2 files changed

+192
-25
lines changed

2 files changed

+192
-25
lines changed

‎git-bisect.sh‎

Lines changed: 102 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -243,33 +243,18 @@ bisect_auto_next() {
243243
bisect_next_check && bisect_next || :
244244
}
245245

246-
eval_rev_list() {
247-
_eval="$1"
248-
249-
eval $_eval
250-
res=$?
251-
252-
if [ $res -ne 0 ]; then
253-
echo >&2 "'git rev-list --bisect-vars' failed:"
254-
echo >&2 "maybe you mistake good and bad revs?"
255-
exit $res
256-
fi
257-
258-
return $res
259-
}
260-
261246
filter_skipped() {
262247
_eval="$1"
263248
_skip="$2"
264249

265250
if [ -z "$_skip" ]; then
266-
eval_rev_list "$_eval"
251+
eval "$_eval"
267252
return
268253
fi
269254

270255
# Let's parse the output of:
271256
# "git rev-list --bisect-vars --bisect-all ..."
272-
eval_rev_list "$_eval" | while read hash line
257+
eval "$_eval" | while read hash line
273258
do
274259
case "$VARS,$FOUND,$TRIED,$hash" in
275260
# We display some vars.
@@ -332,20 +317,113 @@ exit_if_skipped_commits () {
332317
fi
333318
}
334319

320+
bisect_checkout() {
321+
_rev="$1"
322+
_msg="$2"
323+
echo "Bisecting: $_msg"
324+
git checkout -q "$_rev" || exit
325+
git show-branch "$_rev"
326+
}
327+
328+
is_among() {
329+
_rev="$1"
330+
_list="$2"
331+
case "$_list" in *$_rev*) return 0 ;; esac
332+
return 1
333+
}
334+
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+
343+
handle_bad_merge_base() {
344+
_badmb="$1"
345+
_good="$2"
346+
if is_testing_merge_base "$_badmb"; then
347+
cat >&2 <<EOF
348+
The merge base $_badmb is bad.
349+
This means the bug has been fixed between $_badmb and [$_good].
350+
EOF
351+
exit 3
352+
else
353+
cat >&2 <<EOF
354+
Some good revs are not ancestor of the bad rev.
355+
git bisect cannot work properly in this case.
356+
Maybe you mistake good and bad revs?
357+
EOF
358+
exit 1
359+
fi
360+
}
361+
362+
handle_skipped_merge_base() {
363+
_mb="$1"
364+
_bad="$2"
365+
_good="$3"
366+
cat >&2 <<EOF
367+
Warning: the merge base between $_bad and [$_good] must be skipped.
368+
So we cannot be sure the first bad commit is between $_mb and $_bad.
369+
We continue anyway.
370+
EOF
371+
}
372+
373+
check_merge_bases() {
374+
_bad="$1"
375+
_good="$2"
376+
_skip="$3"
377+
for _mb in $(git merge-base --all $_bad $_good)
378+
do
379+
if is_among "$_mb" "$_good"; then
380+
continue
381+
elif test "$_mb" = "$_bad"; then
382+
handle_bad_merge_base "$_bad" "$_good"
383+
elif is_among "$_mb" "$_skip"; then
384+
handle_skipped_merge_base "$_mb" "$_bad" "$_good"
385+
else
386+
mark_testing_merge_base "$_mb"
387+
bisect_checkout "$_mb" "a merge base must be tested"
388+
checkout_done=1
389+
return
390+
fi
391+
done
392+
}
393+
394+
check_good_are_ancestors_of_bad() {
395+
_bad="$1"
396+
_good=$(echo $2 | sed -e 's/\^//g')
397+
_skip="$3"
398+
399+
# Bisecting with no good rev is ok
400+
test -z "$_good" && return
401+
402+
_side=$(git rev-list $_good ^$_bad)
403+
if test -n "$_side"; then
404+
check_merge_bases "$_bad" "$_good" "$_skip"
405+
fi
406+
}
407+
335408
bisect_next() {
336409
case "$#" in 0) ;; *) usage ;; esac
337410
bisect_autostart
338411
bisect_next_check good
339412

413+
# Get bad, good and skipped revs
414+
bad=$(git rev-parse --verify refs/bisect/bad) &&
415+
good=$(git for-each-ref --format='^%(objectname)' \
416+
"refs/bisect/good-*" | tr '\012' ' ') &&
340417
skip=$(git for-each-ref --format='%(objectname)' \
341-
"refs/bisect/skip-*" | tr '\012' ' ') || exit
418+
"refs/bisect/skip-*" | tr '\012' ' ') &&
342419

420+
# Maybe some merge bases must be tested first
421+
check_good_are_ancestors_of_bad "$bad" "$good" "$skip" || exit
422+
test "$checkout_done" -eq "1" && checkout_done='' && return
423+
424+
# Get bisection information
343425
BISECT_OPT=''
344426
test -n "$skip" && BISECT_OPT='--bisect-all'
345-
346-
bad=$(git rev-parse --verify refs/bisect/bad) &&
347-
good=$(git for-each-ref --format='^%(objectname)' \
348-
"refs/bisect/good-*" | tr '\012' ' ') &&
349427
eval="git rev-list --bisect-vars $BISECT_OPT $good $bad --" &&
350428
eval="$eval $(cat "$GIT_DIR/BISECT_NAMES")" &&
351429
eval=$(filter_skipped "$eval" "$skip") &&
@@ -366,9 +444,7 @@ bisect_next() {
366444
# commit is also a "skip" commit (see above).
367445
exit_if_skipped_commits "$bisect_rev"
368446

369-
echo "Bisecting: $bisect_nr revisions left to test after this"
370-
git checkout -q "$bisect_rev" || exit
371-
git show-branch "$bisect_rev"
447+
bisect_checkout "$bisect_rev" "$bisect_nr revisions left to test after this"
372448
}
373449

374450
bisect_visualize() {
@@ -415,6 +491,7 @@ bisect_clean_state() {
415491
do
416492
git update-ref -d $ref $hash || exit
417493
done
494+
rm -f "$GIT_DIR/BISECT_MERGE_BASES" &&
418495
rm -f "$GIT_DIR/BISECT_LOG" &&
419496
rm -f "$GIT_DIR/BISECT_NAMES" &&
420497
rm -f "$GIT_DIR/BISECT_RUN" &&

‎t/t6030-bisect-porcelain.sh‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,96 @@ test_expect_success 'bisect does not create a "bisect" branch' '
350350
git branch -D bisect
351351
'
352352

353+
# This creates a "side" branch to test "siblings" cases.
354+
#
355+
# H1-H2-H3-H4-H5-H6-H7 <--other
356+
# \
357+
# S5-S6-S7 <--side
358+
#
359+
test_expect_success 'side branch creation' '
360+
git bisect reset &&
361+
git checkout -b side $HASH4 &&
362+
add_line_into_file "5(side): first line on a side branch" hello2 &&
363+
SIDE_HASH5=$(git rev-parse --verify HEAD) &&
364+
add_line_into_file "6(side): second line on a side branch" hello2 &&
365+
SIDE_HASH6=$(git rev-parse --verify HEAD) &&
366+
add_line_into_file "7(side): third line on a side branch" hello2 &&
367+
SIDE_HASH7=$(git rev-parse --verify HEAD)
368+
'
369+
370+
test_expect_success 'good merge base when good and bad are siblings' '
371+
git bisect start "$HASH7" "$SIDE_HASH7" > my_bisect_log.txt &&
372+
grep "merge base must be tested" my_bisect_log.txt &&
373+
grep $HASH4 my_bisect_log.txt &&
374+
git bisect good > my_bisect_log.txt &&
375+
test_must_fail grep "merge base must be tested" my_bisect_log.txt &&
376+
grep $HASH6 my_bisect_log.txt &&
377+
git bisect reset
378+
'
379+
test_expect_success 'skipped merge base when good and bad are siblings' '
380+
git bisect start "$SIDE_HASH7" "$HASH7" > my_bisect_log.txt &&
381+
grep "merge base must be tested" my_bisect_log.txt &&
382+
grep $HASH4 my_bisect_log.txt &&
383+
git bisect skip > my_bisect_log.txt 2>&1 &&
384+
grep "Warning" my_bisect_log.txt &&
385+
grep $SIDE_HASH6 my_bisect_log.txt &&
386+
git bisect reset
387+
'
388+
389+
test_expect_success 'bad merge base when good and bad are siblings' '
390+
git bisect start "$HASH7" HEAD > my_bisect_log.txt &&
391+
grep "merge base must be tested" my_bisect_log.txt &&
392+
grep $HASH4 my_bisect_log.txt &&
393+
test_must_fail git bisect bad > my_bisect_log.txt 2>&1 &&
394+
grep "merge base $HASH4 is bad" my_bisect_log.txt &&
395+
grep "fixed between $HASH4 and \[$SIDE_HASH7\]" my_bisect_log.txt &&
396+
git bisect reset
397+
'
398+
399+
# This creates a few more commits (A and B) to test "siblings" cases
400+
# when a good and a bad rev have many merge bases.
401+
#
402+
# We should have the following:
403+
#
404+
# H1-H2-H3-H4-H5-H6-H7
405+
# \ \ \
406+
# S5-A \
407+
# \ \
408+
# S6-S7----B
409+
#
410+
# And there A and B have 2 merge bases (S5 and H5) that should be
411+
# reported by "git merge-base --all A B".
412+
#
413+
test_expect_success 'many merge bases creation' '
414+
git checkout "$SIDE_HASH5" &&
415+
git merge -m "merge HASH5 and SIDE_HASH5" "$HASH5" &&
416+
A_HASH=$(git rev-parse --verify HEAD) &&
417+
git checkout side &&
418+
git merge -m "merge HASH7 and SIDE_HASH7" "$HASH7" &&
419+
B_HASH=$(git rev-parse --verify HEAD) &&
420+
git merge-base --all "$A_HASH" "$B_HASH" > merge_bases.txt &&
421+
test $(wc -l < merge_bases.txt) = "2" &&
422+
grep "$HASH5" merge_bases.txt &&
423+
grep "$SIDE_HASH5" merge_bases.txt
424+
'
425+
426+
test_expect_success 'good merge bases when good and bad are siblings' '
427+
git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
428+
grep "merge base must be tested" my_bisect_log.txt &&
429+
git bisect good > my_bisect_log2.txt &&
430+
grep "merge base must be tested" my_bisect_log2.txt &&
431+
{
432+
{
433+
grep "$SIDE_HASH5" my_bisect_log.txt &&
434+
grep "$HASH5" my_bisect_log2.txt
435+
} || {
436+
grep "$SIDE_HASH5" my_bisect_log2.txt &&
437+
grep "$HASH5" my_bisect_log.txt
438+
}
439+
} &&
440+
git bisect reset
441+
'
442+
353443
#
354444
#
355445
test_done

0 commit comments

Comments
 (0)