[PATCH] bisect: Store first bad commit as comment in log file

When bisect successfully finds a single revision, the first bad commit
should be shown to human readers of 'git bisect log'.

This resolves the apparent disconnect between the bisection result and
the log when a bug reporter says "I know that the first bad commit is
$rev, as you can see from $(git bisect log)".

Signed-off-by: Torstein Hegge <redacted>
---
I don't know how useful the added test is, I didn't find any existing
tests that looks at the comment parts of bisect log.

 git-bisect.sh               |    8 +++++++-
 t/t6030-bisect-porcelain.sh |   18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 99efbe8..c58eea7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -311,7 +311,13 @@ bisect_next() {
 	res=$?
 
 	# Check if we should exit because bisection is finished
-	test $res -eq 10 && exit 0
+	if test $res -eq 10
+	then
+		bad_rev=$(git show-ref --hash --verify refs/bisect/bad)
+		bad_commit=$(git show-branch $bad_rev)
+		echo "# first bad commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
+		exit 0
+	fi
 
 	# Check for an error in the bisection process
 	test $res -ne 0 && exit $res

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2fce99a..6e65cdf 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh

@@ -741,4 +741,22 @@ test_expect_success 'bisect: demonstrate identification of damage boundary' "
 	git bisect reset
 "
 
+cat > expected.bisect-log <<EOF
+# bad: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
+# good: [7b7f204a749c3125d5224ed61ea2ae1187ad046f] Add <2: A new day for git> into <hello>.
+git bisect start '32a594a3fdac2d57cf6d02987e30eec68511498c' '7b7f204a749c3125d5224ed61ea2ae1187ad046f'
+# good: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
+git bisect good 3de952f2416b6084f557ec417709eac740c6818c
+# first bad commit: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
+EOF
+
+test_expect_success 'bisect log: successfull result' '
+	git bisect reset &&
+	git bisect start $HASH4 $HASH2 &&
+	git bisect good &&
+	git bisect log >bisect-log.txt &&
+	test_cmp expected.bisect-log bisect-log.txt &&
+	git bisect reset
+'
+
 test_done

-- 
1.7.10.4

From: Torstein Hegge <redacted>
Subject: [PATCH] bisect: Store first bad commit as comment in log file
Date: Sat, 13 Apr 2013 17:22:57 +0200

When bisect successfully finds a single revision, the first bad commit
should be shown to human readers of 'git bisect log'.

This resolves the apparent disconnect between the bisection result and
the log when a bug reporter says "I know that the first bad commit is
$rev, as you can see from $(git bisect log)".

I agree that it's a good idea to do that.

I wonder if we should also write something into the bisect log if for
example the bisection stopped because there are only 'skip'ped commits
left to test. But maybe this could go into another patch after this
one.
 

Signed-off-by: Torstein Hegge <redacted>
---
I don't know how useful the added test is, I didn't find any existing
tests that looks at the comment parts of bisect log.

Thanks for adding a test. It's always appreciated.

I had a look to make sure that refs/bisect/bad always refered to the
first bad commit at this point, and it is true indeed.

Maybe you could have used "git rev-parse --verify" instead of "git
show-ref --hash --verify". It looks simpler to me.

And maybe, just in case, you could have added: || die "$(gettext "Bad rev: refs/bisect/bad")"

Otherwise this patch looks good to me.

+		bad_commit=$(git show-branch $bad_rev)
+		echo "# first bad commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
+		exit 0
+	fi

Thanks,
Christian.

On Mon, Apr 15, 2013 at 06:38:09 +0200, Christian Couder wrote:

I wonder if we should also write something into the bisect log if for
example the bisection stopped because there are only 'skip'ped commits
left to test. But maybe this could go into another patch after this
one.

Yes, that would be useful, but I wasn't able to determine all the cases
that would be relevant to log. Only skipped commits left to test is one,
but bisect--helper also exits on various problems related to merge base
handling. The handling of problems related to inconsistent user input is
probably not relevant to log.

I think the successful bisect case is most important to log and the one
that requires the least amount of invasive changes.

quoted

diff --git a/git-bisect.sh b/git-bisect.sh
index 99efbe8..c58eea7 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -311,7 +311,13 @@ bisect_next() {
 	res=$?
 
 	# Check if we should exit because bisection is finished
-	test $res -eq 10 && exit 0
+	if test $res -eq 10
+	then
+		bad_rev=$(git show-ref --hash --verify refs/bisect/bad)

I had a look to make sure that refs/bisect/bad always refered to the
first bad commit at this point, and it is true indeed.

According to Documentation/git-bisect.txt, refs/bisect/bad is the proper
way to determine the first bad commit at the end of a bisection.

Maybe you could have used "git rev-parse --verify" instead of "git
show-ref --hash --verify". It looks simpler to me.

I was wondering why "git grep show-ref *.sh" gave so few users. It looks
like rev-parse is more common.

And maybe, just in case, you could have added: || die "$(gettext "Bad rev: refs/bisect/bad")"

Yes, I should probably have done that.

Otherwise this patch looks good to me.

Thanks.


Torstein

On Mon, Apr 15, 2013 at 11:53:39 +0200, Torstein Hegge wrote:

On Mon, Apr 15, 2013 at 06:38:09 +0200, Christian Couder wrote:

quoted

I wonder if we should also write something into the bisect log if for
example the bisection stopped because there are only 'skip'ped commits
left to test. But maybe this could go into another patch after this
one.

Yes, that would be useful, but I wasn't able to determine all the cases
that would be relevant to log. Only skipped commits left to test is one,
but bisect--helper also exits on various problems related to merge base
handling. The handling of problems related to inconsistent user input is
probably not relevant to log.

I took another look at this. I wasn't able to come up with anything
useful for the "The merge base $rev is bad" case, but for the "only
skipped commits left to test" case one could do something like this.

There has to be a better way to get the range of possible first bad
commits, similar to the output of 'git log --bisect --format="%H"'.

--- >8 ---

Subject: [PATCH] bisect: Log possibly bad, skipped commits at bisection end

If the bisection completes with only skipped commits left to as possible
first bad commit, output the list of possible first bad commits to human
readers of the bisection log.

Signed-off-by: Torstein Hegge <redacted>
---
 git-bisect.sh               |   10 ++++++++++
 t/t6030-bisect-porcelain.sh |   20 ++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index c58eea7..d7518e9 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -317,6 +317,16 @@ bisect_next() {
 		bad_commit=$(git show-branch $bad_rev)
 		echo "# first bad commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
 		exit 0
+	elif test $res -eq 2
+	then
+		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
+		good_revs=$(git for-each-ref --format="--not %(objectname)" "refs/bisect/good-*")
+		for skipped in $(git rev-list refs/bisect/bad $good_revs)
+		do
+			skipped_commit=$(git show-branch $skipped)
+			echo "# possible first bad commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
+		done
+		exit $res
 	fi
 
 	# Check for an error in the bisection process

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 4d3074a..064f5ce 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh

@@ -759,4 +759,24 @@ test_expect_success 'bisect log: successfull result' '
 	git bisect reset
 '
 
+cat > expected.bisect-skip-log <<EOF
+# bad: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
+# good: [7b7f204a749c3125d5224ed61ea2ae1187ad046f] Add <2: A new day for git> into <hello>.
+git bisect start '32a594a3fdac2d57cf6d02987e30eec68511498c' '7b7f204a749c3125d5224ed61ea2ae1187ad046f'
+# skip: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
+git bisect skip 3de952f2416b6084f557ec417709eac740c6818c
+# only skipped commits left to test
+# possible first bad commit: [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
+# possible first bad commit: [3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for git> into <hello>.
+EOF
+
+test_expect_success 'bisect log: only skip commits left' '
+	git bisect reset &&
+	git bisect start $HASH4 $HASH2 &&
+	test_must_fail git bisect skip &&
+	git bisect log >bisect-skip-log.txt &&
+	test_cmp expected.bisect-skip-log bisect-skip-log.txt &&
+	git bisect reset
+'
+
 test_done

-- 
1.7.10.4

On Mon, Apr 22, 2013 at 23:02:29 +0200, Torstein Hegge wrote:

There has to be a better way to get the range of possible first bad
commits, similar to the output of 'git log --bisect --format="%H"'.

I just realized that this felt clunky because I didn't understand what
'--not' does in git rev-list.

In the case where the range of skipped commits include a merge and
points in each parent marked good, I want

    git rev-list bad --not good-1 good-2

or 

    git rev-list bad ^good-1 ^good-2

but instead I did

    git rev-list bad --not good-1 --not good-2

which will include commits outside the range of skipped commits. Sorry
about that :/

--- >8 ---

Subject: [PATCH] bisect: Fix log output for multi-parent skip ranges

The bisect log output of skipped commits introduced in f989cac "bisect:
Log possibly bad, skipped commits at bisection end" should obtain the range of
skipped commits from

    git rev-list bad --not good-1 good-2

not

    git rev-list bad --not good-1 --not good-2

when the skipped range contains a merge with good points in each parent.

Signed-off-by: Torstein Hegge <redacted>
---
 git-bisect.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index d7518e9..9f064b6 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -320,8 +320,8 @@ bisect_next() {
 	elif test $res -eq 2
 	then
 		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
-		good_revs=$(git for-each-ref --format="--not %(objectname)" "refs/bisect/good-*")
-		for skipped in $(git rev-list refs/bisect/bad $good_revs)
+		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/good-*")
+		for skipped in $(git rev-list refs/bisect/bad --not $good_revs)
 		do
 			skipped_commit=$(git show-branch $skipped)
 			echo "# possible first bad commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"

-- 
1.8.3.rc1.377.g7010c6b