[PATCH 1/4] bisect : correction of typo

[PATCH 1/4] bisect : correction of typo

[Antoine Delaite]

---
 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 10f5e57..de92953 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.1.414.ge7a9de3.dirty

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

[Antoine Delaite]

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@grenoble-inp.fr>
---
 git-bisect.sh | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
 mode change 100755 => 100644 git-bisect.sh

diff --git a/git-bisect.sh b/git-bisect.sh
old mode 100755
new mode 100644
index ae3fec2..1f16aaf
--- 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()
 {
@@ -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 "$(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
+		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,15 +321,15 @@ 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"
 		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"
@@ -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.1.414.ge7a9de3.dirty

Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

[Junio C Hamano]

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

> +	*,"$NAME_BAD")
> +		die "$(gettext "'git bisect $NAME_BAD' can take only one argument.")" ;;

Hmmmm, doesn't this break i18n the big way?

Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

[Matthieu Moy]

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

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

I would have written

NAME_NEW=bad
NAME_OLD=good

"old/new" are the generic wording, so I think it would make more sense
for the codebase to use it when we don't hardcode old/new.

(and the double-quotes are not needed)

> @@ -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 "$(gettext "'git bisect $NAME_BAD' can take only one argument.")" ;;

As Junio mentionned, you'll need an eval_gettext here.

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

Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

[Christian Couder]

On Tue, Jun 9, 2015 at 8:45 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
>
>> --- 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"
>
> I would have written
>
> NAME_NEW=bad
> NAME_OLD=good
>
> "old/new" are the generic wording, so I think it would make more sense
> for the codebase to use it when we don't hardcode old/new.

I don't agree with NAME_NEW and NAME_OLD instead of NAME_BAD and
NAME_OLD, for me it is easier when reasonning about the code to always
think as if we want to find a bug. This is especially true when
thinking about cases when we are given a "good" commit that is not an
ancestor of the "bad" commit (and we have to find the merge base and
so on), because in this case the "good" commit might be newer than the
"bad" commit.

"old/new" is not more generic than "good/bad". It just has a different
kind of drawbacks, and as "good/bad" is older and is the default we
should keep that in the names.

Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

[Matthieu Moy]

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

> "old/new" is not more generic than "good/bad".

I disagree with this. In any case, we're looking for a pair of commits
where one is a direct parent of the other. So in the end, there's always
the old behavior and the new behavior in the end.

In natural language, I can write "terms good/bad correspond to the
situation where the new behavior is a bug and the old behavior was
correct" and "terms fixed/unfixed correspond to the situation where the
new behavior does not have a bug and the old one does", so I can
describe several pairs of terms with old/new. When looking for a bugfix,
saying "NAME_GOOD=new" seems backward. I would read this as "the good
behavior is to be new", while I would expect "the new behavior is to be
good".

> and as "good/bad" is older and is the default we should keep that in
> the names.

I agree with this part though. If people working with the bisect
codebase (which includes you) are more comfortable with good/bad, that's
a valid reason to keep it.

IOW, I still think old/new is more generic, but that is not a strong
objection and should not block the patch.

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

Re: [PATCH 2/4] bisect: replace hardcoded "bad|good" by variables

[Junio C Hamano]

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

> Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
>
>> --- 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"
>
> I would have written
>
> NAME_NEW=bad
> NAME_OLD=good
>
> "old/new" are the generic wording, so I think it would make more sense
> for the codebase to use it when we don't hardcode old/new.

Yeah, I would think so, especially if we envision that the new/old
will not be the only pair we will ever allow in place for the
traditional bad/good.  Being bad is just a special case of being new
only when you are hunting for a regression.

[PATCH 3/4] bisect: simplify the add of new bisect terms

[Antoine Delaite]

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
In bisect.c :
	handle_bad_merge_base

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@grenoble-inp.fr>
---
 bisect.c      | 65 ++++++++++++++++++++++++++++++++++++++++++++---------------
 git-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 103 insertions(+), 20 deletions(-)

diff --git a/bisect.c b/bisect.c
index de92953..3b7df85 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,7 +406,7 @@ struct commit_list *find_bisection(struct commit_list *list,
 static int register_ref(const char *refname, const unsigned char *sha1,
 			int flags, void *cb_data)
 {
-	if (!strcmp(refname, "bad")) {
+	if (!strcmp(refname, name_bad)) {
 		current_bad_oid = xmalloc(sizeof(*current_bad_oid));
 		hashcpy(current_bad_oid->hash, sha1);
 	} else if (starts_with(refname, "good-")) {
@@ -634,7 +637,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 +735,19 @@ 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")) {
+			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);
+		}
 		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 +759,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 +843,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))
