[PATCH v10 0/7] bisect terms

[PATCH v10 0/7] bisect terms

[Matthieu Moy]

This version include Michael Haggerty's improvement to the doc. The
general ones are at the beginning of the series, and the
terms-specific ones are squashed into the appropriate commits.

I added more tests to "git bisect terms", and there are a few code
improvement.

Antoine Delaite (5):
  bisect: correction of typo
  bisect: replace hardcoded "bad|good" by variables
  bisect: simplify the addition of new bisect terms
  bisect: add the terms old/new
  bisect: allow any terms set by user

Matthieu Moy (1):
  Documentation/bisect: move getting help section to the end

Michael Haggerty (1):
  Documentation/bisect: revise overall content

 Documentation/git-bisect.txt | 225 +++++++++++++++++++++++++++++++------------
 bisect.c                     |  94 ++++++++++++++----
 git-bisect.sh                | 210 ++++++++++++++++++++++++++++++++--------
 revision.c                   |  20 +++-
 t/t6030-bisect-porcelain.sh  | 119 ++++++++++++++++++++++-
 5 files changed, 544 insertions(+), 124 deletions(-)

-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 1/7] bisect: correction of typo

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 bisect.c                    | 2 +-
 t/t6030-bisect-porcelain.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/bisect.c b/bisect.c
index 03d5cd9..5b8357d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -743,7 +743,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");
+		"Maybe you mistook good and bad revs?\n");
 	exit(1);
 }
 
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 06b4868..9e2c203 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -362,7 +362,7 @@ test_expect_success 'bisect starting with a detached HEAD' '
 test_expect_success 'bisect errors out if bad and good are mistaken' '
 	git bisect reset &&
 	test_must_fail git bisect start $HASH2 $HASH4 2> rev_list_error &&
-	grep "mistake good and bad" rev_list_error &&
+	grep "mistook good and bad" rev_list_error &&
 	git bisect reset
 '
 
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 2/7] Documentation/bisect: move getting help section to the end

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-bisect.txt | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 4cb52a7..2bdc3b8 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -31,12 +31,6 @@ This command uses 'git rev-list --bisect' to help drive the
 binary search process to find which change introduced a bug, given an
 old "good" commit object name and a later "bad" commit object name.
 
-Getting help
-~~~~~~~~~~~~
-
-Use "git bisect" to get a short usage description, and "git bisect
-help" or "git bisect -h" to get a long usage description.
-
 Basic bisect commands: start, bad, good
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -379,6 +373,11 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
 has at least one parent whose reachable graph is fully traversable in the sense
 required by 'git pack objects'.
 
+Getting help
+~~~~~~~~~~~~
+
+Use `git bisect` to get a short usage description, and `git bisect
+help` or `git bisect -h` to get a long usage description.
 
 SEE ALSO
 --------
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 3/7] Documentation/bisect: revise overall content

[Matthieu Moy]

From: Michael Haggerty <mhagger@alum.mit.edu>

Thoroughly revise the "git bisect" manpage, including:

* Beef up the "Description" section.

* Make the first long example less specific to kernel development.

* De-emphasize implementation details in a couple of places.

* Add "(roughly N steps)" in the places where example output is shown.

* Properly markup code within the prose.

* Lots of wordsmithing.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-bisect.txt | 122 ++++++++++++++++++++++++-------------------
 1 file changed, 68 insertions(+), 54 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 2bdc3b8..8e8a073 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -3,7 +3,7 @@ git-bisect(1)
 
 NAME
 ----
-git-bisect - Find by binary search the change that introduced a bug
+git-bisect - Use binary search to find the commit that introduced a bug
 
 
 SYNOPSIS
@@ -16,7 +16,6 @@ DESCRIPTION
 The command takes various subcommands, and different options depending
 on the subcommand:
 
- git bisect help
  git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
  git bisect bad [<rev>]
  git bisect good [<rev>...]
@@ -26,58 +25,71 @@ on the subcommand:
  git bisect replay <logfile>
  git bisect log
  git bisect run <cmd>...
+ git bisect help
 
-This command uses 'git rev-list --bisect' to help drive the
-binary search process to find which change introduced a bug, given an
-old "good" commit object name and a later "bad" commit object name.
+This command uses a binary search algorithm to find which commit in
+your project's history introduced a bug. You use it by first telling
+it a "bad" commit that is known to contain the bug, and a "good"
+commit that is known to be before the bug was introduced. Then `git
+bisect` picks a commit between those two endpoints and asks you
+whether the selected commit is "good" or "bad". It continues narrowing
+down the range until it finds the exact commit that introduced the
+change.
 
 Basic bisect commands: start, bad, good
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-Using the Linux kernel tree as an example, basic use of the bisect
-command is as follows:
+As an example, suppose you are trying to find the commit that broke a
+feature that was known to work in version `v2.6.13-rc2` of your
+project. You start a bisect session as follows:
 
 ------------------------------------------------
 $ git bisect start
 $ git bisect bad                 # Current version is bad
-$ git bisect good v2.6.13-rc2    # v2.6.13-rc2 was the last version
-                                 # tested that was good
+$ git bisect good v2.6.13-rc2    # v2.6.13-rc2 is known to be good
+------------------------------------------------
+
+Once you have specified at least one bad and one good commit, `git
+bisect` selects a commit in the middle of that range of history,
+checks it out, and outputs something similar to the following:
+
+------------------------------------------------
+Bisecting: 675 revisions left to test after this (roughly 10 steps)
 ------------------------------------------------
 
-When you have specified at least one bad and one good version, the
-command bisects the revision tree and outputs something similar to
-the following:
+You should now compile the checked-out version and test it. If that
+version works correctly, type
 
 ------------------------------------------------
-Bisecting: 675 revisions left to test after this
+$ git bisect good
 ------------------------------------------------
 
-The state in the middle of the set of revisions is then checked out.
-You would now compile that kernel and boot it. If the booted kernel
-works correctly, you would then issue the following command:
+If that version is broken, type
 
 ------------------------------------------------
-$ git bisect good			# this one is good
+$ git bisect bad
 ------------------------------------------------
 
-The output of this command would be something similar to the following:
+Then `git bisect` will respond with something like
 
 ------------------------------------------------
-Bisecting: 337 revisions left to test after this
+Bisecting: 337 revisions left to test after this (roughly 9 steps)
 ------------------------------------------------
 
-You keep repeating this process, compiling the tree, testing it, and
-depending on whether it is good or bad issuing the command "git bisect good"
-or "git bisect bad" to ask for the next bisection.
+Keep repeating the process: compile the tree, test it, and depending
+on whether it is good or bad run `git bisect good` or `git bisect bad`
+to ask for the next commit that needs testing.
+
+Eventually there will be no more revisions left to bisect, and the
+command will print out a description of the first bad commit. The
+reference `refs/bisect/bad` will be left pointing at that commit.
 
-Eventually there will be no more revisions left to bisect, and you
-will have been left with the first bad kernel revision in "refs/bisect/bad".
 
 Bisect reset
 ~~~~~~~~~~~~
 
 After a bisect session, to clean up the bisection state and return to
-the original HEAD (i.e., to quit bisecting), issue the following command:
+the original HEAD, issue the following command:
 
 ------------------------------------------------
 $ git bisect reset
@@ -94,9 +106,10 @@ instead:
 $ git bisect reset <commit>
 ------------------------------------------------
 
-For example, `git bisect reset HEAD` will leave you on the current
-bisection commit and avoid switching commits at all, while `git bisect
-reset bisect/bad` will check out the first bad revision.
+For example, `git bisect reset bisect/bad` will check out the first
+bad revision, while `git bisect reset HEAD` will leave you on the
+current bisection commit and avoid switching commits at all.
+
 
 Bisect visualize
 ~~~~~~~~~~~~~~~~
@@ -141,17 +154,17 @@ $ git bisect replay that-file
 Avoiding testing a commit
 ~~~~~~~~~~~~~~~~~~~~~~~~~
 
-If, in the middle of a bisect session, you know that the next suggested
-revision is not a good one to test (e.g. the change the commit
-introduces is known not to work in your environment and you know it
-does not have anything to do with the bug you are chasing), you may
-want to find a nearby commit and try that instead.
+If, in the middle of a bisect session, you know that the suggested
+revision is not a good one to test (e.g. it fails to build and you
+know that the failure does not have anything to do with the bug you
+are chasing), you can manually select a nearby commit and test that
+one instead.
 
 For example:
 
 ------------
 $ git bisect good/bad			# previous round was good or bad.
-Bisecting: 337 revisions left to test after this
+Bisecting: 337 revisions left to test after this (roughly 9 steps)
 $ git bisect visualize			# oops, that is uninteresting.
 $ git reset --hard HEAD~3		# try 3 revisions before what
 					# was suggested
@@ -163,18 +176,19 @@ the revision as good or bad in the usual manner.
 Bisect skip
 ~~~~~~~~~~~~
 
-Instead of choosing by yourself a nearby commit, you can ask Git
-to do it for you by issuing the command:
+Instead of choosing a nearby commit by yourself, you can ask Git to do
+it for you by issuing the command:
 
 ------------
 $ git bisect skip                 # Current version cannot be tested
 ------------
 
-But Git may eventually be unable to tell the first bad commit among
-a bad commit and one or more skipped commits.
+However, if you skip a commit adjacent to the one you are looking for,
+Git will be unable to tell exactly which of those commits was the
+first bad one.
 
-You can even skip a range of commits, instead of just one commit,
-using the "'<commit1>'..'<commit2>'" notation. For example:
+You can also skip a range of commits, instead of just one commit,
+using range notation. For example:
 
 ------------
 $ git bisect skip v2.5..v2.6
@@ -190,8 +204,8 @@ would issue the command:
 $ git bisect skip v2.5 v2.5..v2.6
 ------------
 
-This tells the bisect process that the commits between `v2.5` included
-and `v2.6` included should be skipped.
+This tells the bisect process that the commits between `v2.5` and
+`v2.6` (inclusive) should be skipped.
 
 
 Cutting down bisection by giving more parameters to bisect start
@@ -225,14 +239,14 @@ or bad, you can bisect by issuing the command:
 $ git bisect run my_script arguments
 ------------
 
-Note that the script (`my_script` in the above example) should
-exit with code 0 if the current source code is good, and exit with a
-code between 1 and 127 (inclusive), except 125, if the current
-source code is bad.
+Note that the script (`my_script` in the above example) should exit
+with code 0 if the current source code is good/old, and exit with a
+code between 1 and 127 (inclusive), except 125, if the current source
+code is bad/new.
 
 Any other exit code will abort the bisect process. It should be noted
-that a program that terminates via "exit(-1)" leaves $? = 255, (see the
-exit(3) manual page), as the value is chopped with "& 0377".
+that a program that terminates via `exit(-1)` leaves $? = 255, (see the
+exit(3) manual page), as the value is chopped with `& 0377`.
 
 The special exit code 125 should be used when the current source code
 cannot be tested. If the script exits with this code, the current
@@ -241,7 +255,7 @@ as the highest sensible value to use for this purpose, because 126 and 127
 are used by POSIX shells to signal specific error status (127 is for
 command not found, 126 is for command found but not executable---these
 details do not matter, as they are normal errors in the script, as far as
-"bisect run" is concerned).
+`bisect run` is concerned).
 
 You may often find that during a bisect session you want to have
 temporary modifications (e.g. s/#define DEBUG 0/#define DEBUG 1/ in a
@@ -254,7 +268,7 @@ next revision to test, the script can apply the patch
 before compiling, run the real test, and afterwards decide if the
 revision (possibly with the needed patch) passed the test and then
 rewind the tree to the pristine state.  Finally the script should exit
-with the status of the real test to let the "git bisect run" command loop
+with the status of the real test to let the `git bisect run` command loop
 determine the eventual outcome of the bisect session.
 
 OPTIONS
@@ -301,12 +315,12 @@ $ git bisect run ~/test.sh
 $ git bisect reset                   # quit the bisect session
 ------------
 +
-Here we use a "test.sh" custom script. In this script, if "make"
+Here we use a `test.sh` custom script. In this script, if `make`
 fails, we skip the current commit.
-"check_test_case.sh" should "exit 0" if the test case passes,
-and "exit 1" otherwise.
+`check_test_case.sh` should `exit 0` if the test case passes,
+and `exit 1` otherwise.
 +
-It is safer if both "test.sh" and "check_test_case.sh" are
+It is safer if both `test.sh` and `check_test_case.sh` are
 outside the repository to prevent interactions between the bisect,
 make and test processes and the scripts.
 
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 4/7] bisect: replace hardcoded "bad|good" by variables

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

To add new tags like old/new and have keywords less confusing, the
first step is to avoid hardcoding the keywords.

The default mode is still bad/good.

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 bisect.c      | 54 +++++++++++++++++++++++++++++++++++++-----------------
 git-bisect.sh | 57 +++++++++++++++++++++++++++++++--------------------------
 2 files changed, 68 insertions(+), 43 deletions(-)

