RFC: reverse bisect

Subsystems: the rest

7 messages, 5 authors, 2016-06-15 · open the first message on its own page

RFC: reverse bisect

From: Michal Vyskocil <hidden>
Date: 2016-06-15 22:52:08

Hi all,

Following proposed patch try to implement reverse mode for git bisect.

The git bisect command is written in the regression-finding in mind. IOW
it expects the good commit is older than the later one, which caused a
regression.

However common usage for (at least) package maintainer is not to find a
regression and fix it. The main task it to identify a bugfix!. In this
case git bisect is still helpfull as it reduces a time a lot, but user
needs to exchange the good<->bad in his mind, which is confusing and in
case there are delays in the work, it's trivial to forgot that I have to
type git bisect good, when I'm in the bad revision.

This simple patch try to address the problem of poor package
maintainer's brain and introduces --reverse argument for the git bisect
start command.

In this mode, bisect internally exchange the behavior of good/bad
itself, so there's no need to do it manually. I did some basic testing
and

git bisect start --reverse HEAD~999 HEAD
git bisect good/bad/skip/run

really works well, allowing user to identify a first good commit instead
of the first bad one. I did not test other commands like visualize or
replay.

What do you think about it? Do you see other problems I'm not aware of?
---
 bisect.c      |    2 +-
 git-bisect.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/bisect.c b/bisect.c
index c7b7d79..33aaeaa 100644
--- a/bisect.c
+++ b/bisect.c

@@ -768,7 +768,7 @@ static void handle_bad_merge_base(void)
 
 	fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
 		"git bisect cannot work properly in this case.\n"
-		"Maybe you mistake good and bad revs?\n");
+		"Try --reverse to switch the bisect logic.\n");
 	exit(1);
 }

diff --git a/git-bisect.sh b/git-bisect.sh
index 2524060..5c95f25 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -33,6 +33,25 @@ OPTIONS_SPEC=
 _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 
+bisect_reverse_mode() {
+	test -f "$GIT_DIR/BISECT_REVERSE"
+}
+
+bisect_reverse_state() {
+
+	if bisect_reverse_mode; then
+		if test "$1" = "good"; then
+			echo "bad"
+			return 0
+		elif test "$1" = "bad"; then
+			echo "good"
+			return 0
+		fi
+	fi
+
+	echo $1
+}
+
 bisect_head()
 {
 	if test -f "$GIT_DIR/BISECT_HEAD"

@@ -69,6 +88,11 @@ bisect_start() {
 	# Check for one bad and then some good revisions.
 	#
 	has_double_dash=0
+	#
+	# Exchange the internal meainng of good/bad allowing bisect to find
+	# a commit fixing a bug, not "only" the one causes a regression
+	#
+	reverse_mode=1
 	for arg; do
 		case "$arg" in --) has_double_dash=1; break ;; esac
 	done

@@ -91,6 +115,9 @@ bisect_start() {
 		--no-checkout)
 			mode=--no-checkout
 			shift ;;
+		--reverse)
+			reverse_mode=0
+			shift ;;
 		--*)
 			die "$(eval_gettext "unrecognised option: '\$arg'")" ;;
 		*)

@@ -99,10 +126,17 @@ bisect_start() {
 				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
 				break
 			}
-			case $bad_seen in
-			0) state='bad' ; bad_seen=1 ;;
-			*) state='good' ;;
-			esac
+			if test $reverse_mode -ne 0; then
+				case $bad_seen in
+				0) state='bad' ; bad_seen=1 ;;
+				*) state='good' ;;
+				esac
+			else
+				case $bad_seen in
+				0) state='good' ; bad_seen=1 ;;
+				*) state='bad' ;;
+				esac
+			fi
 			eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
 			shift
 			;;

@@ -170,6 +204,9 @@ bisect_start() {
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
+	if test $reverse_mode -eq 0; then
+		/bin/touch "$GIT_DIR/BISECT_REVERSE" || exit
+	fi
 	#
 	# Check if we can proceed to the next bisect state.
 	#

@@ -225,7 +262,7 @@ bisect_skip() {
 
 bisect_state() {
 	bisect_autostart
-	state=$1
+	state=$(bisect_reverse_state $1)
 	case "$#,$state" in
 	0,*)
 		die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;

@@ -377,6 +414,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_LOG" &&
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
+	rm -f "$GIT_DIR/BISECT_REVERSE" &&
 	# Cleanup head-name if it got left by an old version of git-bisect
 	rm -f "$GIT_DIR/head-name" &&
 	git update-ref -d --no-deref BISECT_HEAD &&

@@ -402,6 +440,7 @@ bisect_replay () {
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
 		good|bad|skip)
+			command=$(bisect_reverse_state $1)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;

-- 
1.7.6.3

Re: RFC: reverse bisect

From: Sverre Rabbelier <hidden>
Date: 2016-06-15 22:52:08

Heya,

On Thu, Sep 29, 2011 at 16:20, Michal Vyskocil [off-list ref] wrote:

       fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
               "git bisect cannot work properly in this case.\n"
-               "Maybe you mistake good and bad revs?\n");
+               "Try --reverse to switch the bisect logic.\n");