@@ -890,6 +894,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 }
 
 /*
+ * The terms used for this bisect session are stocked in
+ * BISECT_TERMS: it can be bad/good or new/old.
+ * We read them and stock them to adapt the messages
+ * accordingly. Default is bad/good.
+ */
+void read_bisect_terms(void)
+{
+	struct strbuf str = STRBUF_INIT;
+	const char *filename = git_path("BISECT_TERMS");
+	FILE *fp = fopen(filename, "r");
+
+	if (!fp) {
+		name_bad = "bad";
+		name_good = "good";
+	} else {
+		strbuf_getline(&str, fp, '\n');
+		name_bad = strbuf_detach(&str, NULL);
+		strbuf_getline(&str, fp, '\n');
+		name_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.
@@ -905,6 +934,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	const unsigned char *bisect_rev;
 	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
+	read_bisect_terms();
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
@@ -926,8 +956,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 +974,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 1f16aaf..529bb43 100644
--- 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=''
+	start_bad_good=0
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -101,6 +102,9 @@ bisect_start() {
 				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
 				break
 			}
+
+			start_bad_good=1
+
 			case $bad_seen in
 			0) state='bad' ; bad_seen=1 ;;
 			*) state='good' ;;
@@ -172,6 +176,11 @@ bisect_start() {
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
+	if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"
+	then
+		echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" &&
+		echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS"
+	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
 	# Check if we can proceed to the next bisect state.
@@ -232,6 +241,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.")" ;;
@@ -294,12 +304,12 @@ bisect_next_check() {
 
 		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
+			gettextln "You need to give me at least one $(bisect_voc bad) and one $(bisect_voc good) revision.
+(You can use \"git bisect $(bisect_voc bad)\" and \"git bisect $(bisect_voc good)\" 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
+You then need to give me at least one $(bisect_voc good) and one $(bisect_voc bad) revision.
+(You can use \"git bisect $(bisect_voc bad)\" and \"git bisect $(bisect_voc good)\" for that.)" >&2
 		fi
 		exit 1 ;;
 	esac
@@ -402,6 +412,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,6 +433,8 @@ bisect_replay () {
 			rev="$command"
 			command="$bisect"
 		fi
+		get_terms
+		check_and_set_terms "$command"
 		case "$command" in
 		start)
 			cmd="bisect_start $rev"
@@ -499,11 +512,48 @@ bisect_log () {
 	cat "$GIT_DIR/BISECT_LOG"
 }
 
+get_terms () {
+	if test -s "$GIT_DIR/BISECT_TERMS"
+	then
+		NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
+		NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
+	fi
+}
+
+check_and_set_terms () {
+	cmd="$1"
+	case "$cmd" in
+	bad|good)
+		if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$NAME_BAD" -a "$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
+				echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
+				echo "good" >>"$GIT_DIR/BISECT_TERMS"
+			fi
+			NAME_BAD="bad"
+			NAME_GOOD="good" ;;
+		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)
-- 
2.4.1.414.ge7a9de3.dirty

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Junio C Hamano]

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

> 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
> In bisect.c :
> 	handle_bad_merge_base
>
> 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@grenoble-inp.fr>
> ---

This step seems very straight-forward and makes sense from a cursory
look.

>  /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages
> + * accordingly. Default is bad/good.
> + */

s/stock/store/ perhaps?  I think the idea is not to have this file
in the default case so that absence of it would mean you would be
looking for a transition from (older) good to (more recent) bad.

> +void read_bisect_terms(void)
> +{
> +	struct strbuf str = STRBUF_INIT;
> +	const char *filename = git_path("BISECT_TERMS");
> +	FILE *fp = fopen(filename, "r");
> +
> +	if (!fp) {

We might want to see why fopen() failed here.  If it is because the
file did not exist, great.  But otherwise?

> diff --git a/git-bisect.sh b/git-bisect.sh
> index 1f16aaf..529bb43 100644
> --- 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=''
> +	start_bad_good=0
>  	if test "z$(git rev-parse --is-bare-repository)" != zfalse
>  	then
>  		mode=--no-checkout
> @@ -101,6 +102,9 @@ bisect_start() {
>  				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
>  				break
>  			}
> +
> +			start_bad_good=1
> +

It is unclear what this variable means, or what it means to have
zero or one as its value.

>  			case $bad_seen in
>  			0) state='bad' ; bad_seen=1 ;;
>  			*) state='good' ;;
> @@ -172,6 +176,11 @@ bisect_start() {
>  	} &&
>  	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
>  	eval "$eval true" &&
> +	if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"

Avoid "test <condition1> -a <condition2>" (or "-o").

> +get_terms () {
> +	if test -s "$GIT_DIR/BISECT_TERMS"
> +	then
> +		NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
> +		NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"

It is sad that we need to open the file twice.  Can't we do
something using "read" perhaps?

Don't we want to make sure these two names are sensible?  We do not
want an empty-string, for example.  I suspect you do not want to
take anything that check-ref-format does not like.

Same comment applies to the C code.

> +bisect_voc () {
> +	case "$1" in
> +	bad) echo "bad" ;;
> +	good) echo "good" ;;
> +	esac
> +}

What is voc?

What if "$1" is neither bad/good?

Did you mean to translate 'bad' to $NAME_BAD and 'good' to $NAME_GOOD?

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Antoine Delaite]

