RFC: reverse bisect

RFC: reverse bisect

[Michal Vyskocil]

[-- Attachment #1: Type: text/plain, Size: 4761 bytes --]

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


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: RFC: reverse bisect

[Sverre Rabbelier]

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

Re: RFC: reverse bisect

[Johannes Sixt]

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

[Jeff King]

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

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

[Frans Klaver]

On Fri, 30 Sep 2011 06:09:24 +0200, Jeff King <peff@peff.net> 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

[Michal Vyskocil]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[RFC/PATCH]: reverse bisect v 2.0

[Michal Vyskocil]

[-- Attachment #1: Type: text/plain, Size: 5213 bytes --]

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


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

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

[Junio C Hamano]

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

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

[Jeff King]

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.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Mon, Oct 03, 2011 at 10:00:25AM -0700, Junio C Hamano wrote:

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

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

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

[Junio C Hamano]

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.

Thanks.

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

[Christian Couder]

Hi,

On Tuesday 04 October 2011 17:22:59 Junio C Hamano wrote:
> 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.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

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

[Andrew Ardill]

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

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

[Junio C Hamano]

Andrew Ardill <andrew.ardill@gmail.com> writes:

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

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

[Jeff King]

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.

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

> 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