threads / discuss / 28526

RFC: reverse bisect

Subject: RFC: reverse bisect

## tl;dr

17 messages between Sep 29, 2011 and Oct 12, 2011.

replies: 16people: 8as markdown or json

Michal Vyskocil· Sep 29, 2011, 14:20 UTC · lore
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
Sverre Rabbelier· Sep 29, 2011, 14:42 UTC · re: Michal Vyskocil · lore

Re: RFC: reverse bisect

Heya,
On Thu, Sep 29, 2011 at 16:20, Michal Vyskocil <mvyskocil@suse.cz> 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
Johannes Sixt· Sep 29, 2011, 16:27 UTC · re: Michal Vyskocil · lore

Re: RFC: reverse bisect

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
Jeff King· Sep 30, 2011, 04:09 UTC · re: Johannes Sixt · lore

Re: RFC: reverse bisect

On Thu, Sep 29, 2011 at 06:27:07PM +0200, Johannes Sixt wrote:
Show 7 quoted lines
> > 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
Frans Klaver· Sep 30, 2011, 05:31 UTC · re: Jeff King · lore

Re: RFC: reverse bisect

On Fri, 30 Sep 2011 06:09:24 +0200, Jeff King <peff@peff.net> wrote:
Show 7 quoted lines
> 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
Michal Vyskocil· Sep 30, 2011, 08:29 UTC · re: Johannes Sixt · lore

Re: RFC: reverse bisect

On Thu, Sep 29, 2011 at 06:27:07PM +0200, Johannes Sixt wrote:
Show 17 quoted lines
> 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

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
Michal Vyskocil· Sep 30, 2011, 11:42 UTC · re: Michal Vyskocil · lore

[RFC/PATCH]: reverse bisect v 2.0

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
Junio C Hamano· Sep 30, 2011, 18:13 UTC · re: Michal Vyskocil · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

[administrivia: added people involved in the main discussion thread back on CC line, and also added CCouder who seems to be fond of the command]

Michal Vyskocil <mvyskocil@suse.cz> writes:
> The bugfix command works like the previous git bisect start --reverse.
Does any released version of git have "bisect start --reverse"?

$ git bisect start --reverse unrecognised option: '--reverse'

Let's suppose that we had a "git frotz" Porcelain subcommand, that used to say "xyzzy" when all is well, but says "nitfol" these days. I released a (bad) script that parses the output from the subcommand, and want to say something like "This script only works with Git version after X", and to find out the value of X, I need to bisect the history to find when "frotz" started to say "nitfol".

I am not trying to find a commit that introduced a bug/regression to cause the recent "frotz" to say "nitfol". Neither am I trying to find a fix that corrected the earlier bogus output "xyzzy". No value judgement is involved in this scenario.

I wonder if something along the following line would make the usage more pleasant and self-explanatory:

    $ git bisect start --used-to='git frotz says xyzzy' v0.99 master
    Bisecting: 171 revisions left to test after this (roughly 8 steps)
    $ ... build and then test ...
    $ git bisect tested
    You are trying to check: git frotz says xyzzy
    Does the result satisify what you are trying to find [y/n]? yes

Saying 'yes' would be like saying 'good' and 'no' would be like saying 'bad' here.

When trying to find regression, you would say:
    $ git bisect start --used-to='it works' v0.99 master

and you say 'yes' if it works (equivalent to 'good'), and 'no' if it does not (equivalent to 'bad').

When trying to find a fix, you would say:
    $ git bisect start --used-to='checkout $tree $path clobbers $path' v0.99 master

and you say 'yes' if it is still broken at the tested version (equivalent to your reversed 'good'), and 'no' if the tested version contains a fix (equivalent to your reversed 'bad').

I am not married to the name of the option, but --used-to='...' felt more or less self-explanatory name to signal users what to describe in that text. The condition "You are trying to check:" cue wants to remind the user is "we used to do *this* but now we don't", and the command is trying to see when it changed.

Thoughts?
Jeff King· Oct 3, 2011, 10:41 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