Hi,

Thanks for the review,

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

>>  /*
>> + * The terms used for this bisect session are stocked in
>> + * BISECT_TERMS: it can be bad/good or new/old.
>> + * We read them and stock them to adapt the messages
>> + * accordingly. Default is bad/good.
>> + */
>
>s/stock/store/ perhaps?  I think the idea is not to have this file
>in the default case so that absence of it would mean you would be
>looking for a transition from (older) good to (more recent) bad.

Yes it is nice but a bisect_terms file is a very useful tool for 
verifications at a little cost. For instance, if the user type this:
git bisect start 
git bisect bad HEAD
git bisect old HEAD~10

We created the bisect_terms file after the bad then the error is
directly detected when the user type git bisect old.
And I think having we should limit the impact of this good/bad
default case as we would prefer old/new.

>> +void read_bisect_terms(void)
>> +{
>> +        struct strbuf str = STRBUF_INIT;
>> +        const char *filename = git_path("BISECT_TERMS");
>> +        FILE *fp = fopen(filename, "r");
>> +
>> +        if (!fp) {
>
>We might want to see why fopen() failed here.  If it is because the
>file did not exist, great.  But otherwise?

Should we display a specific message and cancel the last command?

>> diff --git a/git-bisect.sh b/git-bisect.sh
>> index 1f16aaf..529bb43 100644
>> --- 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=''
>> +        start_bad_good=0
>>          if test "z$(git rev-parse --is-bare-repository)" != zfalse
>>          then
>>                  mode=--no-checkout
>> @@ -101,6 +102,9 @@ bisect_start() {
>>                                  die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
>>                                  break
>>                          }
>> +
>> +                        start_bad_good=1
>> +
>
>It is unclear what this variable means, or what it means to have
>zero or one as its value.

This has been done by our elders and we kept because it was 
useful. We are however trying to remove it. It is in case of 
a 'git bisect start bad_rev good_rev', our function that creates
the bisect_terms is not called. Thus we need to do it manually
in the code.

>> +get_terms () {
>> +        if test -s "$GIT_DIR/BISECT_TERMS"
>> +        then
>> +                NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
>> +                NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
>
>It is sad that we need to open the file twice.  Can't we do
>something using "read" perhaps?

The cost of it is quite low and we see directly what we meant. We didn't 
found a pretty way to read two lines with read.

>Don't we want to make sure these two names are sensible?  We do not
>want an empty-string, for example.  I suspect you do not want to
>take anything that check-ref-format does not like.
>
>Same comment applies to the C code.

Yes, for now only bad/good/old/new are allowed. But in the future
it will be necessary.

>> +bisect_voc () {
>> +        case "$1" in
>> +        bad) echo "bad" ;;
>> +        good) echo "good" ;;
>> +        esac
>> +}
>
>What is voc?
>
>What if "$1" is neither bad/good?
>
>Did you mean to translate 'bad' to $NAME_BAD and 'good' to $NAME_GOOD?

voc stands for vocabulary. 
This fonction is mainly used after to display all the builtins possibility.  It
is only called internally and if the argument is bad it returns the synonyms of
bad (bad|new in the next patch). 

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Matthieu Moy]

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