diff --git a/bisect.c b/bisect.c
index 5b8357d..9368f6c 100644
--- a/bisect.c
+++ b/bisect.c
@@ -21,6 +21,9 @@ static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
 static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
 
+static const char *name_bad;
+static const char *name_good;
+
 /* Remember to update object flag allocation in object.h */
 #define COUNTED		(1u<<16)
 
@@ -403,15 +406,21 @@ struct commit_list *find_bisection(struct commit_list *list,
 static int register_ref(const char *refname, const struct object_id *oid,
 			int flags, void *cb_data)
 {
-	if (!strcmp(refname, "bad")) {
+	struct strbuf good_prefix = STRBUF_INIT;
+	strbuf_addstr(&good_prefix, name_good);
+	strbuf_addstr(&good_prefix, "-");
+
+	if (!strcmp(refname, name_bad)) {
 		current_bad_oid = xmalloc(sizeof(*current_bad_oid));
 		oidcpy(current_bad_oid, oid);
-	} else if (starts_with(refname, "good-")) {
+	} else if (starts_with(refname, good_prefix.buf)) {
 		sha1_array_append(&good_revs, oid->hash);
 	} else if (starts_with(refname, "skip-")) {
 		sha1_array_append(&skipped_revs, oid->hash);
 	}
 
+	strbuf_release(&good_prefix);
+
 	return 0;
 }
 
@@ -634,7 +643,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 		return;
 
 	printf("There are only 'skip'ped commits left to test.\n"
-	       "The first bad commit could be any of:\n");
+	       "The first %s commit could be any of:\n", name_bad);
 	print_commit_list(tried, "%s\n", "%s\n");
 	if (bad)
 		printf("%s\n", oid_to_hex(bad));
@@ -732,18 +741,24 @@ static void handle_bad_merge_base(void)
 	if (is_expected_rev(current_bad_oid)) {
 		char *bad_hex = oid_to_hex(current_bad_oid);
 		char *good_hex = join_sha1_array_hex(&good_revs, ' ');
-
-		fprintf(stderr, "The merge base %s is bad.\n"
-			"This means the bug has been fixed "
-			"between %s and [%s].\n",
-			bad_hex, bad_hex, good_hex);
-
+		if (!strcmp(name_bad, "bad") && !strcmp(name_good, "good")) {
+			fprintf(stderr, "The merge base %s is bad.\n"
+				"This means the bug has been fixed "
+				"between %s and [%s].\n",
+				bad_hex, bad_hex, good_hex);
+		} else {
+			fprintf(stderr, "The merge base %s is %s.\n"
+				"This means the first '%s' commit is "
+				"between %s and [%s].\n",
+				bad_hex, name_bad, name_good, bad_hex, good_hex);
+		}
 		exit(3);
 	}
 
-	fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
+	fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
 		"git bisect cannot work properly in this case.\n"
-		"Maybe you mistook good and bad revs?\n");
+		"Maybe you mistook %s and %s revs?\n",
+		name_good, name_bad, name_good, name_bad);
 	exit(1);
 }
 
@@ -755,10 +770,10 @@ static void handle_skipped_merge_base(const unsigned char *mb)
 
 	warning("the merge base between %s and [%s] "
 		"must be skipped.\n"
-		"So we cannot be sure the first bad commit is "
+		"So we cannot be sure the first %s commit is "
 		"between %s and %s.\n"
 		"We continue anyway.",
-		bad_hex, good_hex, mb_hex, bad_hex);
+		bad_hex, good_hex, name_bad, mb_hex, bad_hex);
 	free(good_hex);
 }
 
@@ -839,7 +854,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
 	int fd;
 
 	if (!current_bad_oid)
-		die("a bad revision is needed");
+		die("a %s revision is needed", name_bad);
 
 	/* Check if file BISECT_ANCESTORS_OK exists. */
 	if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -905,6 +920,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	const unsigned char *bisect_rev;
 	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
+	name_bad = "bad";
+	name_good = "good";
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
@@ -926,8 +943,10 @@ int bisect_next_all(const char *prefix, int no_checkout)
 		 */
 		exit_if_skipped_commits(tried, NULL);
 
-		printf("%s was both good and bad\n",
-		       oid_to_hex(current_bad_oid));
+		printf("%s was both %s and %s\n",
+		       oid_to_hex(current_bad_oid),
+		       name_good,
+		       name_bad);
 		exit(1);
 	}
 
@@ -942,7 +961,8 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
 	if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
 		exit_if_skipped_commits(tried, current_bad_oid);
-		printf("%s is the first bad commit\n", bisect_rev_hex);
+		printf("%s is the first %s commit\n", bisect_rev_hex,
+			name_bad);
 		show_diff_tree(prefix, revs.commits->item);
 		/* This means the bisection process succeeded. */
 		exit(10);
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3fec2..ce6412f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -32,6 +32,8 @@ 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"
+NAME_BAD=bad
+NAME_GOOD=good
 
 bisect_head()
 {
@@ -100,8 +102,8 @@ bisect_start() {
 				break
 			}
 			case $bad_seen in
-			0) state='bad' ; bad_seen=1 ;;
-			*) state='good' ;;
+			0) state=$NAME_BAD ; bad_seen=1 ;;
+			*) state=$NAME_GOOD ;;
 			esac
 			eval="$eval bisect_write '$state' '$rev' 'nolog' &&"
 			shift
@@ -184,9 +186,12 @@ bisect_write() {
 	rev="$2"
 	nolog="$3"
 	case "$state" in
-		bad)		tag="$state" ;;
-		good|skip)	tag="$state"-"$rev" ;;
-		*)		die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
+		"$NAME_BAD")
+			tag="$state" ;;
+		"$NAME_GOOD"|skip)
+			tag="$state"-"$rev" ;;
+		*)
+			die "$(eval_gettext "Bad bisect_write argument: \$state")" ;;
 	esac
 	git update-ref "refs/bisect/$tag" "$rev" || exit
 	echo "# $state: $(git show-branch $rev)" >>"$GIT_DIR/BISECT_LOG"
@@ -230,12 +235,12 @@ bisect_state() {
 	case "$#,$state" in
 	0,*)
 		die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
-	1,bad|1,good|1,skip)
+	1,"$NAME_BAD"|1,"$NAME_GOOD"|1,skip)
 		rev=$(git rev-parse --verify $(bisect_head)) ||
 			die "$(gettext "Bad rev input: $(bisect_head)")"
 		bisect_write "$state" "$rev"
 		check_expected_revs "$rev" ;;
-	2,bad|*,good|*,skip)
+	2,"$NAME_BAD"|*,"$NAME_GOOD"|*,skip)
 		shift
 		hash_list=''
 		for rev in "$@"
@@ -249,8 +254,8 @@ bisect_state() {
 			bisect_write "$state" "$rev"
 		done
 		check_expected_revs $hash_list ;;
-	*,bad)
-		die "$(gettext "'git bisect bad' can take only one argument.")" ;;
+	*,"$NAME_BAD")
+		die "$(eval_gettext "'git bisect \$NAME_BAD' can take only one argument.")" ;;
 	*)
 		usage ;;
 	esac
@@ -259,21 +264,21 @@ bisect_state() {
 
 bisect_next_check() {
 	missing_good= missing_bad=
-	git show-ref -q --verify refs/bisect/bad || missing_bad=t
-	test -n "$(git for-each-ref "refs/bisect/good-*")" || missing_good=t
+	git show-ref -q --verify refs/bisect/$NAME_BAD || missing_bad=t
+	test -n "$(git for-each-ref "refs/bisect/$NAME_GOOD-*")" || missing_good=t
 
 	case "$missing_good,$missing_bad,$1" in
 	,,*)
-		: have both good and bad - ok
+		: have both $NAME_GOOD and $NAME_BAD - ok
 		;;
 	*,)
 		# do not have both but not asked to fail - just report.
 		false
 		;;
-	t,,good)
+	t,,"$NAME_GOOD")
 		# have bad but not good.  we could bisect although
 		# this is less optimum.
-		gettextln "Warning: bisecting only with a bad commit." >&2
+		eval_gettextln "Warning: bisecting only with a \$NAME_BAD commit." >&2
 		if test -t 0
 		then
 			# TRANSLATORS: Make sure to include [Y] and [n] in your
@@ -283,7 +288,7 @@ bisect_next_check() {
 			read yesno
 			case "$yesno" in [Nn]*) exit 1 ;; esac
 		fi
-		: bisect without good...
+		: bisect without $NAME_GOOD...
 		;;
 	*)
 
@@ -307,7 +312,7 @@ bisect_auto_next() {
 bisect_next() {
 	case "$#" in 0) ;; *) usage ;; esac
 	bisect_autostart
-	bisect_next_check good
+	bisect_next_check $NAME_GOOD
 
 	# Perform all bisection computation, display and checkout
 	git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
@@ -316,18 +321,18 @@ bisect_next() {
 	# Check if we should exit because bisection is finished
 	if test $res -eq 10
 	then
-		bad_rev=$(git show-ref --hash --verify refs/bisect/bad)
+		bad_rev=$(git show-ref --hash --verify refs/bisect/$NAME_BAD)
 		bad_commit=$(git show-branch $bad_rev)
-		echo "# first bad commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
+		echo "# first $NAME_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
 		exit 0
 	elif test $res -eq 2
 	then
 		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
-		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/good-*")
-		for skipped in $(git rev-list refs/bisect/bad --not $good_revs)
+		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$NAME_GOOD-*")
+		for skipped in $(git rev-list refs/bisect/$NAME_BAD --not $good_revs)
 		do
 			skipped_commit=$(git show-branch $skipped)
-			echo "# possible first bad commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
+			echo "# possible first $NAME_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
 		done
 		exit $res
 	fi
@@ -421,7 +426,7 @@ bisect_replay () {
 		start)
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
-		good|bad|skip)
+		$NAME_GOOD|$NAME_BAD|skip)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
@@ -455,9 +460,9 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 			state='skip'
 		elif [ $res -gt 0 ]
 		then
-			state='bad'
+			state="$NAME_BAD"
 		else
-			state='good'
+			state="$NAME_GOOD"
 		fi
 
 		# We have to use a subshell because "bisect_state" can exit.
@@ -466,7 +471,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 
 		cat "$GIT_DIR/BISECT_RUN"
 
-		if sane_grep "first bad commit could be any of" "$GIT_DIR/BISECT_RUN" \
+		if sane_grep "first $NAME_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
 			>/dev/null
 		then
 			gettextln "bisect run cannot continue any more" >&2
@@ -480,7 +485,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 			exit $res
 		fi
 
-		if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" >/dev/null
+		if sane_grep "is the first $NAME_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null
 		then
 			gettextln "bisect run success"
 			exit 0;
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 5/7] bisect: simplify the addition of new bisect terms

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

We create a file BISECT_TERMS in the repository .git to be read during a
bisection. The fonctions to be changed if we add new terms are quite
few.
In git-bisect.sh :
	check_and_set_terms
	bisect_voc

Co-authored-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Tweaked-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 bisect.c      | 33 ++++++++++++++++++++++++--
 git-bisect.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 revision.c    | 20 ++++++++++++++--
 3 files changed, 118 insertions(+), 11 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9368f6c..c5f96eb 100644