On Fri, Sep 30, 2011 at 11:13:25AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> I wonder if something along the following line would make the usage more
> pleasant and self-explanatory:
> 
>     $ git bisect start --used-to='git frotz says xyzzy' v0.99 master
>     Bisecting: 171 revisions left to test after this (roughly 8 steps)
>     $ ... build and then test ...
>     $ git bisect tested
>     You are trying to check: git frotz says xyzzy
>     Does the result satisify what you are trying to find [y/n]? yes

I like this idea a lot. My "yes/no" thing was a "if I were designing bisect from scratch today..." suggestion, but having something like --used-to makes it a natural addition to the regular good/bad interface. And I really like the prompt to help people remember what it is they're declaring each time.

However, --used-to feels a bit backwards to me. I think of it as "--has-property" or something similar. That is, you are looking for when something appeared (be it a bug, a feature, or whatever). But I guess it depends on what you are bisecting. In my case, "yes" would be the current "bad", and "no" would be the current "good".

So maybe provide both --used-to and --has-property (which I really dislike as a name, but I can't think of anything better at the moment). And then we can interpret "yes" and "no" accordingly, depending which one the user used (and while that _sounds_ confusing, it won't be to the user; we'll be prompting them with "you're looking for ...", so their answer will be very natural).

But with both of them, the user is free to phrase it in whatever way feels natural.

> When trying to find regression, you would say:
> 
>     $ git bisect start --used-to='it works' v0.99 master
Just for completeness, under my proposal this could also be:
  $ git bisect start --has-property='git frotz is broken' v0.99 master
Which is probably slightly less natural. But:
> When trying to find a fix, you would say:
> 
>     $ git bisect start --used-to='checkout $tree $path clobbers $path' v0.99 master
This one would be a bit nicer:
  $ git bisect start --has-property='checkout works' v0.99 master
-Peff
PS Side note: do we really need it to be interactive? I would think
   you could do:
      $ git bisect start --used-to='git frotz says xyzzy'
      Bisecting: 171 revisions left to test after this (roughly 8 steps)
      Checking whether 'git frotz says xyzzy'.
      $ ... build and test ...
      $ git bisect yes
      Bisecting: 89 revisions left to test after this (roughly 7 steps)
      Checking whether 'git frotz says xyzzy'.
   It means the user is reminded, then tests, then responds. But I think
   it would be enough of a reminder. The important thing is that we keep
   mentioning it at each bisection step. Maybe it depends on how long
   your build/test step is (I tend to work on things like git, where
   that is a 30-second procedure, not a kernel with a long build and a
   reboot in between :) ).
Junio C Hamano· Oct 3, 2011, 17:00 UTC · re: Jeff King · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> On Fri, Sep 30, 2011 at 11:13:25AM -0700, Junio C Hamano wrote:
>
>> I wonder if something along the following line would make the usage more
>> pleasant and self-explanatory:
>> 
>>     $ git bisect start --used-to='git frotz says xyzzy' v0.99 master
>>     Bisecting: 171 revisions left to test after this (roughly 8 steps)
>>     $ ... build and then test ...
>>     $ git bisect tested
>>     You are trying to check: git frotz says xyzzy
>>     Does the result satisify what you are trying to find [y/n]? yes
>
> I like this idea a lot. My "yes/no" thing was a "if I were designing
> bisect from scratch today..." suggestion, but having something like
> --used-to makes it a natural addition to the regular good/bad interface.
> And I really like the prompt to help people remember what it is they're
> declaring each time.

I forgot to clarify that "tested" was only to help users who wanted reminder; if the user is confident with the usual "yes/no", the interactivity is not required.

> However, --used-to feels a bit backwards to me. I think of it as
> "--has-property" or something similar.

I do not think --used-to='frotz says xyzzy' is a good phrasing at all; it is grammatically incorrect. But --has-property has one large downside. At least --used-to makes it clear that the user is supposed to decribe the property of the tree in the past.

Let's step back a bit to understand why I think this is not optimal.

The low-level "bisect" machinery is about bisecting a history whose ancient commits have a property (e.g. "used to work" or "used to be broken") and more recent ones does not have the property to find one commit where the property is lost. Conceptually, this "property" does not have any value judgement associated with it, but historically, we called that property "good" (and lack of it "bad"), which caused confusion at the surface level when looking for a recent fix. The user has to say "ah, this is still broken and does not contain the fix---good" with a twisted mind.