>>> +get_terms () {
>>> +        if test -s "$GIT_DIR/BISECT_TERMS"
>>> +        then
>>> +                NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
>>> +                NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
>>
>>It is sad that we need to open the file twice.  Can't we do
>>something using "read" perhaps?
>
> The cost of it is quite low and we see directly what we meant. We didn't 
> found a pretty way to read two lines with read.

Should be stg like:

{
	read good
	read bad
} <"$GIT_DIR/BISECT_TERMS"

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

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Matthieu Moy]

> Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

s/add/addition/

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

> +static const char *name_bad;
> +static const char *name_good;

Same remark as PATCH 2.

>  	} else if (starts_with(refname, "good-")) {

Did you forget this one?

> -
> -		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")) {
> +			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);
> +		}

You need an "else" here. Maybe it comes later, but as a reviewer, I want
to check that you did not forget it now (because I don't trust myself to
remember that it must be added later).

> @@ -890,6 +894,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>  }
>  
>  /*
> + * The terms used for this bisect session are stocked in
> + * BISECT_TERMS: it can be bad/good or new/old.
> + * We read them and stock them to adapt the messages

s/stock/store/ (two instances)

> + * accordingly. Default is bad/good.
> + */
> +void read_bisect_terms(void)
> +{
> +	struct strbuf str = STRBUF_INIT;
> +	const char *filename = git_path("BISECT_TERMS");
> +	FILE *fp = fopen(filename, "r");
> +
> +	if (!fp) {

I think you would want to error out if errno is not ENOENT.

> +		name_bad = "bad";
> +		name_good = "good";
> +	} else {
> +		strbuf_getline(&str, fp, '\n');
> +		name_bad = strbuf_detach(&str, NULL);
> +		strbuf_getline(&str, fp, '\n');
> +		name_good = strbuf_detach(&str, NULL);
> +	}

I would have kept just

	name_bad = "bad";
	name_good = "good";

in this patch, and introduce BISECT_TERMS in a separate one.

> --- 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=''
> +	start_bad_good=0
>  	if test "z$(git rev-parse --is-bare-repository)" != zfalse
>  	then
>  		mode=--no-checkout
> @@ -101,6 +102,9 @@ bisect_start() {
>  				die "$(eval_gettext "'\$arg' does not appear to be a valid revision")"
>  				break
>  			}
> +
> +			start_bad_good=1
> +

Why do you need this variable? It seems to me that you are hardcoding
once more that terms can be either "good/bad" or "old/new", which you
tried to eliminate from the previous round.

> +	if test $start_bad_good -eq 1 -a ! -s "$GIT_DIR/BISECT_TERMS"

Avoid test -a (not strictly POSIX, and sometimes ambiguous). Use

test ... && test ...

instead.

> +	then
> +		echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" &&
> +		echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS"
> +	fi &&

Why not do this unconditionnally? Whether terms are good/bad or old/new,
you can write them to BISECT_TERMS.

> -			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
> +			gettextln "You need to give me at least one $(bisect_voc bad) and one $(bisect_voc good) revision.
> +(You can use \"git bisect $(bisect_voc bad)\" and \"git bisect $(bisect_voc good)\" for that.)" >&2

I suspect you broke the i18n here too.

> +get_terms () {
> +	if test -s "$GIT_DIR/BISECT_TERMS"
> +	then
> +		NAME_BAD="$(sed -n 1p "$GIT_DIR/BISECT_TERMS")"
> +		NAME_GOOD="$(sed -n 2p "$GIT_DIR/BISECT_TERMS")"
> +	fi
> +}
> +
> +check_and_set_terms () {
> +	cmd="$1"
> +	case "$cmd" in
> +	bad|good)
> +		if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$NAME_BAD" -a "$cmd" != "$NAME_GOOD"
> +		then
> +			die "$(eval_gettext "Invalid command : you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"

No space before :

> +		fi
> +		case "$cmd" in
> +		bad|good)
> +			if test ! -s "$GIT_DIR/BISECT_TERMS"
> +			then
> +				echo "bad" >"$GIT_DIR/BISECT_TERMS" &&
> +				echo "good" >>"$GIT_DIR/BISECT_TERMS"
> +			fi
> +			NAME_BAD="bad"
> +			NAME_GOOD="good" ;;
> +		esac ;;
> +	esac
> +}
> +
> +bisect_voc () {
> +	case "$1" in
> +	bad) echo "bad" ;;
> +	good) echo "good" ;;
> +	esac
> +}