Heh, glad to see the "Maybe you mistake" phrasing removed if nothing else :P.

-- 
Cheers,

Sverre Rabbelier

Re: RFC: reverse bisect

From: Johannes Sixt <hidden>
Date: 2016-06-15 22:52:08

Am 29.09.2011 16:20, schrieb Michal Vyskocil:

git bisect start --reverse HEAD~999 HEAD

With the regular meaning of the start subcommand, the revs given are
ordered: bad good good...

With the reversed meaning, this would have to become: good bad bad...

This would have to be mentioned clearly in the documentation.

git bisect good/bad/skip/run

Last time this came up on the list I suggested to add the following
commands:

   git bisect regression  # a synonym for git bisect start
   git bisect improvement # your --reverse

-- Hannes

Re: RFC: reverse bisect

From: Jeff King <hidden>
Date: 2016-06-15 22:52:08

On Thu, Sep 29, 2011 at 06:27:07PM +0200, Johannes Sixt wrote:

quoted

git bisect good/bad/skip/run

Last time this came up on the list I suggested to add the following
commands:

   git bisect regression  # a synonym for git bisect start
   git bisect improvement # your --reverse

That makes some sense to me. But I do wonder if you could simply get rid
of the connotations of "good" and "bad" entirely, by thinking of it as
simply looking for a commit that introduced some property. Like:

  # find a bug
  git bisect start
  git bisect yes ;# has the bug
  git bisect no ;# does not have the bug
  git bisect skip ;# no idea

  # find a feature being implemented
  git bisect start
  git bisect yes ;# has the feature
  git bisect no ;# does not have the feature
  git bisect skip ;# no idea

IOW, I feel like we are having to handle this weird negation only
because we have assigned a value judgement to the tests. That instead of
saying "yes, we have this bug", we say "bad", which only makes sense if
you are looking for a bad thing.

You can still produce a negation in your mind, of course, by asking
"when did this property go away".  But that is usually about a bug being
fixed, so the right answer is generally not a set of command line
options, but to stop asking "when did bug X go away", and instead ask
"when did the fix for bug X appear".

One catch is that the run command assumes a successful exit is "good",
and anything else is "bad". Which makes:

  git bisect run make test

good for finding regressions, but is a little counterintuitive for the
yes/no thing (a successful exit means "no").

-Peff

Re: RFC: reverse bisect

From: Frans Klaver <hidden>
Date: 2016-06-15 22:52:08

On Fri, 30 Sep 2011 06:09:24 +0200, Jeff King [off-list ref] wrote:

One catch is that the run command assumes a successful exit is "good",
and anything else is "bad". Which makes:

  git bisect run make test

good for finding regressions, but is a little counterintuitive for the
yes/no thing (a successful exit means "no").

Then you would require a script that inverts the result, no? From my
point of view it's either that or add an option telling bisect run how
to interpret the results. In the latter case you could still consider
adding the regression/improvement qualification to bisect start. It might
help getting the mind set right.

Frans

Re: RFC: reverse bisect

From: Michal Vyskocil <hidden>
Date: 2016-06-15 22:52:08

On Thu, Sep 29, 2011 at 06:27:07PM +0200, Johannes Sixt wrote:

Am 29.09.2011 16:20, schrieb Michal Vyskocil:

quoted

git bisect start --reverse HEAD~999 HEAD

With the regular meaning of the start subcommand, the revs given are
ordered: bad good good...

With the reversed meaning, this would have to become: good bad bad...

This would have to be mentioned clearly in the documentation.

quoted

git bisect good/bad/skip/run

Last time this came up on the list I suggested to add the following
commands:

   git bisect regression  # a synonym for git bisect start
   git bisect improvement # your --reverse

Good point! As you mentioned, the --switch already reverse the meanings
of arguments as you mentioned. Using a new command will be less
confusing for users.

Michal Vyskocil

[RFC/PATCH]: reverse bisect v 2.0

From: Michal Vyskocil <hidden>
Date: 2016-06-15 22:52:08

The bugfix command works like the previous git bisect start --reverse.
It switch the meaning of good(s) and bad from the default regression
search approach to the bugfix one.

I don't like adding more new subcommands into bisect, so I decided to
not add ideas I have found on this mailinglist, like 'git bisect
regression' or 'yes', 'no', 'fixed', 'unfixed' or whatever.

The git bisect start/bugfix good/bad/skip log replay and vizualize were
tested (however on simple linear example).