To dissociate the value judgement from the "property", it is a good idea to allow users to operate the bisect machinery without using "good/bad", and "yes/no" is a good pair of words to use. However, the user still needs to be aware of the fact that the "property" must exist in older history and must be lacking in newer history.

"--has-property" strips the value judgement but does not surface this crucial point to the UI. When the user wants to find a recent fix, the property can be spelled in various ways: "it is broken", "it no longer is broken", "it is fixed", "it works", etc.. We need to map the 'yes' answer to 'bad' if --has-property=works is given and the user is looking for a fix, not a regression. But --has-property does not explicitly tell the user that the property must exist in the past and absent in the present.

My --used-to was an attempt to surface this "we are looking for a commit that changed the condition that used to hold, but no longer holds for recent commits" to the UI level and force "yes == observation consistent with the older tree" and "no == new tree" interpretation, without a need to map yes/no to good/bad or bad/good depending on what is hunted.

The alternative approach of "bisect start --reverse" was an attempt to allow swapping this mapping. When reverse is in effect, "good" maps to "observation consistent with the newer tree". I do not _mind_ mapping if it turns out to be absolutely necessary, but I wanted to see if we can do this without one.

> something appeared (be it a bug, a feature, or whatever). But I guess it
> depends on what you are bisecting. In my case, "yes" would be the
> current "bad", and "no" would be the current "good".

I think we are aiming for the same thing, but I think you are assuming that a naive user would always describe the condition that used to hold with --has-property without being told; I do not think that assumption holds.

Jeff King· Oct 4, 2011, 10:30 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

On Mon, Oct 03, 2011 at 10:00:25AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > I like this idea a lot. My "yes/no" thing was a "if I were designing
> > bisect from scratch today..." suggestion, but having something like
> > --used-to makes it a natural addition to the regular good/bad interface.
> > And I really like the prompt to help people remember what it is they're
> > declaring each time.
> 
> I forgot to clarify that "tested" was only to help users who wanted
> reminder; if the user is confident with the usual "yes/no", the
> interactivity is not required.

That makes sense to me. I think in either case, it would be nice to mention the --used-to text when we take each step. We're already outputting some status information there (like how many commits left).

Show 9 quoted lines
> > However, --used-to feels a bit backwards to me. I think of it as
> > "--has-property" or something similar.
> 
> I do not think --used-to='frotz says xyzzy' is a good phrasing at all; it
> is grammatically incorrect. But --has-property has one large downside.  At
> least --used-to makes it clear that the user is supposed to decribe the
> property of the tree in the past.
> 
> Let's step back a bit to understand why I think this is not optimal.

What you say makes sense, but isn't it just a problem of the name? IOW, a much better name than "--has-property" would be "--started-to". That would imply the exact same cutoff as --used-to, but negate only the yes/no bit.

So you could say:
  # find a bug:
  git bisect start --used-to='work with --foo=bar'
  # or if you are looking for a specific undesirable behavior, you might
  # write:
  git bisect start --used-to='not segfault with --foo=bar'
  # but now you have a negation in your condition. So it might be more
  # natural to write it as:
  git bisect start --started-to='segfault with --foo=bar'
  # Or we can find a feature
  git bisect start --used-to='not respect core.foo'
  # but again, we have a negation. Instead:
  git bisect start --started-to='respect core.foo'

And the --started-to would literally be implemented as flipping the meaning of "git bisect yes" and "git bisect no", and nothing more. IOW, it's just another way of spelling "git bisect --reverse".

I know you wanted to emphasize the "older tree has this property" of --used-to, but I think it is clear with --started-to that the older tree obviously obviously had the negation of the property.

-Peff
Junio C Hamano· Oct 4, 2011, 15:22 UTC · re: Jeff King · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> On Mon, Oct 03, 2011 at 10:00:25AM -0700, Junio C Hamano wrote:
>
> And the --started-to would literally be implemented as flipping the
> meaning of "git bisect yes" and "git bisect no", and nothing more. IOW,
> it's just another way of spelling "git bisect --reverse".