It's weird to have these hardcoded "bad"/"good" when you already have
BISECT_TERMS in the same patch.

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

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Christian Couder]

On Tue, Jun 9, 2015 at 9:01 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>> Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
>
> s/add/addition/
>
> Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
>
>> +static const char *name_bad;
>> +static const char *name_good;
>
> Same remark as PATCH 2.

As for patch 2, I think "name_bad" and "name_good" are better than
"name_new" and "name_old".

[...]

>> +             name_bad = "bad";
>> +             name_good = "good";
>> +     } else {
>> +             strbuf_getline(&str, fp, '\n');
>> +             name_bad = strbuf_detach(&str, NULL);
>> +             strbuf_getline(&str, fp, '\n');
>> +             name_good = strbuf_detach(&str, NULL);
>> +     }
>
> I would have kept just
>
>         name_bad = "bad";
>         name_good = "good";
>
> in this patch, and introduce BISECT_TERMS in a separate one.

Yeah I agree that it is more logical to have first a patch that does
on bisect.c the same thing as patch 2 does on git-bisect.sh.

For example the patch series could be for now:

1) bisect: typo fix
2) bisect: replace hardcoded "bad|good" with variables
3) git-bisect: replace hardcoded "bad|good" with variables
4) bisect: simplify adding new terms
5) bisect: add old/new terms

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Louis-Alexandre Stuber]

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

> I think you would want to error out if errno is not ENOENT.


Junio C Hamano <jch2355@gmail.com> wrote:

> We might want to see why fopen() failed here. If it is because the
> file did not exist, great. But otherwise?


This file is always supposed to exist when the function is called
unless the user has manually deleted it (or a bug in our programs).

Maybe we should just return an error if the file cannot be opened,
regardless the reason. We kept it like it is, with the default case,
because it was what our elders did, and that aborting because of
BISECT_TERMS is not good for backward compatibility.

But then, in both cases, there is no reason to differenciate ENOENT.
Is that right ?

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Junio C Hamano]

Louis-Alexandre Stuber <stuberl@ensimag.grenoble-inp.fr> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> I think you would want to error out if errno is not ENOENT.
>
>
> Junio C Hamano <jch2355@gmail.com> wrote:
>
>> We might want to see why fopen() failed here. If it is because the
>> file did not exist, great. But otherwise?
>
>
> This file is always supposed to exist when the function is called
> unless the user has manually deleted it (or a bug in our programs).
>
> Maybe we should just return an error if the file cannot be opened,
> regardless the reason. We kept it like it is, with the default case,
> because it was what our elders did, and that aborting because of
> BISECT_TERMS is not good for backward compatibility.
>
> But then, in both cases, there is no reason to differenciate ENOENT.
> Is that right ?

One thing that immediately comes to mind is when you (perhaps
accidentally, or perhaps you were asked to) cd into somebody else's
repository and take a look or help, the file exists but cannot be
read by you and you get EPERM.

That is very different from ENOENT, which is an expected error when
you are not using a customized terms.

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Louis-Alexandre Stuber]

> That is very different from ENOENT, which is an expected error when
> you are not using a customized terms.

But in the current state, we are going to create bisect_terms even if
the bisection is in bad/good mode. Should we differentiate the erors
then ? and should we abort the bisection instead of doing it in
bad/good mode by default ?

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Matthieu Moy]

Louis-Alexandre Stuber <stuberl@ensimag.grenoble-inp.fr> writes:

>> That is very different from ENOENT, which is an expected error when
>> you are not using a customized terms.
>
> But in the current state, we are going to create bisect_terms even if
> the bisection is in bad/good mode.

Which means that in normal cases, you'll either succeed to open it, or
get ENOENT. We're talking about unexcepted cases (you don't have
permission to read it because it's not your file, because you messed up
with a chmod, or whatever reason).

I don't expect a tool to consider that an unreadable file is equivalent
to no file at all. If something's wrong, as a user, I expect a helpful
message telling me what's wrong, to give me a clue on how to solve it.

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

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Louis-Alexandre Stuber]

But ENOENT is not a normal case at all. Should we not treat it the same
way as other fopen() errors ? (either going on with default case or
returning an error)

Would :

>	if (!fp) {
>			die("could not read file '%s': %s",
>				filename, strerror(errno));
>	} else {

be ok ?

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Matthieu Moy]

Louis-Alexandre Stuber <stuberl@ensimag.grenoble-inp.fr> writes:

