[PATCH 4/3] bisect: use a detached HEAD to bisect

[PATCH 4/3] bisect: use a detached HEAD to bisect

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 git-bisect.sh               |   53 ++++++++++++++++++++----------------------
 t/t6030-bisect-porcelain.sh |   38 +++++++++++++++---------------
 2 files changed, 44 insertions(+), 47 deletions(-)

	This patch should be applied on top of the series I just sent.
	But it may be for after v1.5.6.

diff --git a/git-bisect.sh b/git-bisect.sh
index 57168b0..8ec8d04 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -63,40 +63,39 @@ 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 +117,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 +365,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 +412,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 c4f074d..0626544 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
-- 
1.5.5.1.502.gb8b73.dirty

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> When "git bisect" was first written, it was not possible to
> checkout a detached HEAD. The detached feature appeared latter.
> ...
> This patch makes "git bisect" checkout revisions to be tested on
> a detached HEAD. This simplifies the code a bit.

Yay!!

One potential worry/downside is "bisect visualize".  Because <bisect>
branch was used for bisection, the _current_ commit has always been
indicated with a label.  HEAD would not get any special label in gitk,
would it?

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Christian Couder]

Le vendredi 23 mai 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > When "git bisect" was first written, it was not possible to
> > checkout a detached HEAD. The detached feature appeared latter.
> > ...
> > This patch makes "git bisect" checkout revisions to be tested on
> > a detached HEAD. This simplifies the code a bit.
>
> Yay!!
>
> One potential worry/downside is "bisect visualize".  Because <bisect>
> branch was used for bisection, the _current_ commit has always been
> indicated with a label.  HEAD would not get any special label in gitk,
> would it?

You are probably right. I will have a look at that.

Thanks,
Christian.

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Jon Loeliger]

Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> When "git bisect" was first written, it was not possible to
>> checkout a detached HEAD. The detached feature appeared latter.
>> ...
>> This patch makes "git bisect" checkout revisions to be tested on
>> a detached HEAD. This simplifies the code a bit.
> 
> Yay!!
> 
> One potential worry/downside is "bisect visualize".  Because <bisect>
> branch was used for bisection, the _current_ commit has always been
> indicated with a label.  HEAD would not get any special label in gitk,
> would it?

Hrm.  This seems like a potential issue to me as I occasionally
do want to view where it is and possibly adjust it up or down a
few commits depending on context.

Perhaps a "git bisect addfoo" sort of command is needed?  The goal
would be to "add some form of {branch,label,tag}" so it could be named,
seen and possibly moved.

jdl

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Linus Torvalds]

On Thu, 22 May 2008, Junio C Hamano wrote:
> 
> One potential worry/downside is "bisect visualize".  Because <bisect>
> branch was used for bisection, the _current_ commit has always been
> indicated with a label.  HEAD would not get any special label in gitk,
> would it?

This is a general problem in gitk. 

It's worse than not showing a label, btw. If it doesn't realize what the 
HEAD is (and it won't), it also doesn't show the fake "uncommitted 
changes" commit(s).

Test by doing something like

	git checkout HEAD^
	echo "Dummy change" >> Makefile
	gitk

and note the lack of both pointer to detached head and the lack of notice 
about local uncommitted changes.

Paul?

		Linus

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Paul Mackerras]

Linus Torvalds writes:

> This is a general problem in gitk. 
> 
> It's worse than not showing a label, btw. If it doesn't realize what the 
> HEAD is (and it won't), it also doesn't show the fake "uncommitted 
> changes" commit(s).

I could make it show "HEAD" in a green box for a detached head easily
enough.  That could be ambiguous if you had a branch called HEAD, I
suppose, but having a branch called HEAD would be deeply confusing
anyway. :)  Do you have any alternative suggestion for how to display
a detached head?

Paul.

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Linus Torvalds]

On Sat, 24 May 2008, Paul Mackerras wrote:
> 
> I could make it show "HEAD" in a green box for a detached head easily
> enough.  That could be ambiguous if you had a branch called HEAD, I
> suppose, but having a branch called HEAD would be deeply confusing
> anyway. :)  Do you have any alternative suggestion for how to display
> a detached head?

Well, it would be really nice if the "active commit" was always visible 
some way.

Of course, _usually_ it's just the top commit, and it's obvious that way 
which one is the checked-out one, but if you do "gitk --all" or just 
generally have multiple branches, right now it's hard to see what commit 
is the checked-out one, regardless of whether it's detached or not.

I think "HEAD" in a green box would solve that too, but on the other hand, 
we have a *lot* of boxes already. For people who mainly just track another 
repo, you already have one box saying "master", and another one saying 
"remotes/origin/master", and adding yet *another* box saying HEAD that 
just points to the same commit will work, but do we really want that?

I actually like the red circle for "Local uncommitted changes". Maybe we 
can use a similar visual clue for "currently checked out". You already 
picked green for the "added to the index" case, so we have the three 
primary RGB colors already used, but we could make it just be a deep 
yellow.

Of course, maybe people hate lots of colos already, and something more 
akin to the text background thing that we use for the selected commit 
would be better.

I dunno. There's so many options.

Here's a "make it yellow" patch.

		Linus