Yes, if we wanted to also implement the flipping of the mapping between yes/no and good/bad, I do not have any problem with --used-to/--started-to pair of options.

Thanks.
Christian Couder· Oct 4, 2011, 22:34 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Hi,
On Tuesday 04 October 2011 17:22:59 Junio C Hamano wrote:
Show 10 quoted lines
> Jeff King <peff@peff.net> writes:
> > On Mon, Oct 03, 2011 at 10:00:25AM -0700, Junio C Hamano wrote:
> > 
> > And the --started-to would literally be implemented as flipping the
> > meaning of "git bisect yes" and "git bisect no", and nothing more. IOW,
> > it's just another way of spelling "git bisect --reverse".
> 
> Yes, if we wanted to also implement the flipping of the mapping between
> yes/no and good/bad, I do not have any problem with --used-to/--started-to
> pair of options.
If we decide to go with yes/no, an option like:
--yes-means=<it behaves like this>

seems to me easier to understand. Though I recognize that it doesn't tell that the behavior changed.

And before responding to this thread I wanted to have another look at the unfinished patch that Dscho sent a few years ago, but I did not have much time these past few days, and I could not find it anymore.

Thanks, Christian.

Junio C Hamano· Oct 4, 2011, 23:27 UTC · re: Christian Couder · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Christian Couder <chriscool@tuxfamily.org> writes:
Show 6 quoted lines
> If we decide to go with yes/no, an option like:
>
> --yes-means=<it behaves like this>
>
> seems to me easier to understand. Though I recognize that it doesn't tell that 
> the behavior changed.
What problem are you trying to solve?

As I already said while discussing --has-property vs --used-to, I think that --yes-means shares the exact same downside as --has-property.

The --used-to proposal was to make sure that people who are bisecting to find fixes would not by mistake say

	git bisect start --used-to='work fine'

as it is very clear that bisecting in a history with something that used to work fine is _not_ hunting for a fix.

When the users say
	git bisect start --yes-means='frotz is broken'

is it clear to them that they are supposed to define what used to be the case (in which case "yes" is always mapped to "good")? If you are trying to allow them to say either old or new behaviour with --yes-means, how does your "bisect" know if it needs to map "yes" to "good" (regression hunting), or "yes" to "bad" (looking for a fix)?

Andrew Ardill· Oct 7, 2011, 01:57 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Hi, I have given this some thought and have a slightly different proposal for the options to use. I have written a commit message that I think explains the need for the improvement, and justifies my proposal. The options I propose are '--intoduced' and '--removed'.

--->8---

Bisect is used to look for a commit that causes a specific change. Such a change can be classified by the user as either introducing something, or removing something, and this distinction is arbitrary. A change could be said to introduce a bug fix, or remove a bug - both formulations have inherently the same meaning, but search behaviour changes depending on which one we use. Suppose I want to find the commit that removed a bug. If I examine a snapshot and it contains the bug, the bug has not yet been *removed* and I must look in the future for its removal. Conversely, if I examine a snapshot and the bug fix exists, the bug fix must have been *introduced* at some earlier point and so I must search backwards.

Confusion exists at this point because at each verification step a question like 'Does this snapshot have X?' is asked, when what we eventually want to know is 'When was X introduced?' or 'When was X removed?'. Previously there has been no way to specify if we are looking for X being introduced or removed; it was assumed that we were looking for when something was introduced, for example when a bug was introduced.

Teach git bisect if we are looking for when something was introduced or when something was removed.

Examples.
Search for a feature add:
   $ git bisect start --introduced='feature: git frotz says xyzzy' v0.99 master
   Bisecting: 171 revisions left to test after this (roughly 8 steps)
   $ ... build and then test ...
   $ git bisect tested
   Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
# already added, look backwards
Search for a feature regression:
   $ git bisect start --removed='feature: git frotz says xyzzy' v0.99 master
   Bisecting: 171 revisions left to test after this (roughly 8 steps)
   $ ... build and then test ...
   $ git bisect tested
   Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