> But ENOENT is not a normal case at all. Should we not treat it the same
> way as other fopen() errors ? (either going on with default case or
> returning an error)
>
> Would :
>
>>	if (!fp) {
>>			die("could not read file '%s': %s",
>>				filename, strerror(errno));
>>	} else {
>
> be ok ?

That would be much better than what we had in the patch, which did not
look like an error at all:

+	FILE *fp = fopen(filename, "r");
+
+	if (!fp) {
+		name_bad = "bad";
+		name_good = "good";

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

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Junio C Hamano]

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

> Louis-Alexandre Stuber <stuberl@ensimag.grenoble-inp.fr> writes:
>
>>> That is very different from ENOENT, which is an expected error when
>>> you are not using a customized terms.
>>
>> But in the current state, we are going to create bisect_terms even if
>> the bisection is in bad/good mode.
>
> Which means that in normal cases, you'll either succeed to open it, or
> get ENOENT. We're talking about unexcepted cases (you don't have
> permission to read it because it's not your file, because you messed up
> with a chmod, or whatever reason).

I think both I and you misunderstood what they wanted to do, which
is to write out good and bad into terms file even though these are
not customized, and then always read from terms file to learn what
words are used for good and bad.

So from _that_ point of view, ENOENT is an error just like others.

But I do not think it is a good idea to penalize the normal case by
writing the terms file and reading them back from it when the user
is bisecting with good/bad in the first place, so....

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Louis-Alexandre Stuber <stuberl@ensimag.grenoble-inp.fr> writes:
>>
>>>> That is very different from ENOENT, which is an expected error when
>>>> you are not using a customized terms.
>>>
>>> But in the current state, we are going to create bisect_terms even if
>>> the bisection is in bad/good mode.
>>
>> Which means that in normal cases, you'll either succeed to open it, or
>> get ENOENT. We're talking about unexcepted cases (you don't have
>> permission to read it because it's not your file, because you messed up
>> with a chmod, or whatever reason).
>
> I think both I and you misunderstood what they wanted to do, which
> is to write out good and bad into terms file even though these are
> not customized, and then always read from terms file to learn what
> words are used for good and bad.

Yes, indeed.

> But I do not think it is a good idea to penalize the normal case by
> writing the terms file and reading them back from it when the user
> is bisecting with good/bad in the first place, so....

No strong opinion on that, but creating one file doesn't cost much, and
one advantage of writing it unconditionally is that it unifies bad/good
and old/new more in the code. Just the creation of BISECT_TERMS becomes
a special-case.

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

Re: [PATCH 3/4] bisect: simplify the add of new bisect terms

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> But I do not think it is a good idea to penalize the normal case by
>> writing the terms file and reading them back from it when the user
>> is bisecting with good/bad in the first place, so....
>
> No strong opinion on that, but creating one file doesn't cost much, and
> one advantage of writing it unconditionally is that it unifies bad/good
> and old/new more in the code. Just the creation of BISECT_TERMS becomes
> a special-case.

You have to handle that case anyway when I start bisection, which is
going to take very long and I had to leave for the day to come back
the next day to continue, and during the night the sysadmin updates
the installed version of Git.

[PATCH 4/4] bisect: add the terms old/new

[Antoine Delaite]

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.

`git bisect replay` works fine for old/new bisect sessions.

Some commands are still not available for old/new:

     * git bisect start [<new> [<old>...]] is not possible: the
       commits will be treated as bad and good.
     * git rev-list --bisect does not treat the revs/bisect/new and
       revs/bisect/old-SHA1 files.
     * git bisect visualize seem to work partially: the tags are
       displayed correctly but the tree is not limited to the bisect
       section.

Related 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

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@grenoble-inp.fr>
---
 Documentation/git-bisect.txt | 48 ++++++++++++++++++++++++++++++++++++++++++--
 bisect.c                     | 15 ++++++++++----
 git-bisect.sh                | 32 +++++++++++++++++++----------
 t/t6030-bisect-porcelain.sh  | 38 +++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 4cb52a7..441a4bd 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -18,8 +18,8 @@ on the subcommand:
 
  git bisect help
  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
@@ -104,6 +104,35 @@ 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.
 
