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

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

[Torstein Hegge]

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 <hegge@resisty.net>
---
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

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

[Christian Couder]

From: Torstein Hegge <hegge@resisty.net>
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 <hegge@resisty.net>
> ---
> 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.

>  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)

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.

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

[Torstein Hegge]

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.

> > 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

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

[Junio C Hamano]

Torstein Hegge <hegge@resisty.net> writes:

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

It is primarily because show-ref is slightly newer.  When you have a
full refname (e.g. refs/bisect/bad) and not an arbitrary object name
that is spelled in a random way (e.g. master~24):

       show-ref --verify refs/bisect/bad

is a perfectly valid way to make sure it _is_ an existing ref.

Cf. 358ddb62cfd0 (Add "git show-ref" builtin command, 2006-09-15)

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

[Torstein Hegge]

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:
> > 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 <hegge@resisty.net>
---
 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

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

[Junio C Hamano]

Torstein Hegge <hegge@resisty.net> writes:

> 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.

We skipped them because we can gain _no_ information from testing
these commits. They are not even "possibly bad", but are "unknown".

So it feels to me that by definition listing them would not be
useful. What am I missing?

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

[Torstein Hegge]

On Mon, Apr 22, 2013 at 14:13:00 -0700, Junio C Hamano wrote:
> Torstein Hegge <hegge@resisty.net> writes:
> 
> > 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.
> 
> We skipped them because we can gain _no_ information from testing
> these commits. They are not even "possibly bad", but are "unknown".
> 
> So it feels to me that by definition listing them would not be
> useful. What am I missing?

The information lies in that those commits are the only commits with an
unknown state. So if the bisecter hands off the bisect log to someone
else when they can't test further, the current status is recorded.

I think part of the reason I started looking at this is that there are
no good way to see what git said after the previous 'git bisect
good/bad' if the terminal output is lost. And lost terminal output is
fairly likely if you are bisecting something that requires reboots for
each test.

But I don't feel very strongly about this. It was based on Christian's
idea, so unless he comes up with some compelling arguments I'll drop it.

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

[Junio C Hamano]

Torstein Hegge <hegge@resisty.net> writes:

> On Mon, Apr 22, 2013 at 14:13:00 -0700, Junio C Hamano wrote:
>> Torstein Hegge <hegge@resisty.net> writes:
>> 
>> > 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.
>> 
>> We skipped them because we can gain _no_ information from testing
>> these commits. They are not even "possibly bad", but are "unknown".
>> 
>> So it feels to me that by definition listing them would not be
>> useful. What am I missing?
>
> The information lies in that those commits are the only commits with an
> unknown state. So if the bisecter hands off the bisect log to someone
> else when they can't test further, the current status is recorded.

That is an interesting use case: "I've narrowed it down somewhat,
but there are a few commits I do not have proper hardware for to
test them, could you take it over from here?"

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

[Christian Couder]

From: Torstein Hegge <hegge@resisty.net>
Subject: Re: [PATCH] bisect: Store first bad commit as comment in log file
Date: Tue, 23 Apr 2013 00:20:58 +0200

> On Mon, Apr 22, 2013 at 14:13:00 -0700, Junio C Hamano wrote:
>> Torstein Hegge <hegge@resisty.net> writes:
>> 
>> > 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.
>> 
>> We skipped them because we can gain _no_ information from testing
>> these commits. They are not even "possibly bad", but are "unknown".
>> 
>> So it feels to me that by definition listing them would not be
>> useful. What am I missing?
> 
> The information lies in that those commits are the only commits with an
> unknown state. So if the bisecter hands off the bisect log to someone
> else when they can't test further, the current status is recorded.

Yeah, I think it is a good enough reason for your patch.
 
> I think part of the reason I started looking at this is that there are
> no good way to see what git said after the previous 'git bisect
> good/bad' if the terminal output is lost. And lost terminal output is
> fairly likely if you are bisecting something that requires reboots for
> each test.

Yeah, I agree.

> But I don't feel very strongly about this. It was based on Christian's
> idea, so unless he comes up with some compelling arguments I'll drop it.

I think your arguments are good enough.

Thanks,
Christian.

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

[Torstein Hegge]

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 <hegge@resisty.net>
---
 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

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

[Junio C Hamano]

Torstein Hegge <hegge@resisty.net> writes:

> 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 <hegge@resisty.net>
> ---
> 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"

As this is "# commented out", replaying will safely ignore this new
record, so this should be safe.