# not removed yet, look forwards
--->8---
Regards,
Andrew Ardill
Junio C Hamano· Oct 12, 2011, 04:57 UTC · re: Andrew Ardill · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

Andrew Ardill <andrew.ardill@gmail.com> writes:
Show 16 quoted lines
> Examples.
> Search for a feature add:
>    $ git bisect start --introduced='feature: git frotz says xyzzy' v0.99 master
>    Bisecting: 171 revisions left to test after this (roughly 8 steps)
>    $ ... build and then test ...
>    $ git bisect tested
>    Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
> # already added, look backwards
>
> Search for a feature regression:
>    $ git bisect start --removed='feature: git frotz says xyzzy' v0.99 master
>    Bisecting: 171 revisions left to test after this (roughly 8 steps)
>    $ ... build and then test ...
>    $ git bisect tested
>    Does this snapshot have 'feature: git frotz says xyzzy' [y/n]? yes
> # not removed yet, look forwards

With an obvious addition of non-interactive short-cut subcommands "git bisect yes" and "git bisect no", I think --removed= is a much better wording than --used-to= I suggested in the discussion.

I however am still worried about the flipping of the mapping between <good,bad> and <yes,no> this design requires. What are we going to do to the labels of low-level machinery (i.e $GIT_DIR/refs/bisect/bad and $GIT/refs/bisect/good)? They appear in "bisect visualize" and I was hoping that it would be simpler in the code if we do not have to change them in such a way that depends on this introduced/removed switch, and that was the reason why I was trying to see if we can solve this without the switchable mapping between <good,bad> and <yes,no>.

More specifically, I was hoping that we can rename "good" to "old" and "bad" to "new" unconditionally and be done with it. We would ask the user "What did the code used to do in the olden days?" and "Does this version behave the same as it used to?". The possible answers the user can give are "git bisect old" (it behaves the same as the older versions) and "git bisect new" (it behaves the same as the newer versions). Then we do not have to worry about having to flip the meaning of <yes> and <no> at the UI level.

Jeff King· Oct 12, 2011, 20:14 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH]: reverse bisect v 2.0

On Tue, Oct 11, 2011 at 09:57:13PM -0700, Junio C Hamano wrote:
> With an obvious addition of non-interactive short-cut subcommands "git
> bisect yes" and "git bisect no", I think --removed= is a much better
> wording than --used-to= I suggested in the discussion.
Agreed.
Show 8 quoted lines
> I however am still worried about the flipping of the mapping between
> <good,bad> and <yes,no> this design requires. What are we going to do to
> the labels of low-level machinery (i.e $GIT_DIR/refs/bisect/bad and
> $GIT/refs/bisect/good)? They appear in "bisect visualize" and I was hoping
> that it would be simpler in the code if we do not have to change them in
> such a way that depends on this introduced/removed switch, and that was
> the reason why I was trying to see if we can solve this without the
> switchable mapping between <good,bad> and <yes,no>.

Hmm. I hadn't thought about the labels. In a yes/no situation, though, couldn't you use the labels as the user sees them?

Then it is simply a matter of flipping yes/no inside the bisect script whenever we interact with the user (i.e., "git bisect yes") or when we interact with the on-disk labels.

Certainly it's more complex than not allowing reversing, though.
Show 8 quoted lines
> More specifically, I was hoping that we can rename "good" to "old" and
> "bad" to "new" unconditionally and be done with it. We would ask the user
> "What did the code used to do in the olden days?" and "Does this version
> behave the same as it used to?". The possible answers the user can give
> are "git bisect old" (it behaves the same as the older versions) and "git
> bisect new" (it behaves the same as the newer versions). Then we do not
> have to worry about having to flip the meaning of <yes> and <no> at the UI
> level.

Hmm. I think this is not quite as nice, but it is way simpler. It may be worth trying for a bit to see how people like it. If they don't, the cost of failure is that we have to maintain "old/new" forever, even after we implement a yes/no reversible scheme. But maintaining the old/new mapping from yes/no would not be any harder than the good/bad mapping, which we would need to do anyway.

So it sounds like a reasonable first step.
-Peff

← back to recent threads