+
+Alternative terms: bisect new and bisect old
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you are not at ease with the terms "bad" and "good", perhaps
+because you are looking for the commit that introduced a fix, you can
+alternatively use "new" and "old" instead.
+But note that you cannot mix "bad" and good" with "new" and "old".
+
+------------------------------------------------
+git bisect new [<rev>]
+------------------------------------------------
+
+Marks the commit as new, e.g. "the bug is no longer there", if you are looking
+for a commit that fixed a bug, or "the feature that used to work is now broken
+at this point", if you are looking for a commit that introduced a bug.
+It is the equivalent of "git bisect bad [<rev>]".
+
+------------------------------------------------
+git bisect old [<rev>...]
+------------------------------------------------
+
+Marks the commit as old, as the opposite of 'git bisect new'.
+It is the equivalent of "git bisect good [<rev>...]".
+
+You must run `git bisect start` without commits as argument and run
+`git bisect new <rev>`/`git bisect old <rev>...` after to add the
+commits.
+
 Bisect visualize
 ~~~~~~~~~~~~~~~~
 
@@ -379,6 +408,21 @@ 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
+------------
++
+If the last commit has a given property, and we are looking for the commit
+which introduced this property. For each commit the bisection guide us to we
+will test if the property is present, if it is we will mark the commit as new
+with 'git bisect new', otherwise we will mark it as old.
+At the end of the bisect session, the result will be the first new commit (e.g
+the first one with the property).
+
 
 SEE ALSO
 --------
diff --git a/bisect.c b/bisect.c
index 3b7df85..511b905 100644
--- a/bisect.c
+++ b/bisect.c
@@ -409,7 +409,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,
 	if (!strcmp(refname, name_bad)) {
 		current_bad_oid = xmalloc(sizeof(*current_bad_oid));
 		hashcpy(current_bad_oid->hash, sha1);
-	} else if (starts_with(refname, "good-")) {
+	} else if (starts_with(refname, "good-") ||
+		   starts_with(refname, "old-")) {
 		sha1_array_append(&good_revs, sha1);
 	} else if (starts_with(refname, "skip-")) {
 		sha1_array_append(&skipped_revs, sha1);
@@ -741,6 +742,12 @@ static void handle_bad_merge_base(void)
 				"between %s and [%s].\n",
 				bad_hex, bad_hex, good_hex);
 		}
+		else if (!strcmp(name_bad, "new")) {
+			fprintf(stderr, "The merge base %s is new.\n"
+				"The property has changed "
+				"between %s and [%s].\n",
+				bad_hex, bad_hex, good_hex);
+		}
 		exit(3);
 	}
 
@@ -767,11 +774,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 529bb43..896afe9 100644
--- 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|skip|next|reset|visualize|replay|log|run|new|old]'
 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
@@ -286,7 +288,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.
 		gettextln "Warning: bisecting only with a $NAME_BAD commit." >&2
 		if test -t 0
@@ -439,7 +441,7 @@ bisect_replay () {
 		start)
 			cmd="bisect_start $rev"
 			eval "$cmd" ;;
-		good|bad|skip)
+		good|bad|skip|old|new)
 			bisect_write "$command" "$rev" ;;
 		*)
 			die "$(gettext "?? what are you talking about?")" ;;
@@ -523,7 +525,7 @@ get_terms () {
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-	bad|good)
+	bad|good|new|old)
 		if test -s "$GIT_DIR/BISECT_TERMS" -a "$cmd" != "$NAME_BAD" -a "$cmd" != "$NAME_GOOD"
 		then
 			die "$(eval_gettext "Invalid command : you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.")"
@@ -537,14 +539,22 @@ check_and_set_terms () {
 			fi
 			NAME_BAD="bad"
 			NAME_GOOD="good" ;;
+		new|old)
+			if test ! -s "$GIT_DIR/BISECT_TERMS"
+			then
+				echo "new" >"$GIT_DIR/BISECT_TERMS" &&
+				echo "old" >>"$GIT_DIR/BISECT_TERMS"
+			fi
+			NAME_BAD="new"
+			NAME_GOOD="old" ;;
 		esac ;;
 	esac
 }
 
 bisect_voc () {
 	case "$1" in
-	bad) echo "bad" ;;
-	good) echo "good" ;;
+	bad) echo "bad|old" ;;
+	good) echo "good|new" ;;
 	esac
 }
 
@@ -560,7 +570,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..60fd02b 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.1.414.ge7a9de3.dirty

Re: [PATCH 4/4] bisect: add the terms old/new

[Junio C Hamano]

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

> 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.
>
> `git bisect replay` works fine for old/new bisect sessions.
>
> Some commands are still not available for old/new:
>
>      * git bisect start [<new> [<old>...]] is not possible: the
>        commits will be treated as bad and good.

