From d3aca58562ee5c4af5266affd942c58dcb9f064d Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 23 May 2008 00:38:46 +0200 Subject: [PATCH 1/4] bisect: add test cases to check that "git bisect start" is atomic This patch adds some test cases to check that "git bisect start" doesn't leave us in a bad state, especially when it fails. These test cases show that "git bisect start" is not atomic when it fails and leave some files like .git/BISECT_START, and in some cases some refs, over. The test failures should be fixed in latter commits. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- t/t6030-bisect-porcelain.sh | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 933f567983..7557fa1a1b 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -126,6 +126,49 @@ test_expect_success 'bisect reset removes packed refs' ' test -z "$(git for-each-ref "refs/heads/bisect")" ' +test_expect_success 'bisect start: back in good branch' ' + git branch > branch.output && + grep "* other" branch.output > /dev/null && + git bisect start $HASH4 $HASH1 -- && + git bisect good && + git bisect start $HASH4 $HASH1 -- && + git bisect bad && + git bisect reset && + git branch > branch.output && + grep "* other" branch.output > /dev/null +' + +test_expect_failure 'bisect start: no ".git/BISECT_START" if junk rev' ' + git bisect start $HASH4 $HASH1 -- && + git bisect good && + test_must_fail git bisect start $HASH4 foo -- && + git branch > branch.output && + grep "* other" branch.output > /dev/null && + test_must_fail test -e .git/BISECT_START +' + +test_expect_failure 'bisect start: no ".git/BISECT_START" if mistaken rev' ' + git bisect start $HASH4 $HASH1 -- && + git bisect good && + test_must_fail git bisect start $HASH1 $HASH4 -- && + git branch > branch.output && + grep "* other" branch.output > /dev/null && + test_must_fail test -e .git/BISECT_START +' + +test_expect_failure 'bisect start: no ".git/BISECT_START" if checkout error' ' + echo "temp stuff" > hello && + test_must_fail git bisect start $HASH4 $HASH1 -- && + git branch && + git branch > branch.output && + grep "* other" branch.output > /dev/null && + test_must_fail test -e .git/BISECT_START && + test -z "$(git for-each-ref "refs/bisect/*")" +' + +# This cleanup is needed whatever the result of the above test. +git checkout HEAD hello + # $HASH1 is good, $HASH4 is bad, we skip $HASH3 # but $HASH2 is bad, # so we should find $HASH2 as the first bad commit From 9d0cd91c4e497192e89177847d1511acea5793cd Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 23 May 2008 00:38:59 +0200 Subject: [PATCH 2/4] bisect: fix left over "BISECT_START" file when starting with junk rev Before this patch, when using for example: $ git bisect start with or that cannot be parsed as a revision, we could leave a ".git/BISECT_START" file, from a previous "git bisect start", alone. This patch makes sure that it does not happen by removing the "BISECT_START" file in "bisect_clean_state" and then always writing it again at the end of "bisect_start". Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 16 +++++++--------- t/t6030-bisect-porcelain.sh | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 164e8ed81f..0dcb526f4b 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -81,8 +81,8 @@ bisect_start() { start_head='' case "$head" in refs/heads/bisect) - branch=`cat "$GIT_DIR/BISECT_START"` - git checkout $branch || exit + start_head=$(cat "$GIT_DIR/BISECT_START") + git checkout "$start_head" || exit ;; refs/heads/*|$_x40) # This error message should only be triggered by cogito usage, @@ -134,7 +134,7 @@ bisect_start() { done sq "$@" >"$GIT_DIR/BISECT_NAMES" - test -n "$start_head" && echo "$start_head" >"$GIT_DIR/BISECT_START" + echo "$start_head" >"$GIT_DIR/BISECT_START" eval "$eval" echo "git-bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" bisect_auto_next @@ -392,12 +392,7 @@ bisect_reset() { *) usage ;; esac - if git checkout "$branch"; then - # Cleanup head-name if it got left by an old version of git-bisect - rm -f "$GIT_DIR/head-name" - rm -f "$GIT_DIR/BISECT_START" - bisect_clean_state - fi + git checkout "$branch" && bisect_clean_state } bisect_clean_state() { @@ -407,9 +402,12 @@ bisect_clean_state() { do git update-ref -d $ref $hash done + rm -f "$GIT_DIR/BISECT_START" rm -f "$GIT_DIR/BISECT_LOG" rm -f "$GIT_DIR/BISECT_NAMES" rm -f "$GIT_DIR/BISECT_RUN" + # Cleanup head-name if it got left by an old version of git-bisect + rm -f "$GIT_DIR/head-name" } bisect_replay () { diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 7557fa1a1b..68b5440917 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -138,7 +138,7 @@ test_expect_success 'bisect start: back in good branch' ' grep "* other" branch.output > /dev/null ' -test_expect_failure 'bisect start: no ".git/BISECT_START" if junk rev' ' +test_expect_success 'bisect start: no ".git/BISECT_START" if junk rev' ' git bisect start $HASH4 $HASH1 -- && git bisect good && test_must_fail git bisect start $HASH4 foo -- && From ba963de859e76a63d447345eeb3e134116d02433 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 23 May 2008 00:39:22 +0200 Subject: [PATCH 3/4] bisect: trap critical errors in "bisect_start" Before this patch, when using "git bisect start" with mistaken revs or when the checkout of the branch we want to test failed, we exited after having written files like ".git/BISECT_START", ".git/BISECT_NAMES" and after having written "refs/bisect/bad" and "refs/bisect/good-*" refs. With this patch we trap all errors that can happen when writing the new state and when we are in "bisect_next". So that we can try to clean up everything in case of problems, using "bisect_clean_state". This patch also contains a "bisect_write" cleanup to make it exit on error and return 0 otherwise. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 30 ++++++++++++++++++++++++------ t/t6030-bisect-porcelain.sh | 10 ++++------ 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 0dcb526f4b..57168b0ae4 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -133,11 +133,29 @@ bisect_start() { esac done - sq "$@" >"$GIT_DIR/BISECT_NAMES" - echo "$start_head" >"$GIT_DIR/BISECT_START" - eval "$eval" - echo "git-bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" + # + # Change state. + # In case of mistaken revs or checkout error, or signals received, + # "bisect_auto_next" below may exit or misbehave. + # We have to trap this to be able to clean up using + # "bisect_clean_state". + # + trap 'bisect_clean_state' 0 + trap 'exit 255' 1 2 3 15 + + # + # Write new start state. + # + sq "$@" >"$GIT_DIR/BISECT_NAMES" && + echo "$start_head" >"$GIT_DIR/BISECT_START" && + eval "$eval" && + echo "git-bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit + # + # Check if we can proceed to the next bisect state. + # bisect_auto_next + + trap '-' 0 } bisect_write() { @@ -149,9 +167,9 @@ bisect_write() { good|skip) tag="$state"-"$rev" ;; *) die "Bad bisect_write argument: $state" ;; esac - git update-ref "refs/bisect/$tag" "$rev" + git update-ref "refs/bisect/$tag" "$rev" || exit echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG" - test -z "$nolog" && echo "git-bisect $state $rev" >>"$GIT_DIR/BISECT_LOG" + test -n "$nolog" || echo "git-bisect $state $rev" >>"$GIT_DIR/BISECT_LOG" } bisect_state() { diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 68b5440917..c4f074d738 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -147,7 +147,7 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if junk rev' ' test_must_fail test -e .git/BISECT_START ' -test_expect_failure 'bisect start: no ".git/BISECT_START" if mistaken rev' ' +test_expect_success 'bisect start: no ".git/BISECT_START" if mistaken rev' ' git bisect start $HASH4 $HASH1 -- && git bisect good && test_must_fail git bisect start $HASH1 $HASH4 -- && @@ -156,19 +156,17 @@ test_expect_failure 'bisect start: no ".git/BISECT_START" if mistaken rev' ' test_must_fail test -e .git/BISECT_START ' -test_expect_failure 'bisect start: no ".git/BISECT_START" if checkout error' ' +test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' ' echo "temp stuff" > hello && test_must_fail git bisect start $HASH4 $HASH1 -- && git branch && git branch > branch.output && grep "* other" branch.output > /dev/null && test_must_fail test -e .git/BISECT_START && - test -z "$(git for-each-ref "refs/bisect/*")" + test -z "$(git for-each-ref "refs/bisect/*")" && + git checkout HEAD hello ' -# This cleanup is needed whatever the result of the above test. -git checkout HEAD hello - # $HASH1 is good, $HASH4 is bad, we skip $HASH3 # but $HASH2 is bad, # so we should find $HASH2 as the first bad commit From 634f246444e6a1675e351f31362e6a375dc44f70 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 23 May 2008 01:28:57 +0200 Subject: [PATCH 4/4] bisect: use a detached HEAD to bisect When "git bisect" was first written, it was not possible to checkout a detached HEAD. The detached feature appeared latter. That's why before this patch the "git bisect" process used a "bisect" branch to checkout new revisions to be tested (and also a "new-bisect" one to check if the checkouts could work). This patch makes "git bisect" checkout revisions to be tested on a detached HEAD. This simplifies the code a bit. The tests to check that "git bisect" does not start if a "bisect" or a "new-bisect" branch exists are removed as they are not relevant any more. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- git-bisect.sh | 54 ++++++++++++++++++------------------- t/t6030-bisect-porcelain.sh | 38 +++++++++++++------------- 2 files changed, 45 insertions(+), 47 deletions(-) diff --git a/git-bisect.sh b/git-bisect.sh index 57168b0ae4..4bcbaceb8b 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -63,40 +63,40 @@ bisect_autostart() { bisect_start() { # - # Verify HEAD. If we were bisecting before this, reset to the - # top-of-line master first! + # Verify HEAD. # head=$(GIT_DIR="$GIT_DIR" git symbolic-ref -q HEAD) || head=$(GIT_DIR="$GIT_DIR" git rev-parse --verify HEAD) || die "Bad HEAD - I need a HEAD" + # - # Check that we either already have BISECT_START, or that the - # branches bisect, new-bisect don't exist, to not override them. + # Check if we are bisecting. # - test -s "$GIT_DIR/BISECT_START" || - if git show-ref --verify -q refs/heads/bisect || - git show-ref --verify -q refs/heads/new-bisect; then - die 'The branches "bisect" and "new-bisect" must not exist.' - fi start_head='' - case "$head" in - refs/heads/bisect) + if test -s "$GIT_DIR/BISECT_START" + then + # Reset to the rev from where we started. start_head=$(cat "$GIT_DIR/BISECT_START") git checkout "$start_head" || exit - ;; - refs/heads/*|$_x40) - # This error message should only be triggered by cogito usage, - # and cogito users should understand it relates to cg-seek. - [ -s "$GIT_DIR/head-name" ] && die "won't bisect on seeked tree" - start_head="${head#refs/heads/}" - ;; - *) - die "Bad HEAD - strange symbolic ref" - ;; - esac + else + # Get rev from where we start. + case "$head" in + refs/heads/*|$_x40) + # This error message should only be triggered by + # cogito usage, and cogito users should understand + # it relates to cg-seek. + [ -s "$GIT_DIR/head-name" ] && + die "won't bisect on seeked tree" + start_head="${head#refs/heads/}" + ;; + *) + die "Bad HEAD - strange symbolic ref" + ;; + esac + fi # - # Get rid of any old bisect state + # Get rid of any old bisect state. # bisect_clean_state @@ -118,7 +118,7 @@ bisect_start() { break ;; *) - rev=$(git rev-parse --verify "$arg^{commit}" 2>/dev/null) || { + rev=$(git rev-parse -q --verify "$arg^{commit}") || { test $has_double_dash -eq 1 && die "'$arg' does not appear to be a valid revision" break @@ -366,9 +366,7 @@ bisect_next() { exit_if_skipped_commits "$bisect_rev" echo "Bisecting: $bisect_nr revisions left to test after this" - git branch -D new-bisect 2> /dev/null - git checkout -q -b new-bisect "$bisect_rev" || exit - git branch -M new-bisect bisect + git checkout -q "$bisect_rev" || exit git show-branch "$bisect_rev" } @@ -415,7 +413,7 @@ bisect_reset() { bisect_clean_state() { # There may be some refs packed during bisection. - git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* refs/heads/bisect | + git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* | while read ref hash do git update-ref -d $ref $hash diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index c4f074d738..0626544823 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -322,25 +322,6 @@ test_expect_success 'bisect starting with a detached HEAD' ' test $HEAD = $(cat .git/BISECT_START) && git bisect reset && test $HEAD = $(git rev-parse --verify HEAD) - -' - -test_expect_success 'bisect refuses to start if branch bisect exists' ' - git bisect reset && - git branch bisect && - test_must_fail git bisect start && - git branch -d bisect && - git checkout -b bisect && - test_must_fail git bisect start && - git checkout master && - git branch -d bisect -' - -test_expect_success 'bisect refuses to start if branch new-bisect exists' ' - git bisect reset && - git branch new-bisect && - test_must_fail git bisect start && - git branch -d new-bisect ' test_expect_success 'bisect errors out if bad and good are mistaken' ' @@ -350,6 +331,25 @@ test_expect_success 'bisect errors out if bad and good are mistaken' ' git bisect reset ' +test_expect_success 'bisect does not create a "bisect" branch' ' + git bisect reset && + git bisect start $HASH7 $HASH1 && + git branch bisect && + rev_hash4=$(git rev-parse --verify HEAD) && + test "$rev_hash4" = "$HASH4" && + git branch -D bisect && + git bisect good && + git branch bisect && + rev_hash6=$(git rev-parse --verify HEAD) && + test "$rev_hash6" = "$HASH6" && + git bisect good > my_bisect_log.txt && + grep "$HASH7 is first bad commit" my_bisect_log.txt && + git bisect reset && + rev_hash6=$(git rev-parse --verify bisect) && + test "$rev_hash6" = "$HASH6" && + git branch -D bisect +' + # # test_done