--- a/bisect.c
+++ b/bisect.c
@@ -905,6 +905,36 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 }
 
 /*
+ * The terms used for this bisect session are stored in BISECT_TERMS.
+ * We read them and store them to adapt the messages accordingly.
+ * Default is bad/good.
+ */
+void read_bisect_terms(const char **read_bad, const char **read_good)
+{
+	struct strbuf str = STRBUF_INIT;
+	const char *filename = git_path("BISECT_TERMS");
+	FILE *fp = fopen(filename, "r");
+
+	if (!fp) {
+		if (errno == ENOENT) {
+			*read_bad = "bad";
+			*read_good = "good";
+			return;
+		} else {
+			die("could not read file '%s': %s", filename,
+				strerror(errno));
+		}
+	} else {
+		strbuf_getline(&str, fp, '\n');
+		*read_bad = strbuf_detach(&str, NULL);
+		strbuf_getline(&str, fp, '\n');
+		*read_good = strbuf_detach(&str, NULL);
+	}
+	strbuf_release(&str);
+	fclose(fp);
+}
+
+/*
  * We use the convention that exiting with an exit code 10 means that
  * the bisection process finished successfully.
  * In this case the calling shell script should exit 0.
@@ -920,8 +950,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	const unsigned char *bisect_rev;
 	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-	name_bad = "bad";
-	name_good = "good";
+	read_bisect_terms(&name_bad, &name_good);
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
diff --git a/git-bisect.sh b/git-bisect.sh
index ce6412f..52e41e8 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -77,6 +77,7 @@ bisect_start() {
 	orig_args=$(git rev-parse --sq-quote "$@")
 	bad_seen=0
 	eval=''
+	must_write_terms=0
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -101,6 +102,14 @@ bisect_start() {
 				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
 				break
 			}
+
+			# The user ran "git bisect start <sha1>
+			# <sha1>", hence did not explicitly specify
+			# the terms, but we are already starting to
+			# set references named with the default terms,
+			# and won't be able to change afterwards.
+			must_write_terms=1
+
 			case $bad_seen in
 			0) state=$NAME_BAD ; bad_seen=1 ;;
 			*) state=$NAME_GOOD ;;
@@ -172,6 +181,10 @@ bisect_start() {
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
+	if test $must_write_terms -eq 1
+	then
+		write_terms "$NAME_BAD" "$NAME_GOOD"
+	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
 	# Check if we can proceed to the next bisect state.
@@ -232,6 +245,7 @@ bisect_skip() {
 bisect_state() {
 	bisect_autostart
 	state=$1
+	check_and_set_terms $state
 	case "$#,$state" in
 	0,*)
 		die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
@@ -291,15 +305,17 @@ bisect_next_check() {
 		: bisect without $NAME_GOOD...
 		;;
 	*)
-
+		bad_syn=$(bisect_voc bad)
+		good_syn=$(bisect_voc good)
 		if test -s "$GIT_DIR/BISECT_START"
 		then
-			gettextln "You need to give me at least one good and one bad revision.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
+
+			eval_gettextln "You need to give me at least one \$bad_syn and one \$good_syn revision.
+(You can use \"git bisect \$bad_syn\" and \"git bisect \$good_syn\" for that.)" >&2
 		else
-			gettextln "You need to start by \"git bisect start\".
-You then need to give me at least one good and one bad revision.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
+			eval_gettextln "You need to start by \"git bisect start\".
+You then need to give me at least one \$good_syn and one \$bad_syn revision.
+(You can use \"git bisect \$bad_syn\" and \"git bisect \$good_syn\" for that.)" >&2
 		fi
 		exit 1 ;;
 	esac
@@ -402,6 +418,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_TERMS" &&
 	# 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 &&
@@ -422,11 +439,13 @@ bisect_replay () {
 			rev="$command"
 			command="$bisect"
 		fi
+		get_terms
+		check_and_set_terms "$command"
 		case "$command" in
 		start)
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
-		$NAME_GOOD|$NAME_BAD|skip)
+		"$NAME_GOOD"|"$NAME_BAD"|skip)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
@@ -499,11 +518,54 @@ bisect_log () {
 	cat "$GIT_DIR/BISECT_LOG"
 }
 
+get_terms () {
+	if test -s "$GIT_DIR/BISECT_TERMS"
+	then
+		{
+		read NAME_BAD
+		read NAME_GOOD
+		} <"$GIT_DIR/BISECT_TERMS"
+	fi
+}
+
+write_terms () {
+	NAME_BAD=$1
+	NAME_GOOD=$2
+	printf '%s\n%s\n' "$NAME_BAD" "$NAME_GOOD" >"$GIT_DIR/BISECT_TERMS"
+}
+
+check_and_set_terms () {
+	cmd="$1"
+	case "$cmd" in
+	bad|good)
+		if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != "$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
+		then
+			die "$(eval_gettext "Invalid command: you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
+		fi
+		case "$cmd" in
+		bad|good)
+			if ! test -s "$GIT_DIR/BISECT_TERMS"
+			then
+				write_terms bad good
+			fi
+			;;
+		esac ;;
+	esac
+}
+
+bisect_voc () {
+	case "$1" in
+	bad) echo "bad" ;;
+	good) echo "good" ;;
+	esac
+}
+
 case "$#" in
 0)
 	usage ;;
 *)
 	cmd="$1"
+	get_terms
 	shift
 	case "$cmd" in
 	help)
diff --git a/revision.c b/revision.c
index 3ff8723..24ce842 100644
--- a/revision.c
+++ b/revision.c
@@ -21,6 +21,9 @@
 
 volatile show_early_output_fn_t show_early_output;
 
+static const char *name_bad;
+static const char *name_good;
+
 char *path_name(const struct name_path *path, const char *name)
 {
 	const struct name_path *p;
@@ -2076,14 +2079,26 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 	ctx->argc -= n;
 }
 
+extern void read_bisect_terms(const char **bad, const char **good);
+
 static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
+	struct strbuf bisect_refs = STRBUF_INIT;
+	int status;
+	strbuf_addf(&bisect_refs, "refs/bisect/%s", name_bad);
+	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
+	strbuf_release(&bisect_refs);
+	return status;
 }
 
 static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+	struct strbuf bisect_refs = STRBUF_INIT;
+	int status;
+	strbuf_addf(&bisect_refs, "refs/bisect/%s", name_good);
+	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
+	strbuf_release(&bisect_refs);
+	return status;
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2112,6 +2127,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
+		read_bisect_terms(&name_bad, &name_good);
 		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
 		handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);
 		revs->bisect = 1;
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 6/7] bisect: add the terms old/new

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

When not looking for a regression during a bisect but for a fix or a
change in another given property, it can be confusing to use 'good'
and 'bad'.

This patch introduce `git bisect new` and `git bisect old` as an
alternative to 'bad' and good': the commits which have a certain
property must be marked as `new` and the ones which do not as `old`.

The output will be the first commit after the change in the property.
During a new/old bisect session you cannot use bad/good commands and
vice-versa.

Some commands are still not available for old/new:
     * git rev-list --bisect does not treat the revs/bisect/new and
       revs/bisect/old-SHA1 files.

Old discussions:
	- http://thread.gmane.org/gmane.comp.version-control.git/86063
		introduced bisect fix unfixed to find fix.
	- http://thread.gmane.org/gmane.comp.version-control.git/182398
		discussion around bisect yes/no or old/new.
	- http://thread.gmane.org/gmane.comp.version-control.git/199758
		last discussion and reviews
New discussions:
	- http://thread.gmane.org/gmane.comp.version-control.git/271320
		( v2 1/7-4/7 )
	- http://comments.gmane.org/gmane.comp.version-control.git/271343
		( v2 5/7-7/7 )

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-bisect.txt | 58 ++++++++++++++++++++++++++++++++++++++++++--
 bisect.c                     | 11 ++++++---
 git-bisect.sh                | 28 +++++++++++++--------
 t/t6030-bisect-porcelain.sh  | 38 +++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 8e8a073..24171a5 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,8 +17,8 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
- git bisect bad [<rev>]
- git bisect good [<rev>...]
+ git bisect (bad|new) [<rev>]
+ git bisect (good|old) [<rev>...]
  git bisect skip [(<rev>|<range>)...]
  git bisect reset [<commit>]
  git bisect visualize
@@ -36,6 +36,13 @@ whether the selected commit is "good" or "bad". It continues narrowing
 down the range until it finds the exact commit that introduced the
 change.
 
+In fact, `git bisect` can be used to find the commit that changed
+*any* property of your project; e.g., the commit that fixed a bug, or
+the commit that caused a benchmark's performance to improve. To
+support this more general usage, the terms "old" and "new" can be used
+in place of "good" and "bad". See
+section "Alternate terms" below for more information.
+
 Basic bisect commands: start, bad, good
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -111,6 +118,45 @@ bad revision, while `git bisect reset HEAD` will leave you on the
 current bisection commit and avoid switching commits at all.
 
 
+Alternate terms
+~~~~~~~~~~~~~~~
+
+Sometimes you are not looking for the commit that introduced a
+breakage, but rather for a commit that caused a change between some
+other "old" state and "new" state. For example, you might be looking
+for the commit that introduced a particular fix. Or you might be
+looking for the first commit in which the source-code filenames were
+finally all converted to your company's naming standard. Or whatever.
+
+In such cases it can be very confusing to use the terms "good" and
+"bad" to refer to "the state before the change" and "the state after
+the change". So instead, you can use the terms "old" and "new",
+respectively, in place of "good" and "bad". (But note that you cannot
+mix "good" and "bad" with "old" and "new" in a single session.)
+
+In this more general usage, you provide `git bisect` with a "new"
+commit has some property and an "old" commit that doesn't have that
+property. Each time `git bisect` checks out a commit, you test if that
+commit has the property. If it does, mark the commit as "new";
+otherwise, mark it as "old". When the bisection is done, `git bisect`
+will report which commit introduced the property.
+
+To use "old" and "new" instead of "good" and bad, you must run `git
+bisect start` without commits as argument and then run the following
+commands to add the commits:
+
+------------------------------------------------
+git bisect old [<rev>]
+------------------------------------------------
+
+to indicate that a commit was before the sought change, or
+
+------------------------------------------------
+git bisect new [<rev>...]
+------------------------------------------------
+
+to indicate that it was after.
+
 Bisect visualize
 ~~~~~~~~~~~~~~~~
 
@@ -387,6 +433,14 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
 has at least one parent whose reachable graph is fully traversable in the sense
 required by 'git pack objects'.
 
+* Look for a fix instead of a regression in the code
++
+------------
+$ git bisect start
+$ git bisect new HEAD    # current commit is marked as new
+$ git bisect old HEAD~10 # the tenth commit from now is marked as old
+------------
+
 Getting help
 ~~~~~~~~~~~~
 
diff --git a/bisect.c b/bisect.c
index c5f96eb..d447b65 100644
--- a/bisect.c
+++ b/bisect.c
@@ -746,6 +746,11 @@ static void handle_bad_merge_base(void)
 				"This means the bug has been fixed "
 				"between %s and [%s].\n",
 				bad_hex, bad_hex, good_hex);
+		} else if (!strcmp(name_bad, "new") && !strcmp(name_good, "old")) {
+			fprintf(stderr, "The merge base %s is new.\n"
+				"The property has changed "
+				"between %s and [%s].\n",
+				bad_hex, bad_hex, good_hex);
 		} else {
 			fprintf(stderr, "The merge base %s is %s.\n"
 				"This means the first '%s' commit is "
@@ -778,11 +783,11 @@ static void handle_skipped_merge_base(const unsigned char *mb)
 }
 
 /*
- * "check_merge_bases" checks that merge bases are not "bad".
+ * "check_merge_bases" checks that merge bases are not "bad" (or "new").
  *
- * - If one is "bad", it means the user assumed something wrong
+ * - If one is "bad" (or "new"), it means the user assumed something wrong
  * and we must exit with a non 0 error code.
- * - If one is "good", that's good, we have nothing to do.
+ * - If one is "good" (or "old"), that's good, we have nothing to do.
  * - If one is "skipped", we can't know but we should warn.
  * - If we don't know, we should check it out and ask the user to test.
  */
diff --git a/git-bisect.sh b/git-bisect.sh
index 52e41e8..5769eaf 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,14 +1,16 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|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.
-git bisect bad [<rev>]
-	mark <rev> a known-bad revision.
-git bisect good [<rev>...]
-	mark <rev>... known-good revisions.
+git bisect (bad|new) [<rev>]
+	mark <rev> a known-bad revision/
+		a revision after change in a given property.
+git bisect (good|old) [<rev>...]
+	mark <rev>... known-good revisions/
+		revisions before change in a given property.
 git bisect skip [(<rev>|<range>)...]
 	mark <rev>... untestable revisions.
 git bisect next
@@ -290,7 +292,7 @@ bisect_next_check() {
 		false
 		;;
 	t,,"$NAME_GOOD")
-		# have bad but not good.  we could bisect although
+		# have bad (or new) but not good (or old).  we could bisect although
 		# this is less optimum.
 		eval_gettextln "Warning: bisecting only with a \$NAME_BAD commit." >&2
 		if test -t 0
@@ -537,7 +539,7 @@ write_terms () {
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-	bad|good)
+	bad|good|new|old)
 		if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != "$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
 		then
 			die "$(eval_gettext "Invalid command: you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
@@ -549,14 +551,20 @@ check_and_set_terms () {
 				write_terms bad good
 			fi
 			;;
+		new|old)
+			if ! test -s "$GIT_DIR/BISECT_TERMS"
+			then
+				write_terms new old
+			fi
+			;;
 		esac ;;
 	esac
 }
 
 bisect_voc () {
 	case "$1" in
-	bad) echo "bad" ;;
-	good) echo "good" ;;
+	bad) echo "bad|new" ;;
+	good) echo "good|old" ;;
 	esac
 }
 
@@ -572,7 +580,7 @@ case "$#" in
 		git bisect -h ;;
 	start)
 		bisect_start "$@" ;;
-	bad|good)
+	bad|good|new|old)
 		bisect_state "$cmd" "$@" ;;
 	skip)
 		bisect_skip "$@" ;;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 9e2c203..983c503 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -759,4 +759,42 @@ test_expect_success '"git bisect bad HEAD" behaves as "git bisect bad"' '
 	git bisect reset
 '
 
+test_expect_success 'bisect starts with only one new' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect new $HASH4 &&
+	git bisect next
+'
+
+test_expect_success 'bisect does not start with only one old' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect old $HASH1 &&
+	test_must_fail git bisect next
+'
+
+test_expect_success 'bisect start with one new and old' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect old $HASH1 &&
+	git bisect new $HASH4 &&
+	git bisect new &&
+	git bisect new >bisect_result &&
+	grep "$HASH2 is the first new commit" bisect_result &&
+	git bisect log >log_to_replay.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bisect replay with old and new' '
+	git bisect replay log_to_replay.txt >bisect_result &&
+	grep "$HASH2 is the first new commit" bisect_result &&
+	git bisect reset
+'
+
+test_expect_success 'bisect cannot mix old/new and good/bad' '
+	git bisect start &&
+	git bisect bad $HASH4 &&
+	test_must_fail git bisect old $HASH1
+'
+
 test_done
