patchbisect: 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 <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(-)

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

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.
 

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.

Thanks, Christian.

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

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.

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.

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

Yes, I should probably have done that.

Thanks.

Torstein

Torstein Hegge <hegge@resisty.net> writes:

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)

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

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

Torstein Hegge <hegge@resisty.net> writes:

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?

On Mon, Apr 22, 2013 at 14:13:00 -0700, Junio C Hamano wrote:

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.

Torstein Hegge <hegge@resisty.net> writes:

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

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

Yeah, I think it is a good enough reason for your patch.
 

Yeah, I agree.

I think your arguments are good enough.

Thanks, Christian.

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

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

Torstein Hegge <hegge@resisty.net> writes:

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