The missing points:
 * git-bisect--helper has the "bad" hardcoded, so the commit fixing a
   bug is reffered as a bad one
 * in git bisect vizualize, the good commit is shown under refs/bad in
   gitk (however that's the same problem if user reverse the usage of
   good/bad itself).
 * documentation and tests of course

Regards
Michal Vyskocil
---
 bisect.c      |    2 +-
 git-bisect.sh |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/bisect.c b/bisect.c
index c7b7d79..2eb34db 100644
--- a/bisect.c
+++ b/bisect.c

@@ -768,7 +768,7 @@ static void handle_bad_merge_base(void)
 
 	fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
 		"git bisect cannot work properly in this case.\n"
-		"Maybe you mistake good and bad revs?\n");
+		"Try git bisect bugfix to switch the default bisect logic.\n");
 	exit(1);
 }

diff --git a/git-bisect.sh b/git-bisect.sh
index 2524060..6959cf8 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -4,7 +4,9 @@ USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
 LONG_USAGE='git bisect help
 	print this long help message.
 git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<pathspec>...]
-	reset bisect state and start bisection.
+	reset bisect state and start bisection to find a regression.
+git bisect bugfix [--no-checkout] [<good> [<bad>...]] [--] [<pathspec>...]
+	reset bisect state and start bisection to find a bugfix.
 git bisect bad [<rev>]
 	mark <rev> a known-bad revision.
 git bisect good [<rev>...]

@@ -33,6 +35,29 @@ OPTIONS_SPEC=
 _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
 _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 
+setup_bugfix_mode() {
+	/bin/touch "$GIT_DIR/BISECT_BUGFIX"
+}
+
+bisect_bugfix_mode() {
+	test -f "$GIT_DIR/BISECT_BUGFIX"
+}
+
+bisect_check_state() {
+
+	if bisect_bugfix_mode; then
+		if test "$1" = "good"; then
+			echo "bad"
+			return 0
+		elif test "$1" = "bad"; then
+			echo "good"
+			return 0
+		fi
+	fi
+
+	echo $1
+}
+
 bisect_head()
 {
 	if test -f "$GIT_DIR/BISECT_HEAD"

@@ -69,6 +94,15 @@ bisect_start() {
 	# Check for one bad and then some good revisions.
 	#
 	has_double_dash=0
+	#
+	# Exchange the internal meaning of good/bad allowing bisect to find
+	# a commit fixing a bug, not "only" the one causes a regression
+	#
+	cmd="start"
+	if test -n "$1" && test "$1" = "bugfix"; then
+		cmd="bugfix"
+		shift 1
+	fi
 	for arg; do
 		case "$arg" in --) has_double_dash=1; break ;; esac
 	done

@@ -99,10 +133,17 @@ bisect_start() {
 				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
 				break
 			}
-			case $bad_seen in
-			0) state='bad' ; bad_seen=1 ;;
-			*) state='good' ;;
-			esac
+			#if test $cmd = "bisect"; then
+				case $bad_seen in
+				0) state='bad' ; bad_seen=1 ;;
+				*) state='good' ;;
+				esac
+			#else
+			#	case $bad_seen in
+			#	0) state='good' ; bad_seen=1 ;;
+			#	*) state='bad' ;;
+			#	esac
+			#fi
 			eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
 			shift
 			;;

@@ -169,7 +210,10 @@ bisect_start() {
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
-	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
+	echo "git bisect $cmd$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
+	if test $cmd = "bugfix"; then
+		setup_bugfix_mode || exit
+	fi
 	#
 	# Check if we can proceed to the next bisect state.
 	#

@@ -225,7 +269,7 @@ bisect_skip() {
 
 bisect_state() {
 	bisect_autostart
-	state=$1
+	state=$(bisect_check_state $1)
 	case "$#,$state" in
 	0,*)
 		die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;

@@ -377,6 +421,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_LOG" &&
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
+	rm -f "$GIT_DIR/BISECT_BUGFIX" &&
 	# Cleanup head-name if it got left by an old version of git-bisect
 	rm -f "$GIT_DIR/head-name" &&
 	git update-ref -d --no-deref BISECT_HEAD &&

@@ -401,7 +446,12 @@ bisect_replay () {
 		start)
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
+		bugfix)
+			cmd="bisect_start 'bugfix' $rev"
+			setup_bugfix_mode || exit
+			eval "$cmd" ;;
 		good|bad|skip)
+			command=$(bisect_check_state $1)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;

@@ -485,6 +535,8 @@ case "$#" in
 		git bisect -h ;;
 	start)
 		bisect_start "$@" ;;
+	bugfix)
+		bisect_start "bugfix" "$@" ;;
 	bad|good)
 		bisect_state "$cmd" "$@" ;;
 	skip)

-- 
1.7.6.3

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help