-- 
2.4.4.414.g318df7a.dirty

[PATCH v10 7/7] bisect: allow any terms set by user

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

Introduction of the git bisect terms command. The user can set his own
terms. It will work exactly like before. The terms must be set before the
start.

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-bisect.txt | 36 +++++++++++++++++++-
 git-bisect.sh                | 65 +++++++++++++++++++++++++++++++++---
 t/t6030-bisect-porcelain.sh  | 79 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 24171a5..b1ef41c 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -19,6 +19,7 @@ on the subcommand:
  git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new) [<rev>]
  git bisect (good|old) [<rev>...]
+ git bisect terms <term-old> <term-new>
  git bisect skip [(<rev>|<range>)...]
  git bisect reset [<commit>]
  git bisect visualize
@@ -40,7 +41,7 @@ In fact, `git bisect` can be used to find the commit that changed
 *any* property of your project; e.g., the commit that fixed a bug, or
 the commit that caused a benchmark's performance to improve. To
 support this more general usage, the terms "old" and "new" can be used
-in place of "good" and "bad". See
+in place of "good" and "bad", or you can choose your own terms. See
 section "Alternate terms" below for more information.
 
 Basic bisect commands: start, bad, good
@@ -157,6 +158,31 @@ git bisect new [<rev>...]
 
 to indicate that it was after.
 
+If you would like to use your own terms instead of "bad"/"good" or
+"new"/"old", you can choose any names you like by typing
+
+------------------------------------------------
+git bisect terms <term-new> <term-old>
+------------------------------------------------
+
+before starting a bisection session. For example, if you are looking
+for a commit that introduced a performance regression, you might use
+
+------------------------------------------------
+git bisect terms slow fast
+------------------------------------------------
+
+Or if you are looking for the commit that fixed a bug, you might use
+
+------------------------------------------------
+git bisect terms fixed broken
+------------------------------------------------
+
+Only the bisection following the `git bisect terms` will use the
+terms. If you mistyped one of the terms you can do again `git bisect
+terms <term-new> <term-old>`, but that is possible only before you
+start the bisection.
+
 Bisect visualize
 ~~~~~~~~~~~~~~~~
 
@@ -440,6 +466,14 @@ $ git bisect start
 $ git bisect new HEAD    # current commit is marked as new
 $ git bisect old HEAD~10 # the tenth commit from now is marked as old
 ------------
++
+or:
+------------
+$ git bisect terms fixed broken
+$ git bisect start
+$ git bisect fixed
+$ git bisect broken HEAD~10
+------------
 
 Getting help
 ~~~~~~~~~~~~
diff --git a/git-bisect.sh b/git-bisect.sh
index 5769eaf..64ea122 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|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>...]
@@ -11,6 +11,8 @@ git bisect (bad|new) [<rev>]
 git bisect (good|old) [<rev>...]
 	mark <rev>... known-good revisions/
 		revisions before change in a given property.
+git bisect terms <term-new> <term-old>
+	set up <term-new> and <term-old> as terms (default: bad, good)
 git bisect skip [(<rev>|<range>)...]
 	mark <rev>... untestable revisions.
 git bisect next
@@ -80,6 +82,16 @@ bisect_start() {
 	bad_seen=0
 	eval=''
 	must_write_terms=0
+	must_log_terms=0
+	if test -s "$GIT_DIR/BISECT_TERMS"
+	then
+		# We're going to restart from a clean state and the
+		# file will be deleted. Record the old state in
+		# variables and restore it below.
+		must_write_terms=1
+		must_log_terms=1
+		get_terms
+	fi
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -185,7 +197,12 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$NAME_BAD" "$NAME_GOOD"
+		write_terms "$NAME_BAD" "$NAME_GOOD" &&
+		if test $must_log_terms -eq 1
+		then
+			echo "git bisect terms $NAME_BAD $NAME_GOOD" \
+			    >>"$GIT_DIR/BISECT_LOG"
+		fi
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -449,6 +466,8 @@ bisect_replay () {
 			eval "$cmd" ;;
 		"$NAME_GOOD"|"$NAME_BAD"|skip)
 			bisect_write "$command" "$rev" ;;
+		terms)
+			bisect_terms $rev ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
@@ -533,13 +552,22 @@ get_terms () {
 write_terms () {
 	NAME_BAD=$1
 	NAME_GOOD=$2
+	check_term_format "$NAME_BAD"
+	check_term_format "$NAME_GOOD"
 	printf '%s\n%s\n' "$NAME_BAD" "$NAME_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
+check_term_format () {
+	term=$1
+	git check-ref-format refs/bisect/"$term" ||
+	die "$(eval_gettext "'\$term' is not a valid term")"
+}
+
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-	bad|good|new|old)
+	skip|start|terms) ;;
+	*)
 		if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != "$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
 		then
 			die "$(eval_gettext "Invalid command: you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
@@ -568,6 +596,33 @@ bisect_voc () {
 	esac
 }
 
+bisect_terms () {
+	case "$#" in
+	0)
+		if test -s "$GIT_DIR/BISECT_TERMS"
+		then
+			get_terms
+			gettextln "Your current terms are $NAME_GOOD for the old state
+and $NAME_BAD for the new state."
+		else
+			die "$(gettext "No terms defined.")"
+		fi ;;
+	2)
+		if ! test -s "$GIT_DIR/BISECT_START"
+		then
+			write_terms "$1" "$2"
+			echo "git bisect terms $NAME_BAD $NAME_GOOD" >>"$GIT_DIR/BISECT_LOG" || exit
+		else
+			die "$(gettext "A bisection has already started, and you can't change terms in the middle of it.
+Use 'git bisect terms' to see the current terms.
+Otherwise, to start a new bisection with new terms, please use
+'git bisect reset' and set the terms before the start")"
+		fi ;;
+	*)
+		usage ;;
+	esac
+}
+
 case "$#" in
 0)
 	usage ;;
@@ -580,7 +635,7 @@ case "$#" in
 		git bisect -h ;;
 	start)
 		bisect_start "$@" ;;
-	bad|good|new|old)
+	bad|good|new|old|"$NAME_BAD"|"$NAME_GOOD")
 		bisect_state "$cmd" "$@" ;;
 	skip)
 		bisect_skip "$@" ;;
@@ -597,6 +652,8 @@ case "$#" in
 		bisect_log ;;
 	run)
 		bisect_run "$@" ;;
+	terms)
+		bisect_terms "$@" ;;
 	*)
 		usage ;;
 	esac
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 983c503..83254d2 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -797,4 +797,83 @@ test_expect_success 'bisect cannot mix old/new and good/bad' '
 	test_must_fail git bisect old $HASH1
 '
 
+test_expect_success 'bisect start with one term1 and term2' '
+	git bisect reset &&
+	git bisect terms term1 term2 &&
+	git bisect start &&
+	git bisect term2 $HASH1 &&
+	git bisect term1 $HASH4 &&
+	git bisect term1 &&
+	git bisect term1 >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect log >log_to_replay.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bisect replay with term1 and term2' '
+	git bisect replay log_to_replay.txt >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect reset
+'
+
+test_expect_success 'bisect start term1 term2' '
+	git bisect reset &&
+	git bisect terms term1 term2 &&
+	git bisect start $HASH4 $HASH1 &&
+	git bisect term1 &&
+	git bisect term1 >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect log >log_to_replay.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bisect cannot mix terms' '
+	git bisect reset &&
+	git bisect terms a b &&
+	git bisect terms term1 term2 &&
+	git bisect start $HASH4 $HASH1 &&
+	test_must_fail git bisect a &&
+	test_must_fail git bisect b &&
+	test_must_fail git bisect bad &&
+	test_must_fail git bisect good &&
+	test_must_fail git bisect new &&
+	test_must_fail git bisect old
+'
+
+test_expect_success 'bisect terms rejects invalid terms' '
+	git bisect reset &&
+	test_must_fail git bisect terms invalid..term term2 &&
+	test_must_fail git bisect terms term1 invalid..term &&
+	test_path_is_missing .git/BISECT_TERMS
+'
+
+test_expect_success 'bisect terms needs 0 or 2 arguments' '
+	git bisect reset &&
+	test_must_fail git bisect terms only-one &&
+	test_must_fail git bisect terms 1 2 3 &&
+	test_must_fail git bisect terms 2>actual
+'
+
+test_expect_success 'bisect terms does store terms' '
+	echo "No terms defined." >expected &&
+	test_cmp expected actual &&
+	git bisect terms one two &&
+	git bisect terms >actual &&
+	cat <<-EOF >expected &&
+	Your current terms are two for the old state
+	and one for the new state.
+	EOF
+	test_cmp expected actual'
+
+test_expect_success 'bisect start keeps defined terms' '
+	git bisect terms one two &&
+	git bisect start &&
+	git bisect terms >actual &&
+	cat <<-EOF >expected &&
+	Your current terms are two for the old state
+	and one for the new state.
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.4.4.414.g318df7a.dirty

Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index 24171a5..b1ef41c 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -19,6 +19,7 @@ on the subcommand:
>   git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
>   git bisect (bad|new) [<rev>]
>   git bisect (good|old) [<rev>...]
> + git bisect terms <term-old> <term-new>

I think this is the other way around.

Other than that, this round looked OK to me.

Thanks.

Re: [PATCH v10 5/7] bisect: simplify the addition of new bisect terms

[Christian Couder]

On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>
>  static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -       return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
> +       struct strbuf bisect_refs = STRBUF_INIT;
> +       int status;
> +       strbuf_addf(&bisect_refs, "refs/bisect/%s", name_bad);
> +       status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
> +       strbuf_release(&bisect_refs);
> +       return status;
>  }
>
>  static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -       return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
> +       struct strbuf bisect_refs = STRBUF_INIT;
> +       int status;
> +       strbuf_addf(&bisect_refs, "refs/bisect/%s", name_good);
> +       status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
> +       strbuf_release(&bisect_refs);
> +       return status;
>  }

I wonder if it would not be better to just have:

static int for_each_bisect_ref(const char *submodule, each_ref_fn fn,
const char *term, void *cb_data)
{
      struct strbuf bisect_refs = STRBUF_INIT;
      int status;
      strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
      status = for_each_ref_in_submodule(submodule, bisect_refs.buf,
fn, cb_data);
      strbuf_release(&bisect_refs);
      return status;
}

This way it can be used with either name_good, name_bad or "skip" as
the term argument.

Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Christian Couder]

On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
>
> Introduction of the git bisect terms command. The user can set his own
> terms. It will work exactly like before. The terms must be set before the
> start.

After looking a bit at the code, I think that for now existing
predefined terms ("bad", "good", "new" and "old") as well as some
other terms that look like bisect subcommands like "skip", "start" and
"terms" should be disallowed as arguments to "git bisect terms", and
this should be stated in the commit message and in the documentation
as well as checked and tested.

For example a user might want to search for a fix by using "git bisect
terms good bad" (which should swap "good" and "bad"), but we should
not at least for now allow that.