Just throwing a suggestion.  You could perhaps add a new verb to be
used before starting to do anything, e.g.

	$ git bisect terms new old

(where the default mode is "git bisect terms bad good") to set up
the "terms", and then after that

	$ git bisect start $new $old1 $old2...

would be treated as you would naturally expect, no?


>      * git rev-list --bisect does not treat the revs/bisect/new and
>        revs/bisect/old-SHA1 files.

That shouldn't be hard to add, I would imagine, either by

	git rev-list --bisect=new,old

or teaching "git rev-list --bisect" to read the "terms" itself, as a
follow-up patch.

>      * git bisect visualize seem to work partially: the tags are
>        displayed correctly but the tree is not limited to the bisect
>        section.

This is directly related to the lack of "git rev-list --bisect"
support, I would imagine.  If you make the command read "terms", I
suspect that it would solve itself.

> @@ -379,6 +408,21 @@ 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
> +------------
> ++
> +If the last commit has a given property, and we are looking for the commit
> +which introduced this property.

Hmph, that is not a complete sentence and I cannot understand what
it is trying to say.

> +For each commit the bisection guide us to we

s/guide us to we/guides us to, we/;

> +will test if the property is present, if it is we will mark the commit as new
> +with 'git bisect new', otherwise we will mark it as old.

It would be easier to read as separate sentences, i.e.

s/is present, if it is/is present. If it is/;

> diff --git a/bisect.c b/bisect.c
> index 3b7df85..511b905 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -409,7 +409,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,
>  	if (!strcmp(refname, name_bad)) {
>  		current_bad_oid = xmalloc(sizeof(*current_bad_oid));
>  		hashcpy(current_bad_oid->hash, sha1);
> -	} else if (starts_with(refname, "good-")) {
> +	} else if (starts_with(refname, "good-") ||
> +		   starts_with(refname, "old-")) {
>  		sha1_array_append(&good_revs, sha1);
>  	} else if (starts_with(refname, "skip-")) {
>  		sha1_array_append(&skipped_revs, sha1);
> @@ -741,6 +742,12 @@ static void handle_bad_merge_base(void)
>  				"between %s and [%s].\n",
>  				bad_hex, bad_hex, good_hex);
>  		}
> +		else if (!strcmp(name_bad, "new")) {
> +			fprintf(stderr, "The merge base %s is new.\n"
> +				"The property has changed "
> +				"between %s and [%s].\n",
> +				bad_hex, bad_hex, good_hex);
> +		}

After reading the previous patches in the series, I expected that
this new code would know to read "terms" and does not limit us only
to "new" and "old".  Somewhat disappointing.

> @@ -439,7 +441,7 @@ bisect_replay () {
>  		start)
>  			cmd="bisect_start $rev"
>  			eval "$cmd" ;;
> -		good|bad|skip)
> +		good|bad|skip|old|new)

Not NAME_GOOD / NAME_BAD?

To support replay, you may want to consider the "bisect terms"
approach and when the bisection is using non-standard terms record
that as the first entry to the bisect log.

[PATCH 3/4] bisect: simplify the add of new bisect terms

[Antoine Delaite]

Hi, 

Thanks for the review ! 
(sorry if you received this twice)

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

>>> + name_bad = "bad"; 
>>> + name_good = "good"; 
>>> + } else { 
>>> + strbuf_getline(&str, fp, '\n'); 
>>> + name_bad = strbuf_detach(&str, NULL); 
>>> + strbuf_getline(&str, fp, '\n'); 
>>> + name_good = strbuf_detach(&str, NULL); 
>>> + } 
>> 
>> I would have kept just 
>> 
>> name_bad = "bad"; 
>> name_good = "good"; 
>> 
>> in this patch, and introduce BISECT_TERMS in a separate one. 
> 
>Yeah I agree that it is more logical to have first a patch that does 
>on bisect.c the same thing as patch 2 does on git-bisect.sh. 
> 
>For example the patch series could be for now: 
> 
>1) bisect: typo fix 
>2) bisect: replace hardcoded "bad|good" with variables 
>3) git-bisect: replace hardcoded "bad|good" with variables 
>4) bisect: simplify adding new terms 
>5) bisect: add old/new terms 

For now we will keep name_bad and name_good as variables. 
About the patch series shouldn't I squash the commit 2) and 3) into one?