----
 gitk-git/gitk |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 9ab6dba..94ca826 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4841,7 +4841,8 @@ proc drawcmittext {id row col} {
     global cmitlisted commitinfo rowidlist parentlist
     global rowtextx idpos idtags idheads idotherrefs
     global linehtag linentag linedtag selectedline
-    global canvxmax boldrows boldnamerows fgcolor nullid nullid2
+    global canvxmax boldrows boldnamerows fgcolor
+    global HEAD nullid nullid2
 
     # listed is 0 for boundary, 1 for normal, 2 for negative, 3 for left, 4 for right
     set listed $cmitlisted($curview,$id)
@@ -4849,6 +4850,8 @@ proc drawcmittext {id row col} {
 	set ofill red
     } elseif {$id eq $nullid2} {
 	set ofill green
+    } elseif {$id eq $HEAD} {
+	set ofill yellow
     } else {
 	set ofill [expr {$listed != 0 ? $listed == 2 ? "gray" : "blue" : "white"}]
     }
@@ -9886,6 +9889,8 @@ set viewperm(0) 0
 set viewargs(0) {}
 set viewargscmd(0) {}
 
+set HEAD [exec git rev-parse HEAD]
+
 set numcommits 0
 set loginstance 0
 set cmdlineok 0

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Jeff King]

On Sat, May 24, 2008 at 09:14:21AM -0700, Linus Torvalds wrote:

> I actually like the red circle for "Local uncommitted changes". Maybe we 
> can use a similar visual clue for "currently checked out". You already 
> picked green for the "added to the index" case, so we have the three 
> primary RGB colors already used, but we could make it just be a deep 
> yellow.
>
> [...]
> 
> Here's a "make it yellow" patch.

I kind of expected this to color the _ref_, not the commit bubble. That
is, I think it is useful not just to see that "this commit is pointed to
by HEAD" but that "master is the currently checked out branch."

Of course in the case of detached HEAD, we would have only the commit to
color. But in that case a green box probably makes sense, since you
probably aren't going to have a bunch of other boxes pointing to it
(at most, a tag name or remote tracking branch name).

IOW,

  1. If head points to a ref, color the background of that ref box
     specially (yellow and green are taken. Red is probably a bit too
     danger-implying.  Blue?)
     ...
     Hmm, actually it looks like we already bold the refname in that
     case. I never noticed that until now. If I'm not the only one,
     then maybe that is an argument for making the differentiation more
     obvious.

  2. If head is detached, add a green 'HEAD' box.

-Peff

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Paul Mackerras]

Linus Torvalds writes:

> Of course, _usually_ it's just the top commit, and it's obvious that way 
> which one is the checked-out one, but if you do "gitk --all" or just 
> generally have multiple branches, right now it's hard to see what commit 
> is the checked-out one, regardless of whether it's detached or not.

If it's not detached, the branch name is in bold, but evidently that's
either not intuitive or not sufficiently distinct...

> I think "HEAD" in a green box would solve that too, but on the other hand, 
> we have a *lot* of boxes already. For people who mainly just track another 
> repo, you already have one box saying "master", and another one saying 
> "remotes/origin/master", and adding yet *another* box saying HEAD that 
> just points to the same commit will work, but do we really want that?
> 
> I actually like the red circle for "Local uncommitted changes". Maybe we 
> can use a similar visual clue for "currently checked out". You already 
> picked green for the "added to the index" case, so we have the three 
> primary RGB colors already used, but we could make it just be a deep 
> yellow.
> 
> Of course, maybe people hate lots of colos already, and something more 
> akin to the text background thing that we use for the selected commit 
> would be better.
> 
> I dunno. There's so many options.
> 
> Here's a "make it yellow" patch.

Thanks.  I have checked in something similar, that also handles the
cases where you update the graph and the head has moved, and when you
do a checkout or reset using the gitk menus.

Paul.

Re: [PATCH 4/3] bisect: use a detached HEAD to bisect

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Linus Torvalds writes:
>
>> Of course, _usually_ it's just the top commit, and it's obvious that way 
>> which one is the checked-out one, but if you do "gitk --all" or just 
>> generally have multiple branches, right now it's hard to see what commit 
>> is the checked-out one, regardless of whether it's detached or not.
>
> If it's not detached, the branch name is in bold, but evidently that's
> either not intuitive or not sufficiently distinct...
>
>> I think "HEAD" in a green box would solve that too, but on the other hand, 
>> we have a *lot* of boxes already. For people who mainly just track another 
>> repo, you already have one box saying "master", and another one saying 
>> "remotes/origin/master", and adding yet *another* box saying HEAD that 
>> just points to the same commit will work, but do we really want that?
>> 
>> I actually like the red circle for "Local uncommitted changes". Maybe we 
>> can use a similar visual clue for "currently checked out". You already 
>> picked green for the "added to the index" case, so we have the three 
>> primary RGB colors already used, but we could make it just be a deep 
>> yellow.
>> 
>> Of course, maybe people hate lots of colos already, and something more 
>> akin to the text background thing that we use for the selected commit 
>> would be better.
>> 
>> I dunno. There's so many options.
>> 
>> Here's a "make it yellow" patch.
>
> Thanks.  I have checked in something similar, that also handles the
> cases where you update the graph and the head has moved, and when you
> do a checkout or reset using the gitk menus.

Thanks.  Will pull, but it will have to miss upcoming 1.5.6-rc0,
unfortunately.