> @@ -185,7 +197,12 @@ bisect_start() {
>         eval "$eval true" &&
>         if test $must_write_terms -eq 1
>         then
> -               write_terms "$NAME_BAD" "$NAME_GOOD"
> +               write_terms "$NAME_BAD" "$NAME_GOOD" &&
> +               if test $must_log_terms -eq 1
> +               then
> +                       echo "git bisect terms $NAME_BAD $NAME_GOOD" \
> +                           >>"$GIT_DIR/BISECT_LOG"
> +               fi

Maybe you could move appending to the log into write_terms() though
you might need to pass an additional argument to enable or disable
logging.

[PATCH v10.1 5/7] bisect: simplify the addition of new bisect terms

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

We create a file BISECT_TERMS in the repository .git to be read during a
bisection. The fonctions to be changed if we add new terms are quite few.

In git-bisect.sh:
	check_and_set_terms
	bisect_voc

Co-authored-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Tweaked-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Valentin Duperray <Valentin.Duperray@ensimag.imag.fr>
Signed-off-by: Franck Jonas <Franck.Jonas@ensimag.imag.fr>
Signed-off-by: Lucien Kong <Lucien.Kong@ensimag.imag.fr>
Signed-off-by: Thomas Nguy <Thomas.Nguy@ensimag.imag.fr>
Signed-off-by: Huynh Khoi Nguyen Nguyen <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
> I wonder if it would not be better to just have:
> 
> static int for_each_bisect_ref(const char *submodule, each_ref_fn fn,
> const char *term, void *cb_data)

We do need two functions because we pass the pointer as callback, but
it reads nicer with a third helper function.

 bisect.c      | 33 ++++++++++++++++++++++++--
 git-bisect.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 revision.c    | 19 +++++++++++++--
 3 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9368f6c..c5f96eb 100644
--- a/bisect.c
+++ b/bisect.c
@@ -905,6 +905,36 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 }
 
 /*
+ * The terms used for this bisect session are stored in BISECT_TERMS.
+ * We read them and store them to adapt the messages accordingly.
+ * Default is bad/good.
+ */
+void read_bisect_terms(const char **read_bad, const char **read_good)
+{
+	struct strbuf str = STRBUF_INIT;
+	const char *filename = git_path("BISECT_TERMS");
+	FILE *fp = fopen(filename, "r");
+
+	if (!fp) {
+		if (errno == ENOENT) {
+			*read_bad = "bad";
+			*read_good = "good";
+			return;
+		} else {
+			die("could not read file '%s': %s", filename,
+				strerror(errno));
+		}
+	} else {
+		strbuf_getline(&str, fp, '\n');
+		*read_bad = strbuf_detach(&str, NULL);
+		strbuf_getline(&str, fp, '\n');
+		*read_good = strbuf_detach(&str, NULL);
+	}
+	strbuf_release(&str);
+	fclose(fp);
+}
+
+/*
  * We use the convention that exiting with an exit code 10 means that
  * the bisection process finished successfully.
  * In this case the calling shell script should exit 0.
@@ -920,8 +950,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	const unsigned char *bisect_rev;
 	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-	name_bad = "bad";
-	name_good = "good";
+	read_bisect_terms(&name_bad, &name_good);
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
diff --git a/git-bisect.sh b/git-bisect.sh
index ce6412f..52e41e8 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -77,6 +77,7 @@ bisect_start() {
 	orig_args=$(git rev-parse --sq-quote "$@")
 	bad_seen=0
 	eval=''
+	must_write_terms=0
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -101,6 +102,14 @@ bisect_start() {
 				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
 				break
 			}
+
+			# The user ran "git bisect start <sha1>
+			# <sha1>", hence did not explicitly specify
+			# the terms, but we are already starting to
+			# set references named with the default terms,
+			# and won't be able to change afterwards.
+			must_write_terms=1
+
 			case $bad_seen in
 			0) state=$NAME_BAD ; bad_seen=1 ;;
 			*) state=$NAME_GOOD ;;
@@ -172,6 +181,10 @@ bisect_start() {
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
+	if test $must_write_terms -eq 1
+	then
+		write_terms "$NAME_BAD" "$NAME_GOOD"
+	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
 	# Check if we can proceed to the next bisect state.
@@ -232,6 +245,7 @@ bisect_skip() {
 bisect_state() {
 	bisect_autostart
 	state=$1
+	check_and_set_terms $state
 	case "$#,$state" in
 	0,*)
 		die "$(gettext "Please call 'bisect_state' with at least one argument.")" ;;
@@ -291,15 +305,17 @@ bisect_next_check() {
 		: bisect without $NAME_GOOD...
 		;;
 	*)
-
+		bad_syn=$(bisect_voc bad)
+		good_syn=$(bisect_voc good)
 		if test -s "$GIT_DIR/BISECT_START"
 		then
-			gettextln "You need to give me at least one good and one bad revision.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
+
+			eval_gettextln "You need to give me at least one \$bad_syn and one \$good_syn revision.
+(You can use \"git bisect \$bad_syn\" and \"git bisect \$good_syn\" for that.)" >&2
 		else
-			gettextln "You need to start by \"git bisect start\".
-You then need to give me at least one good and one bad revision.
-(You can use \"git bisect bad\" and \"git bisect good\" for that.)" >&2
+			eval_gettextln "You need to start by \"git bisect start\".
+You then need to give me at least one \$good_syn and one \$bad_syn revision.
+(You can use \"git bisect \$bad_syn\" and \"git bisect \$good_syn\" for that.)" >&2
 		fi
 		exit 1 ;;
 	esac
@@ -402,6 +418,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_TERMS" &&
 	# 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 &&
@@ -422,11 +439,13 @@ bisect_replay () {
 			rev="$command"
 			command="$bisect"
 		fi
+		get_terms
+		check_and_set_terms "$command"
 		case "$command" in
 		start)
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
-		$NAME_GOOD|$NAME_BAD|skip)
+		"$NAME_GOOD"|"$NAME_BAD"|skip)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
@@ -499,11 +518,54 @@ bisect_log () {
 	cat "$GIT_DIR/BISECT_LOG"
 }
 
+get_terms () {
+	if test -s "$GIT_DIR/BISECT_TERMS"
+	then
+		{
+		read NAME_BAD
+		read NAME_GOOD
+		} <"$GIT_DIR/BISECT_TERMS"
+	fi
+}
+
+write_terms () {
+	NAME_BAD=$1
+	NAME_GOOD=$2
+	printf '%s\n%s\n' "$NAME_BAD" "$NAME_GOOD" >"$GIT_DIR/BISECT_TERMS"
+}
+
+check_and_set_terms () {
+	cmd="$1"
+	case "$cmd" in
+	bad|good)
+		if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != "$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
+		then
+			die "$(eval_gettext "Invalid command: you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
+		fi
+		case "$cmd" in
+		bad|good)
+			if ! test -s "$GIT_DIR/BISECT_TERMS"
+			then
+				write_terms bad good
+			fi
+			;;
+		esac ;;
+	esac
+}
+
+bisect_voc () {
+	case "$1" in
+	bad) echo "bad" ;;
+	good) echo "good" ;;
+	esac
+}
+
 case "$#" in
 0)
 	usage ;;
 *)
 	cmd="$1"
+	get_terms
 	shift
 	case "$cmd" in
 	help)
diff --git a/revision.c b/revision.c
index 3ff8723..5cd08e9 100644
--- a/revision.c
+++ b/revision.c
@@ -21,6 +21,9 @@
 
 volatile show_early_output_fn_t show_early_output;
 
+static const char *name_bad;
+static const char *name_good;
+
 char *path_name(const struct name_path *path, const char *name)
 {
 	const struct name_path *p;
@@ -2076,14 +2079,25 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 	ctx->argc -= n;
 }
 
+extern void read_bisect_terms(const char **bad, const char **good);
+
+static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data, const char *term) {
+	struct strbuf bisect_refs = STRBUF_INIT;
+	int status;
+	strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
+	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
+	strbuf_release(&bisect_refs);
+	return status;
+}
+
 static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
+	return for_each_bisect_ref(submodule, fn, cb_data, "bad");
 }
 
 static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
 {
-	return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
+	return for_each_bisect_ref(submodule, fn, cb_data, "good");
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2112,6 +2126,7 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		handle_refs(submodule, revs, *flags, for_each_branch_ref_submodule);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--bisect")) {
+		read_bisect_terms(&name_bad, &name_good);
 		handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
 		handle_refs(submodule, revs, *flags ^ (UNINTERESTING | BOTTOM), for_each_good_bisect_ref);
 		revs->bisect = 1;
-- 
2.5.0.rc0.7.g0f487ca.dirty

[PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>

Introduction of the git bisect terms command. The user can set his own
terms. It will work exactly like before. The terms must be set before the
start.

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> + git bisect terms <term-old> <term-new>
>
> I think this is the other way around.

Indeed.

The test scripts were messed up, I sent too quickly after refactoring.

There was also a small bug: "git bisect reset" was not reseting the
terms when ran before "git bisect start".

My branch https://github.com/moy/git/commits/bisect-terms is up to
date in case the individual resend are too messy. I can also resend
later, but I was too ashamed of my bugs with tests to leave it as
is ;-).

 Documentation/git-bisect.txt | 36 +++++++++++++++++++-
 git-bisect.sh                | 67 +++++++++++++++++++++++++++++++++---
 t/t6030-bisect-porcelain.sh  | 81 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 179 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index abaf462..28f6104 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -19,6 +19,7 @@ on the subcommand:
  git bisect start [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new) [<rev>]
  git bisect (good|old) [<rev>...]
+ git bisect terms <term-new> <term-old>
  git bisect skip [(<rev>|<range>)...]
  git bisect reset [<commit>]
  git bisect visualize
@@ -40,7 +41,7 @@ In fact, `git bisect` can be used to find the commit that changed
 *any* property of your project; e.g., the commit that fixed a bug, or
 the commit that caused a benchmark's performance to improve. To
 support this more general usage, the terms "old" and "new" can be used
-in place of "good" and "bad". See
+in place of "good" and "bad", or you can choose your own terms. See
 section "Alternate terms" below for more information.
 
 Basic bisect commands: start, bad, good
@@ -157,6 +158,31 @@ git bisect new [<rev>...]
 
 to indicate that it was after.
 
+If you would like to use your own terms instead of "bad"/"good" or
+"new"/"old", you can choose any names you like by typing
+
+------------------------------------------------
+git bisect terms <term-new> <term-old>
+------------------------------------------------
+
+before starting a bisection session. For example, if you are looking
+for a commit that introduced a performance regression, you might use
+
+------------------------------------------------
+git bisect terms slow fast
+------------------------------------------------
+
+Or if you are looking for the commit that fixed a bug, you might use
+
+------------------------------------------------
+git bisect terms fixed broken
+------------------------------------------------
+
+Only the bisection following the `git bisect terms` will use the
+terms. If you mistyped one of the terms you can do again `git bisect
+terms <term-new> <term-old>`, but that is possible only before you
+start the bisection.
+
 Bisect visualize
 ~~~~~~~~~~~~~~~~
 
@@ -440,6 +466,14 @@ $ git bisect start
 $ git bisect new HEAD    # current commit is marked as new
 $ git bisect old HEAD~10 # the tenth commit from now is marked as old
 ------------
++
+or:
+------------
+$ git bisect terms fixed broken
+$ git bisect start
+$ git bisect fixed
+$ git bisect broken HEAD~10
+------------
 
 Getting help
 ~~~~~~~~~~~~
diff --git a/git-bisect.sh b/git-bisect.sh
index 5769eaf..cf07a91 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|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>...]
@@ -11,6 +11,8 @@ git bisect (bad|new) [<rev>]
 git bisect (good|old) [<rev>...]
 	mark <rev>... known-good revisions/
 		revisions before change in a given property.
+git bisect terms <term-new> <term-old>
+	set up <term-new> and <term-old> as terms (default: bad, good)
 git bisect skip [(<rev>|<range>)...]
 	mark <rev>... untestable revisions.
 git bisect next
@@ -80,6 +82,16 @@ bisect_start() {
 	bad_seen=0
 	eval=''
 	must_write_terms=0
+	must_log_terms=0
+	if test -s "$GIT_DIR/BISECT_TERMS"
+	then
+		# We're going to restart from a clean state and the
+		# file will be deleted. Record the old state in
+		# variables and restore it below.
+		must_write_terms=1
+		must_log_terms=1
+		get_terms
+	fi
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -185,7 +197,12 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$NAME_BAD" "$NAME_GOOD"
+		write_terms "$NAME_BAD" "$NAME_GOOD" &&
+		if test $must_log_terms -eq 1
+		then
+			echo "git bisect terms $NAME_BAD $NAME_GOOD" \
+			    >>"$GIT_DIR/BISECT_LOG"
+		fi
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -387,6 +404,8 @@ bisect_visualize() {
 bisect_reset() {
 	test -s "$GIT_DIR/BISECT_START" || {
 		gettextln "We are not bisecting."
+		# We may still have leftovers like BISECT_TERMS
+		bisect_clean_state
 		return
 	}
 	case "$#" in
@@ -449,6 +468,8 @@ bisect_replay () {
 			eval "$cmd" ;;
 		"$NAME_GOOD"|"$NAME_BAD"|skip)
 			bisect_write "$command" "$rev" ;;
+		terms)
+			bisect_terms $rev ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
@@ -533,13 +554,22 @@ get_terms () {
 write_terms () {
 	NAME_BAD=$1
 	NAME_GOOD=$2
+	check_term_format "$NAME_BAD"
+	check_term_format "$NAME_GOOD"
 	printf '%s\n%s\n' "$NAME_BAD" "$NAME_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
+check_term_format () {
+	term=$1
+	git check-ref-format refs/bisect/"$term" ||
+	die "$(eval_gettext "'\$term' is not a valid term")"
+}
+
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-	bad|good|new|old)
+	skip|start|terms) ;;
+	*)
 		if test -s "$GIT_DIR/BISECT_TERMS" && test "$cmd" != "$NAME_BAD" && test "$cmd" != "$NAME_GOOD"
 		then
 			die "$(eval_gettext "Invalid command: you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
@@ -568,6 +598,33 @@ bisect_voc () {
 	esac
 }
 
+bisect_terms () {
+	case "$#" in
+	0)
+		if test -s "$GIT_DIR/BISECT_TERMS"
+		then
+			get_terms
+			gettextln "Your current terms are $NAME_GOOD for the old state
+and $NAME_BAD for the new state."
+		else
+			die "$(gettext "No terms defined.")"
+		fi ;;
+	2)
+		if ! test -s "$GIT_DIR/BISECT_START"
+		then
+			write_terms "$1" "$2"
+			echo "git bisect terms $NAME_BAD $NAME_GOOD" >>"$GIT_DIR/BISECT_LOG" || exit
+		else
+			die "$(gettext "A bisection has already started, and you can't change terms in the middle of it.
+Use 'git bisect terms' to see the current terms.
+Otherwise, to start a new bisection with new terms, please use
+'git bisect reset' and set the terms before the start")"
+		fi ;;
+	*)
+		usage ;;
+	esac
+}
+
 case "$#" in
 0)
 	usage ;;
@@ -580,7 +637,7 @@ case "$#" in
 		git bisect -h ;;
 	start)
 		bisect_start "$@" ;;
-	bad|good|new|old)
+	bad|good|new|old|"$NAME_BAD"|"$NAME_GOOD")
 		bisect_state "$cmd" "$@" ;;
 	skip)
 		bisect_skip "$@" ;;
@@ -597,6 +654,8 @@ case "$#" in
 		bisect_log ;;
 	run)
 		bisect_run "$@" ;;
+	terms)
+		bisect_terms "$@" ;;
 	*)
 		usage ;;
 	esac
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 983c503..eb8cc80 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -797,4 +797,85 @@ test_expect_success 'bisect cannot mix old/new and good/bad' '
 	test_must_fail git bisect old $HASH1
 '
 
+test_expect_success 'bisect start with one term1 and term2' '
+	git bisect reset &&
+	git bisect terms term1 term2 &&
+	git bisect start &&
+	git bisect term2 $HASH1 &&
+	git bisect term1 $HASH4 &&
+	git bisect term1 &&
+	git bisect term1 >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect log >log_to_replay.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bisect replay with term1 and term2' '
+	git bisect replay log_to_replay.txt >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect reset
+'
+
+test_expect_success 'bisect start term1 term2' '
+	git bisect reset &&
+	git bisect terms term1 term2 &&
+	git bisect start $HASH4 $HASH1 &&
+	git bisect term1 &&
+	git bisect term1 >bisect_result &&
+	grep "$HASH2 is the first term1 commit" bisect_result &&
+	git bisect log >log_to_replay.txt &&
+	git bisect reset
+'
+
+test_expect_success 'bisect cannot mix terms' '
+	git bisect reset &&
+	git bisect terms a b &&
+	git bisect terms term1 term2 &&
+	git bisect start $HASH4 $HASH1 &&
+	test_must_fail git bisect a &&
+	test_must_fail git bisect b &&
+	test_must_fail git bisect bad &&
+	test_must_fail git bisect good &&
+	test_must_fail git bisect new &&
+	test_must_fail git bisect old
+'
+
+test_expect_success 'bisect terms rejects invalid terms' '
+	git bisect reset &&
+	test_must_fail git bisect terms invalid..term term2 &&
+	test_must_fail git bisect terms term1 invalid..term &&
+	test_path_is_missing .git/BISECT_TERMS
+'
+
+test_expect_success 'bisect terms needs 0 or 2 arguments' '
+	git bisect reset &&
+	test_must_fail git bisect terms only-one &&
+	test_must_fail git bisect terms 1 2 3 &&
+	test_must_fail git bisect terms 2>actual &&
+	echo "No terms defined." >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'bisect terms does store terms' '
+	git bisect reset &&
+	git bisect terms one two &&
+	git bisect terms >actual &&
+	cat <<-EOF >expected &&
+	Your current terms are two for the old state
+	and one for the new state.
+	EOF
+	test_cmp expected actual'
+
+test_expect_success 'bisect start keeps defined terms' '
+	git bisect reset &&
+	git bisect terms one two &&
+	git bisect start &&
+	git bisect terms >actual &&
+	cat <<-EOF >expected &&
+	Your current terms are two for the old state
+	and one for the new state.
+	EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
2.5.0.rc0.7.g0f487ca.dirty

Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>> From: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
>>
>> Introduction of the git bisect terms command. The user can set his own
>> terms. It will work exactly like before. The terms must be set before the
>> start.
>
> After looking a bit at the code, I think that for now existing
> predefined terms ("bad", "good", "new" and "old") as well as some
> other terms that look like bisect subcommands like "skip", "start" and
> "terms" should be disallowed as arguments to "git bisect terms",

More importantly, subcommands should be disallowed, or the user may
completely break bisect (e.g. running 'git bisect terms reset bar'
prevents you from running 'git bisect reset' later).

And they should be different, or some more funny situation will occur.

I've just squashed this into my last patch:

diff --git a/git-bisect.sh b/git-bisect.sh
index cf07a91..f6be218 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -607,9 +607,21 @@ bisect_terms () {
 			gettextln "Your current terms are $NAME_GOOD for the old state
 and $NAME_BAD for the new state."
 		else
-			die "$(gettext "No terms defined.")"
+			die "$(gettext "no terms defined")"
 		fi ;;
 	2)
+		for term in "$@"
+		do
+			case "$term" in
+				help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run)
+				die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
+				;;
+			esac
+		done
+		if test "$1" = "$2"
+		then
+			die "$(gettext "please use two different terms")"
+		fi
 		if ! test -s "$GIT_DIR/BISECT_START"
 		then
 			write_terms "$1" "$2"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index eb8cc80..5a7243b 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -852,7 +852,7 @@ test_expect_success 'bisect terms needs 0 or 2 arguments' '
 	test_must_fail git bisect terms only-one &&
 	test_must_fail git bisect terms 1 2 3 &&
 	test_must_fail git bisect terms 2>actual &&
-	echo "No terms defined." >expected &&
+	echo "no terms defined" >expected &&
 	test_cmp expected actual
 '
 
Updated my GitHub branch, but I'll stop spamming the list with git
send-email for a while ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 5/7] bisect: simplify the addition of new bisect terms

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> We do need two functions because we pass the pointer as callback, but
> it reads nicer with a third helper function.
>
> diff --git a/revision.c b/revision.c
> index 3ff8723..5cd08e9 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -21,6 +21,9 @@
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> +static const char *name_bad;
> +static const char *name_good;
> +
> ...
> +
> +static int for_each_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data, const char *term) {
> +	struct strbuf bisect_refs = STRBUF_INIT;
> +	int status;
> +	strbuf_addf(&bisect_refs, "refs/bisect/%s", term);
> +	status = for_each_ref_in_submodule(submodule, bisect_refs.buf, fn, cb_data);
> +	strbuf_release(&bisect_refs);
> +	return status;
> +}
> +
>  static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -	return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
> +	return for_each_bisect_ref(submodule, fn, cb_data, "bad");
>  }

Shouldn't this be passing name_bad instead of "bad"?

>  
>  static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>  {
> -	return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
> +	return for_each_bisect_ref(submodule, fn, cb_data, "good");
>  }

Likewise.

Re: [PATCH v10.1 5/7] bisect: simplify the addition of new bisect terms

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

>>  static int for_each_bad_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>>  {
>> -	return for_each_ref_in_submodule(submodule, "refs/bisect/bad", fn, cb_data);
>> +	return for_each_bisect_ref(submodule, fn, cb_data, "bad");
>>  }
>
> Shouldn't this be passing name_bad instead of "bad"?
>
>>  
>>  static int for_each_good_bisect_ref(const char *submodule, each_ref_fn fn, void *cb_data)
>>  {
>> -	return for_each_ref_in_submodule(submodule, "refs/bisect/good", fn, cb_data);
>> +	return for_each_bisect_ref(submodule, fn, cb_data, "good");
>>  }
>
> Likewise.

Indeed. Fixed.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>> + git bisect terms <term-old> <term-new>
>>
>> I think this is the other way around.
>
> Indeed.

I hate to be saying this, but this is a strong indication that
consistency with "start $bad $good..." must be broken.  If the
person who has been working on this topic for a few iterations in
the past few days cannot get it right, no ordinary user can.  With
or without a mnemonic hint "N comes before O, so does B before G".

Of course we cannot just say "git bisect terms old new".  That would
only invite "eh, I do not remember, which between terms and start
take the old one first?" without helping people.

The best I can come up with is to forbid positional arguments to
this subcommand and always require them to be given like so:

	git bisect terms --old=fast --new=slow
	git bisect terms --new=slow --old=fast

We may want to start supporting

	git bisect start --new=master --old=maint

while at it and then gently nudging people to stop using

	git bisect start master maint

by showing depreation notice.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Christian Couder]

On Sat, Jun 27, 2015 at 12:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>>
>>>> + git bisect terms <term-old> <term-new>
>>>
>>> I think this is the other way around.
>>
>> Indeed.
>
> I hate to be saying this, but this is a strong indication that
> consistency with "start $bad $good..." must be broken.  If the
> person who has been working on this topic for a few iterations in
> the past few days cannot get it right, no ordinary user can.  With
> or without a mnemonic hint "N comes before O, so does B before G".
>
> Of course we cannot just say "git bisect terms old new".  That would
> only invite "eh, I do not remember, which between terms and start
> take the old one first?" without helping people.
>
> The best I can come up with is to forbid positional arguments to
> this subcommand and always require them to be given like so:
>
>         git bisect terms --old=fast --new=slow
>         git bisect terms --new=slow --old=fast

If we don't want to support positional arguments, then I would suggest
supporting first the following instead:

         git bisect terms --name-good=fast --name-bad=slow
         git bisect terms --name-bad=slow --name-good=fast

This would make the interface consistent with the code.

Of course we could also accept --name-old and --name-new as synonyms
for --name-good and --name-bad.

> We may want to start supporting
>
>         git bisect start --new=master --old=maint

Maybe we could also support:

git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
<christian.couder@gmail.com> wrote:
>
> If we don't want to support positional arguments, then I would suggest
> supporting first the following instead:
>
>          git bisect terms --name-good=fast --name-bad=slow
>          git bisect terms --name-bad=slow --name-good=fast
>
> This would make the interface consistent with the code.

Which somewhat defeats the point of introducing "old" and "new", though.
The "terms" support is for people who feel that good/bad would be too confusing
for the particular bisect session (e.g. because they are hunting for a fix).

>> We may want to start supporting
>>
>>         git bisect start --new=master --old=maint
>
> Maybe we could also support:
>
> git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master

The same comment for the token after --name-, but allowing the terms to be set
at "start" could be a type-saver.  With need for added "--name-"
prefix (worse, twice),
I am not sure if it would be seen as a useful type-saver, though.

Thanks.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Christian Couder]

On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> If we don't want to support positional arguments, then I would suggest
>> supporting first the following instead:
>>
>>          git bisect terms --name-good=fast --name-bad=slow
>>          git bisect terms --name-bad=slow --name-good=fast
>>
>> This would make the interface consistent with the code.
>
> Which somewhat defeats the point of introducing "old" and "new", though.
> The "terms" support is for people who feel that good/bad would be too confusing
> for the particular bisect session (e.g. because they are hunting for a fix).

Well if --name-old and --name-new are also available as synonyms, it
would not be too bad I think.
People could use the option names that fit their mental model or their
use case better.

>>> We may want to start supporting
>>>
>>>         git bisect start --new=master --old=maint
>>
>> Maybe we could also support:
>>
>> git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master
>
> The same comment for the token after --name-, but allowing the terms to be set
> at "start" could be a type-saver.  With need for added "--name-"
> prefix (worse, twice),
> I am not sure if it would be seen as a useful type-saver, though.

At least people don't need to remember if they have to use "git bisect
term" before or after starting :-)

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>>
>>> If we don't want to support positional arguments, then I would suggest
>>> supporting first the following instead:
>>>
>>>          git bisect terms --name-good=fast --name-bad=slow
>>>          git bisect terms --name-bad=slow --name-good=fast
>>>
>>> This would make the interface consistent with the code.
>>
>> Which somewhat defeats the point of introducing "old" and "new", though.
>> The "terms" support is for people who feel that good/bad would be too confusing
>> for the particular bisect session (e.g. because they are hunting for a fix).
>
> Well if --name-old and --name-new are also available as synonyms, it
> would not be too bad I think.
> People could use the option names that fit their mental model or their
> use case better.

OK, I'll add both.

>>>> We may want to start supporting
>>>>
>>>>         git bisect start --new=master --old=maint
>>>
>>> Maybe we could also support:
>>>
>>> git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master
>>
>> The same comment for the token after --name-, but allowing the terms to be set
>> at "start" could be a type-saver.  With need for added "--name-"
>> prefix (worse, twice),
>> I am not sure if it would be seen as a useful type-saver, though.
>
> At least people don't need to remember if they have to use "git bisect
> term" before or after starting :-)

OK, first lesson learnt: the UI is controversial. So, I'm working on a
series that splits the last patch into several preparation steps, and a
very simple patch to implement the UI.

I have a draft here https://github.com/moy/git/commits/bisect-terms
I'll go through it once more and send it later.

As a teaser, the patch implementing the UI is just:

--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -70,6 +70,26 @@ bisect_autostart() {
 	}
 }
 
+parse_name_args() {
+	while [ $# -gt 0 ]; do
+		arg="$1"
+		case "$arg" in
+		--name-good|--name-old)
+			shift
+			must_write_terms=1
+			NAME_GOOD=$1
+			shift ;;
+		--name-bad|--name-new)
+			shift
+			must_write_terms=1
+			NAME_BAD=$1
+			shift ;;
+		*)
+			shift ;;
+		esac
+	done
+}
+
 bisect_start() {
 	#
 	# Check for one bad and then some good revisions.
@@ -88,6 +108,9 @@ bisect_start() {
 	else
 		mode=''
 	fi
+	# Parse --name-* options before the other to allow any mix of
+	# --name-* and revisions on the command-line.
+	parse_name_args "$@"
 	while [ $# -gt 0 ]; do
 		arg="$1"
 		case "$arg" in
@@ -98,6 +121,8 @@ bisect_start() {
 		--no-checkout)
 			mode=--no-checkout
 			shift ;;
+		--name-good|--name-old|--name-bad|--name-new)
+			shift 2 ;;
 		--*)
 			die "$(eval_gettext "unrecognised option: '\$arg'")" ;;
 		*)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
>>> <christian.couder@gmail.com> wrote:
>>>>
>>>> If we don't want to support positional arguments, then I would suggest
>>>> supporting first the following instead:
>>>>
>>>>          git bisect terms --name-good=fast --name-bad=slow
>>>>          git bisect terms --name-bad=slow --name-good=fast
>>>>
>>>> This would make the interface consistent with the code.
>>>
>>> Which somewhat defeats the point of introducing "old" and "new", though.
>>> The "terms" support is for people who feel that good/bad would be too confusing
>>> for the particular bisect session (e.g. because they are hunting for a fix).
>>
>> Well if --name-old and --name-new are also available as synonyms, it
>> would not be too bad I think.
>> People could use the option names that fit their mental model or their
>> use case better.
>
> OK, I'll add both.

I moderately hate to see both from aesthetics point of view, but can
we at least lose "--name-" prefix?  Having to repeat that twice does
not add any value to the subcommand.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Michael Haggerty]

On 06/27/2015 06:25 AM, Junio C Hamano wrote:
> On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> If we don't want to support positional arguments, then I would suggest
>> supporting first the following instead:
>>
>>          git bisect terms --name-good=fast --name-bad=slow
>>          git bisect terms --name-bad=slow --name-good=fast
>>
>> This would make the interface consistent with the code.
> 
> Which somewhat defeats the point of introducing "old" and "new", though.
> The "terms" support is for people who feel that good/bad would be too confusing
> for the particular bisect session (e.g. because they are hunting for a fix).
> 
>>> We may want to start supporting
>>>
>>>         git bisect start --new=master --old=maint
>>
>> Maybe we could also support:
>>
>> git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master
> 
> The same comment for the token after --name-, but allowing the terms to be set
> at "start" could be a type-saver.  With need for added "--name-"
> prefix (worse, twice),
> I am not sure if it would be seen as a useful type-saver, though.

I would like to remind everybody of my old claim that it would be
possible to teach `git bisect` to infer by itself which term means
"older" and which term means "newer":

    http://article.gmane.org/gmane.comp.version-control.git/244036

I think that making `bisect` smarter could make the UI simpler, though
admittedly it would be more work than the current proposal.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> I would like to remind everybody of my old claim that it would be
> possible to teach `git bisect` to infer by itself which term means
> "older" and which term means "newer":
>
>     http://article.gmane.org/gmane.comp.version-control.git/244036

But then one mistake at the beginning and the user will be on a wrong
track during the whole bisect session, no? Unless you make absolutely
clear when making the "intelligent" decision what Git inferred, that is.

For something complex like bisect, I highly suspect that a tool that is
more intelligent than the end users (more precisely, a tool that it thinks
it is more intelligent) would hurt them more than it helps them.

Of course, that is only my claim ;-)

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Michael Haggerty]

On 06/28/2015 08:15 AM, Junio C Hamano wrote:
> On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>
>> I would like to remind everybody of my old claim that it would be
>> possible to teach `git bisect` to infer by itself which term means
>> "older" and which term means "newer":
>>
>>     http://article.gmane.org/gmane.comp.version-control.git/244036
> 
> But then one mistake at the beginning and the user will be on a wrong
> track during the whole bisect session, no? Unless you make absolutely
> clear when making the "intelligent" decision what Git inferred, that is.

Definitely, `git bisect` should tell the user what it inferred.

> For something complex like bisect, I highly suspect that a tool that is
> more intelligent than the end users (more precisely, a tool that it thinks
> it is more intelligent) would hurt them more than it helps them.

This isn't about making bisect "more intelligent than the end users". It
is about not forcing the user cumbersomely to spell out redundant
information because the tool is too stupid.

If I mark one commit "broken" and another commit "fixed", and the
"broken" commit is an ancestor of the "fixed" commit, then it is pretty
obvious that I am looking for the commit that caused the transition
"broken" -> "fixed". The same if I mark one commit "xyzzy" and the other
one "plugh".

I understand that the user might make a mistake when marking the initial
commits, but as soon as bisect says

    Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
    will look for the commit that caused the transition from
    "xyzzy" to "plugh".

then I hope the user will notice and correct her/his mistake.

For example, a session could be started with

    git bisect start --mark=broken <committish> --mark=fixed <committish>

and from then on

    git bisect broken
    git bisect fixed

Or, if the user doesn't want to specify both endpoints on the `start` line,

    git bisect start
    git bisect --mark=broken [<committish>]
    git bisect --mark=fixed [<committish>]

Essentially, specifying `--mark=<name>` once would make `<name>` a
shorthand for `--mark=<name>`.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I understand that the user might make a mistake when marking the initial
> commits, but as soon as bisect says
>
>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>     will look for the commit that caused the transition from
>     "xyzzy" to "plugh".
>
> then I hope the user will notice and correct her/his mistake.
>
> For example, a session could be started with
>
>     git bisect start --mark=broken <committish> --mark=fixed <committish>

Interesting.

If we extend that line of thought further, maybe we do not even need
to add new/old, fixed/broken, or slow/fast.

You just _always_ say "good" or "bad".  If something is slow, you
say "bad" and if something is fast, you say "good".

If you start "git bisect start maint master" (and as always, "bad"
comes before "good") because some operation is very slow in maint
but somehow is usably fast in master, you are automatically hunting
for a performance fix that you can cherry-pick to the maintenance
track.

No need for "bisect new", "bisect old", or "bisect terms", let alone
"bisect terms --new=fast --old=slow".  The tool just does the right
thing because it already has the information necessary to infer what
the user means by 'good' and 'bad', and the initial topology determines
which transition, either from 'good' to 'bad' or from 'bad' to 'good',
the user is hunting for.

I really like that simplicity.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Michael Haggerty]

On 06/28/2015 09:32 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I understand that the user might make a mistake when marking the initial
>> commits, but as soon as bisect says
>>
>>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>>     will look for the commit that caused the transition from
>>     "xyzzy" to "plugh".
>>
>> then I hope the user will notice and correct her/his mistake.
>>
>> For example, a session could be started with
>>
>>     git bisect start --mark=broken <committish> --mark=fixed <committish>
> 
> Interesting.
> 
> If we extend that line of thought further, maybe we do not even need
> to add new/old, fixed/broken, or slow/fast.
> 
> You just _always_ say "good" or "bad".  If something is slow, you
> say "bad" and if something is fast, you say "good".

Yes, I think "good" and "bad" would usually be perfectly intuitive and
would almost always be usable.

> [...]
> No need for "bisect new", "bisect old", or "bisect terms", let alone
> "bisect terms --new=fast --old=slow".  The tool just does the right
> thing because it already has the information necessary to infer what
> the user means by 'good' and 'bad', and the initial topology determines
> which transition, either from 'good' to 'bad' or from 'bad' to 'good',
> the user is hunting for.

Correct. The only caveat is if the initial "good" and "bad" commits are
not ancestrally related to each other. But in this case, I think
"bisect" asks the user to test a merge base anyway, and once that one
has been tested it will be clear which of the labels comes "before" the
other.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 06/28/2015 09:32 AM, Junio C Hamano wrote:
>> 
>> You just _always_ say "good" or "bad".  If something is slow, you
>> say "bad" and if something is fast, you say "good".
>
> Yes, I think "good" and "bad" would usually be perfectly intuitive and
> would almost always be usable.
>
>> [...]
>> No need for "bisect new", "bisect old", or "bisect terms", let alone
>> "bisect terms --new=fast --old=slow".  The tool just does the right
>> thing because it already has the information necessary to infer what
>> the user means by 'good' and 'bad', and the initial topology determines
>> which transition, either from 'good' to 'bad' or from 'bad' to 'good',
>> the user is hunting for.
>
> Correct. The only caveat is if the initial "good" and "bad" commits are
> not ancestrally related to each other. But in this case, I think
> "bisect" asks the user to test a merge base anyway, and once that one
> has been tested it will be clear which of the labels comes "before" the
> other.

The more I look at the proposal, the more I like it.  The old way of
thinking is that we need to keep 'bad' for newer one and 'good' for
older one, that required us to invent 'broken' vs 'fixed', or value
neutral 'old' vs 'new'.  Then we extend it to a random pair of
'terms', but we reserve 'good', 'bad', etc. and do not allow the
user to say "old was bad, new is now good".  With your proposal, the
user can just say "oh this is good", vs "oh this is bad".  The
mental model becomes much simpler.

I _think_ bulk of Antoine and Matthieu's work can be salvaged/reused
to implement the proposal, but now it would be more clear that
$name_good and $name_bad is a bad way to name internal variables and
files in $GIT_DIR.  The inferred 'ah you are hunting for regression'
mode would call old ones 'bad' and new ones 'good', they have to be
given value neutral names, e.g. $name_old and $name_new.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Christian Couder]

On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 06/28/2015 08:15 AM, Junio C Hamano wrote:
>> On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>>
>>> I would like to remind everybody of my old claim that it would be
>>> possible to teach `git bisect` to infer by itself which term means
>>> "older" and which term means "newer":
>>>
>>>     http://article.gmane.org/gmane.comp.version-control.git/244036
>>
>> But then one mistake at the beginning and the user will be on a wrong
>> track during the whole bisect session, no? Unless you make absolutely
>> clear when making the "intelligent" decision what Git inferred, that is.
>
> Definitely, `git bisect` should tell the user what it inferred.
>
>> For something complex like bisect, I highly suspect that a tool that is
>> more intelligent than the end users (more precisely, a tool that it thinks
>> it is more intelligent) would hurt them more than it helps them.
>
> This isn't about making bisect "more intelligent than the end users". It
> is about not forcing the user cumbersomely to spell out redundant
> information because the tool is too stupid.
>
> If I mark one commit "broken" and another commit "fixed", and the
> "broken" commit is an ancestor of the "fixed" commit, then it is pretty
> obvious that I am looking for the commit that caused the transition
> "broken" -> "fixed". The same if I mark one commit "xyzzy" and the other
> one "plugh".
>
> I understand that the user might make a mistake when marking the initial
> commits, but as soon as bisect says
>
>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>     will look for the commit that caused the transition from
>     "xyzzy" to "plugh".
>
> then I hope the user will notice and correct her/his mistake.

This looks fragile to me. Unfortunately many users will probably not
read it and continue, and then spend a lot of time later trying to
understand what went wrong, not remembering about the message at all.

The message looks like an informative message. At least we should add
something like "Please check that it is what you want to do and abort
with 'git bisect reset' if it is not."

> For example, a session could be started with
>
>     git bisect start --mark=broken <committish> --mark=fixed <committish>

This look nearly the same as:

git bisect start --name-old=broken --broken=<committish>
--name-new=fixed --fixed=<committish>

except that it looks safer and more backward compatible to me with
--name-old and --name-new.

By the way we could use "mark" or "term" instead of "name" in the
option name (like --mark-old or --term-old) and in the code too if it
looks clearer.

> and from then on
>
>     git bisect broken
>     git bisect fixed
>
> Or, if the user doesn't want to specify both endpoints on the `start` line,
>
>     git bisect start
>     git bisect --mark=broken [<committish>]
>     git bisect --mark=fixed [<committish>]

We could do that too with:

     git bisect start
     git bisect --name-old=broken broken [<committish>]
     git bisect --name-new=fixed fixed [<committish>]

and/or:

     git bisect start
     git bisect --name-old=broken --broken=[<committish>]
     git bisect --name-new=fixed --fixed=[<committish>]

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I _think_ bulk of Antoine and Matthieu's work can be salvaged/reused
> to implement the proposal,

I'm obviously biaised since I already spent time on the "bisect terms"
idea, and I would hate to see my work and Antoine & Louis' thrown away.
But I have to admit that I do like the idea of "git bisect" figuring out
which commit is the old and which commit is the new one.

It's much easier to implement after the series. I'm currently forbidding
redefining "good" as "bad" and vice versa, but that's just to avoid
confusion and because I didn't test this case, but it should just work.
So, essentially an implementation of this "guess who's old and who's
new" could be: if we find a good commit that is an ancestor of an old
commit, swap the terms in BISECT_TERMS. When this happens before we
started to set any refs, this should do the trick. In general, we should
rename the good-$sha1 reference to good and the bad to bad-$sha1 (there
are corner-cases where the user already specified several good-$sha1
refs, in which case we would need to discard some of them).

I'm getting out of Git time budget, so I can't be the one doing this, at
least not soon.

So, one option is to take the series as-is, and wait for someone to
implement the "guess who's old and who's new" later on top of it. One
drawback would be that we'd end up having the not-so-useful feature
"bisect terms" in the user-interface. At least, I am now convinced that
hardcoding the pair old/new is not needed. In the short term, we can
have "git bisect start --name-old foo --name-new bar" which avoids the
"One needs to remember in which order to give terms" issue we used to
have, so we don't need to clutter the user-interface with many ways to
do the same thing. OTOH, the "bisect terms" feature wouldn't be
completely useless: not everything is good or bad, so leaving the option
to the user to tag "slow/fast", "present/absent", ... still makes sense.

So, my proposal would be to remove the "old/new" patch from the series,
and keep the other patches.

What do you think?

> but now it would be more clear that $name_good and $name_bad is a bad
> way to name internal variables and files in $GIT_DIR. The inferred 'ah
> you are hunting for regression' mode would call old ones 'bad' and new
> ones 'good', they have to be given value neutral names, e.g. $name_old
> and $name_new.

Ideally, the whole code would be ported to use old/new, but the more I
read the code the more I agree with Christian actually: we still have
many more instances of good/bad in the code (e.g. functions
check_good_are_ancestors_of_bad, for_each_good_bisect_ref, ...), so
having just name_new/name_old would make the code very inconsistant.
It's easier to read the code thinking "good revs are old, bad revs are
recent; maybe some magic swapped the terms but I don't need to worry
about this" for now.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> I understand that the user might make a mistake when marking the initial
>> commits, but as soon as bisect says
>>
>>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>>     will look for the commit that caused the transition from
>>     "xyzzy" to "plugh".
>>
>> then I hope the user will notice and correct her/his mistake.
>
> This looks fragile to me. Unfortunately many users will probably not
> read it and continue, and then spend a lot of time later trying to
> understand what went wrong,

I don't understand what you mean by "went wrong". As a user, when I
discovered "git bisect", I was actually surprised that it expected one
particular order between good and bad. I would have expected to be able
to say "this is good, this is bad, tell me where it changed" without
having an idea of who's good and who's bad. In particular when bisecting
from two branches, the user knows that branch A is good, and branch B is
bad, but does not necessarily know whether it's a regression in B or a
fix in A. The fact that bisect can find out should be just "normal" from
the user point of view. There's no mistake involved, nothing to fix, and
nothing that went wrong.

> By the way we could use "mark" or "term" instead of "name" in the
> option name (like --mark-old or --term-old) and in the code too if it
> looks clearer.

I prefer "term" to "mark" because "mark" is both a verb and a noun, so
--mark-old=foo could mean both "mark foo as old" or "the name of the
marks for old commits is foo".

I think I prefer "term" to "name".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Christian Couder]

On Mon, Jun 29, 2015 at 9:34 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> I understand that the user might make a mistake when marking the initial
>>> commits, but as soon as bisect says
>>>
>>>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>>>     will look for the commit that caused the transition from
>>>     "xyzzy" to "plugh".
>>>
>>> then I hope the user will notice and correct her/his mistake.
>>
>> This looks fragile to me. Unfortunately many users will probably not
>> read it and continue, and then spend a lot of time later trying to
>> understand what went wrong,
>
> I don't understand what you mean by "went wrong".

It happens that users mistake the "good" and the "bad" commits when
giving them to git bisect.

Right now in the most common case, we can error out because we know
that a "bad" commit cannot be an ancestor of a "good" commit.

> As a user, when I
> discovered "git bisect", I was actually surprised that it expected one
> particular order between good and bad. I would have expected to be able
> to say "this is good, this is bad, tell me where it changed" without
> having an idea of who's good and who's bad.

Maybe, but it's not how it has been developed.

> In particular when bisecting
> from two branches, the user knows that branch A is good, and branch B is
> bad, but does not necessarily know whether it's a regression in B or a
> fix in A. The fact that bisect can find out should be just "normal" from
> the user point of view. There's no mistake involved, nothing to fix, and
> nothing that went wrong.

Well in this case, it's possible that the merge base is bad and what
the user is interested in is the first bad commit that was commited
before the merge base. We just don't know, in the case the merge base
is bad, what is more interesting for the user.

So I disagree with you and Michael that we should decide that the user
is interested by the fix in this case. It's better to error out like
we do now and let the user decide what he/she wants rather than decide
for him/her that he/she is interested by the fix.

>> By the way we could use "mark" or "term" instead of "name" in the
>> option name (like --mark-old or --term-old) and in the code too if it
>> looks clearer.
>
> I prefer "term" to "mark" because "mark" is both a verb and a noun, so
> --mark-old=foo could mean both "mark foo as old" or "the name of the
> marks for old commits is foo".
>
> I think I prefer "term" to "name".

Ok with that. I agree that it would be more consistent to have a "git
bisect terms" and "--term-{old,new,bad,good}".

Thanks,
Christian.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Jun 29, 2015 at 9:34 AM, Matthieu Moy
>
>> As a user, when I
>> discovered "git bisect", I was actually surprised that it expected one
>> particular order between good and bad. I would have expected to be able
>> to say "this is good, this is bad, tell me where it changed" without
>> having an idea of who's good and who's bad.
>
> Maybe, but it's not how it has been developed.

... but it's how it would behave if we had this guessing feature. The
user does not have to care whether we internally need "good is an
ancestor of bad" if we can provide a user-interface which does not need
that.

I find it particularly frustrating that we issue the message:

  "The merge base %s is bad.\n"
  "This means the bug has been fixed "
  "between %s and [%s].\n"

bisect is all about finding the commit where a property has changed, and
here it stops, saying "I know it's between A and B, but I won't go
further".

>> In particular when bisecting from two branches, the user knows that
>> branch A is good, and branch B is bad, but does not necessarily know
>> whether it's a regression in B or a
>> fix in A. The fact that bisect can find out should be just "normal" from
>> the user point of view. There's no mistake involved, nothing to fix, and
>> nothing that went wrong.
>
> Well in this case, it's possible that the merge base is bad and what
> the user is interested in is the first bad commit that was commited
> before the merge base. We just don't know, in the case the merge base
> is bad, what is more interesting for the user.

The question asked by the user is "X is good, Y is bad, tell me where
exactly it changed". We can't know for sure what is "interesting" for
the user, but we can answer his question anyway.

Similarly, there can be several commits that introduce the same
regression (this may happen if you cherry picked the guilty commit from
branch A to branch B, and then merged A and B, or if you broke, fixed,
and then broke again). bisect finds one transition from good to bad, but
not all. It may or may not be the one the user wanted, but we can't
know.

>> I think I prefer "term" to "name".
>
> Ok with that. I agree that it would be more consistent to have a "git
> bisect terms" and "--term-{old,new,bad,good}".

OK, I've applied it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> I moderately hate to see both from aesthetics point of view, but can
> we at least lose "--name-" prefix?

I changed it to --term- prefix, but I'd rather not drop it. When reading
"--old=foo", it is not clear to me whether the meaning should be "the
term used for old is foo" or "mark foo as old". The longer version does
not have this problem.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Christian Couder]

On Mon, Jun 29, 2015 at 11:32 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:

[...]

> I find it particularly frustrating that we issue the message:
>
>   "The merge base %s is bad.\n"
>   "This means the bug has been fixed "
>   "between %s and [%s].\n"

I find it a good safety feature.

> bisect is all about finding the commit where a property has changed,

That is your interpretation of this command. On the man page there is:

    git-bisect - Find by binary search the change that introduced a bug

So its stated purpose is to find the first "bad" commit. Not to find a fix.

This doesn't mean that we cannot or should not make it possible to
find a fix, but we shoudn't try to find a fix when there is doubt
about whether the user wants to find a fix or the first bad commit.

> and
> here it stops, saying "I know it's between A and B, but I won't go
> further".

You can interpret it that way, but my interpretation of this is that
the real reason why we stop is because we don't know what the users
really wants to do, find the fix, or really find the first bad commit.

>>> In particular when bisecting from two branches, the user knows that
>>> branch A is good, and branch B is bad, but does not necessarily know
>>> whether it's a regression in B or a
>>> fix in A. The fact that bisect can find out should be just "normal" from
>>> the user point of view. There's no mistake involved, nothing to fix, and
>>> nothing that went wrong.
>>
>> Well in this case, it's possible that the merge base is bad and what
>> the user is interested in is the first bad commit that was commited
>> before the merge base. We just don't know, in the case the merge base
>> is bad, what is more interesting for the user.
>
> The question asked by the user is "X is good, Y is bad, tell me where
> exactly it changed".

That's your interpretation of the question asked by the user.
It can be interpreted otherwise.

> We can't know for sure what is "interesting" for
> the user, but we can answer his question anyway.

You can answer your interpretation of the question, yes, but this
means taking risks that we don't need to take in my opinion.

If people really want it, we can still have an option called for
example --find-fix that could automatically try to find the fix when
the merge base is "bad" without displaying the message that annoys
you. It would make it explicit that it is ok to find a fix.

> Similarly, there can be several commits that introduce the same
> regression (this may happen if you cherry picked the guilty commit from
> branch A to branch B, and then merged A and B, or if you broke, fixed,
> and then broke again). bisect finds one transition from good to bad, but
> not all. It may or may not be the one the user wanted, but we can't
> know.

Yeah, but this is different as we would still find a first "bad"
commit, not a fix.

>>> I think I prefer "term" to "name".
>>
>> Ok with that. I agree that it would be more consistent to have a "git
>> bisect terms" and "--term-{old,new,bad,good}".
>
> OK, I've applied it.

Great!

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Jun 29, 2015 at 11:32 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> bisect is all about finding the commit where a property has changed,
>
> That is your interpretation of this command. On the man page there is:
>
>     git-bisect - Find by binary search the change that introduced a bug
>
> So its stated purpose is to find the first "bad" commit. Not to find a fix.

This is a limitation of the current bisect, but the discussion is
precisely about removing this limitation.

I still don't understand what "risk" we are taking by doing the
bisection anyway. I can't imagine a case where we would harm the user by
doing so.

I just tested with Mercurial, and looking for a fix instead of a
regression just works:

$ hg bisect --good 4        
$ hg bisect --bad 1         
Testing changeset 2:d75a2d042c99 (3 changesets remaining, ~1 tests)
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg bisect --bad  
Testing changeset 3:9d27d9c02e28 (2 changesets remaining, ~1 tests)
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg bisect --bad
The first good revision is:
changeset:   4:1dd9bb959eb6
tag:         tip
user:        Matthieu Moy <Matthieu.Moy@imag.fr>
date:        Mon Jun 29 17:07:51 2015 +0200
summary:     foo

I don't see anything wrong with this.

(OTOH, "hg bisect" does not accept revisions which aren't parent of each
other)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I moderately hate to see both from aesthetics point of view, but can
>> we at least lose "--name-" prefix?
>
> I changed it to --term- prefix, but I'd rather not drop it. When reading
> "--old=foo", it is not clear to me whether the meaning should be "the
> term used for old is foo" or "mark foo as old". The longer version does
> not have this problem.

Yeah, my suggestion was based on one assumption I did not mention,
which is that we do not need to crowd "bisect start" with this
option when we have "bisect terms", as long as "bisect start" does
not have to take both "what are the terms" and "which commits are
painted using which one of the two terms" on its command line, the
"--name-" prefix was unnecessary.

But if you are dropping "bisect terms" and allowing the terms
specified only from "bisect start" as you mentioned in the cover
letter of v11, that changes the equation.  And I think "start is the
place that sets up a clean slate, and that is the only place where
you can optionally declare your custom terms" is a very sensible
design.  I do not have a problem with "--terms" prefix in that case.

Thanks.

Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> So, my proposal would be to remove the "old/new" patch from the series,
> and keep the other patches.
>
> What do you think?

Let me answer after reading v11 through.

>> but now it would be more clear that $name_good and $name_bad is a bad
>> way to name internal variables and files in $GIT_DIR. The inferred 'ah
>> you are hunting for regression' mode would call old ones 'bad' and new
>> ones 'good', they have to be given value neutral names, e.g. $name_old
>> and $name_new.
>
> Ideally, the whole code would be ported to use old/new, but the more I
> read the code the more I agree with Christian actually: we still have
> many more instances of good/bad in the code (e.g. functions
> check_good_are_ancestors_of_bad, for_each_good_bisect_ref, ...), so
> having just name_new/name_old would make the code very inconsistant.

Oh, no question about that.  I was hoping that we would at least get
the concensus that we should move all to old/new and these good/bad
in code no longer make sense.

It just was that introducing new variables and functions whose names
follow the convention that reflects the world view that is no longer
valid (i.e. good is always old and bad is always new) in a series
that introduces the new world view somehow felt wrong.