[PATCH v2 5/7] bisect: change read_bisect_terms parameters

[PATCH v2 5/7] bisect: change read_bisect_terms parameters

[Antoine Delaite]

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

The function reads BISECT_TERMS and stores it at the adress given in
parameters (instead of global variables name_bad and name_good).

This allows to use the function outside bisect.c.

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
---
 bisect.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/bisect.c b/bisect.c
index eaa85b6..7afd335 100644
--- a/bisect.c
+++ b/bisect.c
@@ -908,12 +908,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 }
 
 /*
- * The terms used for this bisect session are stored in
- * BISECT_TERMS: it can be bad/good or new/old.
- * We read them and store them to adapt the messages
- * accordingly. Default is bad/good.
+ * 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(void)
+void read_bisect_terms(const char **read_bad, const char **read_good)
 {
 	struct strbuf str = STRBUF_INIT;
 	const char *filename = git_path("BISECT_TERMS");
@@ -924,9 +923,9 @@ void read_bisect_terms(void)
 			strerror(errno));
 	} else {
 		strbuf_getline(&str, fp, '\n');
-		name_bad = strbuf_detach(&str, NULL);
+		*read_bad = strbuf_detach(&str, NULL);
 		strbuf_getline(&str, fp, '\n');
-		name_good = strbuf_detach(&str, NULL);
+		*read_good = strbuf_detach(&str, NULL);
 	}
 	strbuf_release(&str);
 	fclose(fp);
@@ -948,7 +947,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();
+	read_bisect_terms(&name_bad, &name_good);
 	if (read_bisect_refs())
 		die("reading bisect refs failed");
 
-- 
1.7.1

[PATCH v2 6/7] revision: fix rev-list --bisect in old/new mode

[Antoine Delaite]

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

Calling git rev-list --bisect when an old/new mode bisection was started
shows the help notice. This has been fixed by reading BISECT_TERMS in
revision.c to find the correct bisect refs path (which was always
refs/bisect/bad (or good) before and can be refs/bisect/new (old) now).

Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
---
 revision.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 7ddbaa0..a332270 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;
@@ -2073,14 +2076,22 @@ 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);
+	char bisect_refs_path[256];
+	strcpy(bisect_refs_path, "refs/bisect/");
+	strcat(bisect_refs_path, name_bad);
+	return for_each_ref_in_submodule(submodule, bisect_refs_path, fn, cb_data);
 }
 
 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);
+	char bisect_refs_path[256];
+	strcpy(bisect_refs_path, "refs/bisect/");
+	strcat(bisect_refs_path, name_good);
+	return for_each_ref_in_submodule(submodule, bisect_refs_path, fn, cb_data);
 }
 
 static int handle_revision_pseudo_opt(const char *submodule,
@@ -2109,6 +2120,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;
-- 
1.7.1

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

Introduction of the git bisect terms function.
The user can set its own terms.

List of known commands not available :
`git bisect replay`
`git bisect terms term1 term2
then
git bisect start bad_rev good_rev`

Signed-off-by: Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr>
Signed-off-by: Louis Stuber <stuberl@ensimag.grenoble-inp.fr>
---
 Documentation/git-bisect.txt |   19 ++++++++++++++++++
 git-bisect.sh                |   44 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 3c3021a..ef0c03c 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -133,6 +133,25 @@ 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.
 
+Alternative terms: use your own terms
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If the builtins terms bad/good and new/old do not satisfy you, you can
+set your own terms.
+
+------------------------------------------------
+git bisect terms term1 term2
+------------------------------------------------
+
+This command has to be used before a bisection has started.
+The term1 must be associated with the latest revisions and term2 with the
+ancestors of term1.
+
+Only the first bisection following the 'git bisect terms' will use the terms.
+If you mistyped one of the terms you can do again 'git bisect terms term1
+term2'.
+
+
 Bisect visualize
 ~~~~~~~~~~~~~~~~
 
diff --git a/git-bisect.sh b/git-bisect.sh
index c012f5d..22d65b1 100644
--- 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 term1 term2
+	set up term1 and term2 as bisection terms.
 git bisect skip [(<rev>|<range>)...]
 	mark <rev>... untestable revisions.
 git bisect next
@@ -79,9 +81,16 @@ bisect_start() {
 	orig_args=$(git rev-parse --sq-quote "$@")
 	bad_seen=0
 	eval=''
-	# start_bad_good is used to detect if we did a 
-	# 'git bisect start bad_rev good_rev'
-	start_bad_good=0
+	# terms_defined is used to detect if we did a
+	# 'git bisect start bad_rev good_rev' or if the user
+	# defined his own terms with git bisect terms
+	terms_defined=0
+	if test -s "$GIT_DIR/TERMS_DEFINED"
+	then
+		terms_defined=1
+		get_terms
+		rm -rf "$GIT_DIR/TERMS_DEFINED"
+	fi
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
 		mode=--no-checkout
@@ -107,7 +116,7 @@ bisect_start() {
 				break
 			}
 
-			start_bad_good=1
+			terms_defined=1
 
 			case $bad_seen in
 			0) state=$NAME_BAD ; bad_seen=1 ;;
@@ -180,7 +189,7 @@ bisect_start() {
 	} &&
 	git rev-parse --sq-quote "$@" >"$GIT_DIR/BISECT_NAMES" &&
 	eval "$eval true" &&
-	if test $start_bad_good -eq 1 && test ! -s "$GIT_DIR/BISECT_TERMS"
+	if test $terms_defined -eq 1 && test ! -s "$GIT_DIR/BISECT_TERMS"
 	then
 		echo "$NAME_BAD" >"$GIT_DIR/BISECT_TERMS" &&
 		echo "$NAME_GOOD" >>"$GIT_DIR/BISECT_TERMS"
@@ -419,6 +428,7 @@ bisect_clean_state() {
 	rm -f "$GIT_DIR/BISECT_NAMES" &&
 	rm -f "$GIT_DIR/BISECT_RUN" &&
 	rm -f "$GIT_DIR/BISECT_TERMS" &&
+	rm -f "$GIT_DIR/TERMS_DEFINED" &&
 	# 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 &&
@@ -529,7 +539,8 @@ get_terms () {
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
-	bad|good|new|old)
+	skip) ;;
+	*)
 		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.")"
@@ -562,6 +573,21 @@ bisect_voc () {
 	esac
 }
 
+bisect_terms () {
+	test $# -eq 2 ||
+	die "You need to give me at least two arguments"
+
+	if ! test -s "$GIT_DIR/BISECT_START"
+	then
+		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
+		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
+		echo "1" > "$GIT_DIR/TERMS_DEFINED"
+	else
+		die "A bisection has already started, please use "\
+		"'git bisect reset' to restart and change the terms"
+	fi
+}
+
 case "$#" in
 0)
 	usage ;;
@@ -574,7 +600,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 "$@" ;;
@@ -591,6 +617,8 @@ case "$#" in
 		bisect_log ;;
 	run)
 		bisect_run "$@" ;;
+	terms)
+		bisect_terms "$@" ;;
 	*)
 		usage ;;
 	esac
-- 
1.7.1

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Junio C Hamano]

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

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

I think this patch makes the whole series go in the right direction.

I wonder if you can skip the "we only support new/old if you are not
doing bog-standard bad/good" step and start from this "bisect terms"
one, though.

Then you do not even have to treat new/old any specially, and do not
even have to list them in the above list.

> @@ -79,9 +81,16 @@ bisect_start() {
>  	orig_args=$(git rev-parse --sq-quote "$@")
>  	bad_seen=0
>  	eval=''
> -	# start_bad_good is used to detect if we did a 
> -	# 'git bisect start bad_rev good_rev'
> -	start_bad_good=0
> +	# terms_defined is used to detect if we did a
> +	# 'git bisect start bad_rev good_rev' or if the user
> +	# defined his own terms with git bisect terms
> +	terms_defined=0

I like this change very much; it removes the mysteriously misnamed
start-bad-good variable (because you do not really _care_ that
'start' was what implicitly decided that good/bad pair is the term
we use in this session; what you care is that the terms are already
known or not).

That is another reason why I think it would be a better organization
for the patch series to do without the intermediate "we now add new/old
as another hardcoded values on top of the traditional bad/good".

That is, I would think a reasonable progression of the series would
look more like these three steps:

 - preliminary clean-up steps (e.g. "correct 'mistook'");

 - use $name_new and $name_old throughout the code, giving them
   'bad' and 'good' as hardcoded values; finally

 - add 'bisect terms' support.

Thanks.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Junio C Hamano]

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

> I like this change very much; it removes the mysteriously misnamed
> start-bad-good variable (because you do not really _care_ that
> 'start' was what implicitly decided that good/bad pair is the term
> we use in this session; what you care is that the terms are already
> known or not).
>
> That is another reason why I think it would be a better organization
> for the patch series to do without the intermediate "we now add new/old
> as another hardcoded values on top of the traditional bad/good".
>
> That is, I would think a reasonable progression of the series would
> look more like these three steps:
>
>  - preliminary clean-up steps (e.g. "correct 'mistook'");
>
>  - use $name_new and $name_old throughout the code, giving them
>    'bad' and 'good' as hardcoded values; finally
>
>  - add 'bisect terms' support.

Just in case I confused readers with a message that apparently
conflicts with what I said in the ancient thread:

  http://thread.gmane.org/gmane.comp.version-control.git/199758/focus=200025

I am admitting that I was wrong.  Or perhaps I was right back then,
but the world has changed ;-).

We have been hearing "bisect bad/good" is hard to use for the last 3
years since that thread discussed this topic, and that made me
realize that addition of single new/old may not be good enough, and
we should bite the bullet to support 'bisect terms' properly, making
bad/good and new/old even less special (contrary to what I said back
then in that thread "we only need these two pairs"), following the
suggestion by Phil Hord in that thread.

And the suggested three-step approach above reflects that new world
order in my mind.  We admit that the machinery should have been
built around a value-agnostic "old vs new" in the first place, but
unfortunately it wasn't.  So we belatedly update the system to use
these two terms internally to express the logic by naming the
variables name_old and name_new after these two value-agnostic
concepts, and then support the traditional "good vs bad" as a mere
default values for the names of old and new states.

One thing I forgot to say in the review of this patch:

> +bisect_terms () {
> +	test $# -eq 2 ||
> +	die "You need to give me at least two arguments"
> +
> +	if ! test -s "$GIT_DIR/BISECT_START"
> +	then
> +		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
> +		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
> +		echo "1" > "$GIT_DIR/TERMS_DEFINED"
> +	else
> +		die "A bisection has already started, please use "\
> +		"'git bisect reset' to restart and change the terms"
> +	fi
> +}
> +

I think "git bisect terms" is a good way to help a user to recall
what two names s/he decided to use for the current session.  So
dying 'already started' with suggestion for 'reset' is OK, but at
the same time, helping the user to continue the current bisection by
giving a message along the lines of "You are hunting for a commit
that is at the boundary of the old state (you are calling it
'$NAME_OLD') and the new state ('$NAME_NEW')" would be a good idea.

Thanks.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> +bisect_terms () {
>> +	test $# -eq 2 ||
>> +	die "You need to give me at least two arguments"
>> +
>> +	if ! test -s "$GIT_DIR/BISECT_START"
>> +	then
>> +		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
>> +		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
>> +		echo "1" > "$GIT_DIR/TERMS_DEFINED"
>> +	else
>> +		die "A bisection has already started, please use "\
>> +		"'git bisect reset' to restart and change the terms"
>> +	fi
>> +}
>> +
>
> I think "git bisect terms" is a good way to help a user to recall
> what two names s/he decided to use for the current session.  So
> dying 'already started' with suggestion for 'reset' is OK, but at
> the same time, helping the user to continue the current bisection by
> giving a message along the lines of "You are hunting for a commit
> that is at the boundary of the old state (you are calling it
> '$NAME_OLD') and the new state ('$NAME_NEW')" would be a good idea.

I'd put a very verbose message explaining the situation and the way out
(use 'git bisect') for the second "die", and I would consider "git
bisect terms" without arguments as a valid command to ask "please tell
me what the terms are".

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

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> +bisect_terms () {
>>> +	test $# -eq 2 ||
>>> +	die "You need to give me at least two arguments"
>>> +
>>> +	if ! test -s "$GIT_DIR/BISECT_START"
>>> +	then
>>> +		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
>>> +		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
>>> +		echo "1" > "$GIT_DIR/TERMS_DEFINED"
>>> +	else
>>> +		die "A bisection has already started, please use "\
>>> +		"'git bisect reset' to restart and change the terms"
>>> +	fi
>>> +}
>>> +
>>
>> I think "git bisect terms" is a good way to help a user to recall
>> what two names s/he decided to use for the current session.  So
>> dying 'already started' with suggestion for 'reset' is OK, but at
>> the same time, helping the user to continue the current bisection by
>> giving a message along the lines of "You are hunting for a commit
>> that is at the boundary of the old state (you are calling it
>> '$NAME_OLD') and the new state ('$NAME_NEW')" would be a good idea.
>
> I'd put a very verbose message explaining the situation and the way out
> (use 'git bisect') for the second "die", and I would consider "git
> bisect terms" without arguments as a valid command to ask "please tell
> me what the terms are".

Of course you are right.  The "remind me what I was doing" help
should be given when the user asks "git bisect terms" without any
parameters, not in "else die" part.

Thanks for a correction.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
>
>> -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]'
>
> I think this patch makes the whole series go in the right direction.
>
> I wonder if you can skip the "we only support new/old if you are not
> doing bog-standard bad/good" step and start from this "bisect terms"
> one, though.

While I think we should not hardcode too much in the code, I also think
it makes sense to have hardcoded old/new in the user-interface. If you
give me just

  git bisect terms <first-term> <second-term>

then half of the times, I'll use old/new in the wrong order. And if I
hadn't looked to bisect close enough, I'd even complain that the terms
are not usable interchangeably.

At least, a in a flow like

  git bisect start
  git bisect new
  git checkout <old-sha>
  git bisect old

there's no ambiguity.

So, to me, having both hardcoded old/new (unambiguous) and "git bisect
terms" (for flexibility) makes sense.

Another important parameter: the students are finishing their project
tomorrow. They may continue on their personnal free time, but at best
their time budget is considerably reduced, and from my past experience,
the time budget usually reaches 0. That's not a reason to merge bad code
in git.git, but an incentive to close the series with something going in
the right direction. Typically:

>  - preliminary clean-up steps (e.g. "correct 'mistook'");

This is straightforward

>  - use $name_new and $name_old throughout the code, giving them
>    'bad' and 'good' as hardcoded values; finally

This is relatively uncontroversial, and should be easy to finish. I
would consider it a good thing anyway.

>  - add 'bisect terms' support.

I'd put this on the low-priority whishlist. If the codebase is ready and
a preliminary patch exists, it will be relatively easy for someone else
to finish the job.

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

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:
>>
>>> -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]'
>>
>> I think this patch makes the whole series go in the right direction.
>>
>> I wonder if you can skip the "we only support new/old if you are not
>> doing bog-standard bad/good" step and start from this "bisect terms"
>> one, though.
>
> While I think we should not hardcode too much in the code, I also think
> it makes sense to have hardcoded old/new in the user-interface. 

OK.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> -	# start_bad_good is used to detect if we did a 
> -	# 'git bisect start bad_rev good_rev'
> -	start_bad_good=0
> +	# terms_defined is used to detect if we did a
> +	# 'git bisect start bad_rev good_rev' or if the user
> +	# defined his own terms with git bisect terms
> +	terms_defined=0

Modifying in PATCH 7 some code that you introduced in PATCH 3 is
suspicious. Is there any reason you did not name the variable
"terms_defined" in the first place? (i.e. squash this hunk and the other
instance of start_bad_good into PATCH 3)

(Whether this is a rethorical question is up to you ;-) )

> +	if test -s "$GIT_DIR/TERMS_DEFINED"
> +	then
> +		terms_defined=1
> +		get_terms
> +		rm -rf "$GIT_DIR/TERMS_DEFINED"

I don't understand why you need to delete this file. I did not review
thoroughly so there may be a reason, but you can help the reader with a
comment here.

> +bisect_terms () {
> +	test $# -eq 2 ||
> +	die "You need to give me at least two arguments"
> +
> +	if ! test -s "$GIT_DIR/BISECT_START"
> +	then
> +		echo $1 >"$GIT_DIR/BISECT_TERMS" &&
> +		echo $2 >>"$GIT_DIR/BISECT_TERMS" &&
> +		echo "1" > "$GIT_DIR/TERMS_DEFINED"

Space after >.

> @@ -574,7 +600,7 @@ case "$#" in
>  		git bisect -h ;;
>  	start)
>  		bisect_start "$@" ;;
> -	bad|good|new|old)
> +	bad|good|new|old|$NAME_BAD|$NAME_GOOD)

See my other message about quoting.

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

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Louis-Alexandre Stuber]

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

> Modifying in PATCH 7 some code that you introduced in PATCH 3 is
> suspicious. Is there any reason you did not name the variable
> "terms_defined" in the first place? (i.e. squash this hunk and the other
> instance of start_bad_good into PATCH 3)
>
> (Whether this is a rethorical question is up to you ;-) )

In the previouses versions where we only want to introduce old/new,
the terms can only be defined in bisect_start if the user typed
start <bad> <good>. The name "start_bad_good" is not very explicit
indeed, but isn't it more appropriate in this case than terms_defined ?

> I don't understand why you need to delete this file. I did not review
> thoroughly so there may be a reason, but you can help the reader with a
> comment here.

I think it's a mistake. I'd say we should put this test just before the
"bisect_clean_state || exit" line, but that would deserve more attention
indeed. The idea is to delete the file at the right moment because we 
don't want it to exist again when the user starts an other bisection,
but also have an intelligent behaviour if the start command fails.

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

Louis Stuber <louis--alexandre.stuber@ensimag.grenoble-inp.fr> writes: 

>Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: 
> 
>> Modifying in PATCH 7 some code that you introduced in PATCH 3 is 
>> suspicious. Is there any reason you did not name the variable 
>> "terms_defined" in the first place? (i.e. squash this hunk and the other 
>> instance of start_bad_good into PATCH 3) 
>> 
>> (Whether this is a rethorical question is up to you ;-) ) 
> 
>In the previouses versions where we only want to introduce old/new, 
>the terms can only be defined in bisect_start if the user typed 
>start <bad> <good>. The name "start_bad_good" is not very explicit 
>indeed, but isn't it more appropriate in this case than terms_defined ? 

I agree with Louis, but maybe a consistant commit history is more 
important. But if only the first patches (which implement old/new ) 
would come to be accepted the name of the variable would sounds strange. 

>> I don't understand why you need to delete this file. I did not review 
>> thoroughly so there may be a reason, but you can help the reader with a 
>> comment here. 
> 
>I think it's a mistake. I'd say we should put this test just before the 
>"bisect_clean_state || exit" line, but that would deserve more attention 
>indeed. The idea is to delete the file at the right moment because we 
>don't want it to exist again when the user starts an other bisection, 
>but also have an intelligent behaviour if the start command fails. 

Yes if the start commands fails, like if you gave wrong sha1, it would be 
nice not to have to type again 'git bisect terms ...' . There is no use to 
put it before clean_state because clean_state because clean_state will 
actually delete the file. So we can just let clean_state do this. 

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Louis Stuber <louis--alexandre.stuber@ensimag.grenoble-inp.fr> writes: 
>
>>Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: 
>> 
>>> Modifying in PATCH 7 some code that you introduced in PATCH 3 is 
>>> suspicious. Is there any reason you did not name the variable 
>>> "terms_defined" in the first place? (i.e. squash this hunk and the other 
>>> instance of start_bad_good into PATCH 3) 
>>> 
>>> (Whether this is a rethorical question is up to you ;-) ) 
>> 
>>In the previouses versions where we only want to introduce old/new, 
>>the terms can only be defined in bisect_start if the user typed 
>>start <bad> <good>. The name "start_bad_good" is not very explicit 
>>indeed, but isn't it more appropriate in this case than terms_defined ? 
>
> I agree with Louis, but maybe a consistant commit history is more 
> important. But if only the first patches (which implement old/new ) 
> would come to be accepted the name of the variable would sounds strange. 

I would say terms_defined is OK even if only the first patches get
merged. The reason why you need this variable is because you need to
know whether the terms have been defined or not, and to me that's the
most important.

I'd suggest something like this:

# terms_defined is 0 when the user did not define the terms explicitely
# yet. This is the case when running 'git bisect start bad_rev good_rev'
# before we see an explicit reference to a term.
terms_defined=0

Then PATCH 7/7 can add a mention of 'git bisect terms' just in the
comment.

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

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Junio C Hamano]

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

> I would say terms_defined is OK even if only the first patches get
> merged. The reason why you need this variable is because you need to
> know whether the terms have been defined or not, and to me that's the
> most important.

Well said.  Thanks.

>
> I'd suggest something like this:
>
> # terms_defined is 0 when the user did not define the terms explicitely

s/citely/citly/;

> # yet. This is the case when running 'git bisect start bad_rev good_rev'
> # before we see an explicit reference to a term.
> terms_defined=0
>
> Then PATCH 7/7 can add a mention of 'git bisect terms' just in the
> comment.

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

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

># terms_defined is 0 when the user did not define the terms explicitely
># yet. This is the case when running 'git bisect start bad_rev good_rev'
># before we see an explicit reference to a term.
>terms_defined=0

The thing is:
'git bisect reset
git bisect new HEAD
(autostart ?) y'

is strictly equivalent to

'git bisect reset
git bisect start
git bisect new HEAD'

In both case terms_defined value would be 0 while we kinda know that a
term has been used. It just that in the code it is not taken in account yet.
I don't really mind doing the change but I'm just pointing out that it can be
confusing.

Thanks.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>># terms_defined is 0 when the user did not define the terms explicitely
>># yet. This is the case when running 'git bisect start bad_rev good_rev'
>># before we see an explicit reference to a term.
>>terms_defined=0
>
> The thing is:
> 'git bisect reset
> git bisect new HEAD

"git bisect new" does not exist. Did you mean "git bisect start HEAD"?

> (autostart ?) y'
>
> is strictly equivalent to
>
> 'git bisect reset
> git bisect start
> git bisect new HEAD'
>
> In both case terms_defined value would be 0 while we kinda know that a
> term has been used.

I don't understand. The user didn't say either "bad" or "good", so in
both cases we haven't seen a term yet. Or I misunderstood what you meant
by "define a term".

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

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

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

>> 
>>># terms_defined is 0 when the user did not define the terms explicitely 
>>># yet. This is the case when running 'git bisect start bad_rev good_rev' 
>>># before we see an explicit reference to a term. 
>>>terms_defined=0 
>> 
>> The thing is: 
>> 'git bisect reset 
>> git bisect new HEAD 

>"git bisect new" does not exist. Did you mean "git bisect start HEAD"? 

No I meant new but it can be 'git bisect bad' aswell
So 
'
git bisect reset
git bisect bad
answer yes to "autostart ?"
'

>I don't understand. The user didn't say either "bad" or "good", so in 
>both cases we haven't seen a term yet. Or I misunderstood what you meant 
>by "define a term". 

In the case I rewrited, we saw a 'bad' but terms_defined value in bisect_start
(called by the autostart) is 0. 


Thanks.

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: 
>
>>> 
>>>># terms_defined is 0 when the user did not define the terms explicitely 
>>>># yet. This is the case when running 'git bisect start bad_rev good_rev' 
>>>># before we see an explicit reference to a term. 
>>>>terms_defined=0 
>>> 
>>> The thing is: 
>>> 'git bisect reset 
>>> git bisect new HEAD 
>
>>"git bisect new" does not exist. Did you mean "git bisect start HEAD"? 
>
> No I meant new but it can be 'git bisect bad' aswell

OK, answering emails before coffee doesn't suit me well, I did not
remember that the series was about "new" ;-).

(Actually your use-case is not possible yet as of PATCH 3 which
introduces start_bad_good. It is possible after PATCH 4)

> So 
> '
> git bisect reset
> git bisect bad
> answer yes to "autostart ?"
> '

> In the case I rewrited, we saw a 'bad' but terms_defined value in bisect_start
> (called by the autostart) is 0. 

As you said, it is really equivalent to

git bisect start
git bisect bad

the autostart is just a convenience piece of code to run "git bisect
start" for the user, but does not change the logic of the code. Write
good code for the normal case (start, and then bad), and it will just
work without having to worry about in in the autostart case.

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

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

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

>> +        if test -s "$GIT_DIR/TERMS_DEFINED"
>> +        then
>> +                terms_defined=1
>> +                get_terms
>> +                rm -rf "$GIT_DIR/TERMS_DEFINED"
>
>I don't understand why you need to delete this file. I did not review
>thoroughly so there may be a reason, but you can help the reader with a
>comment here.

I will just complete Louis' answer. We delete it with backward
compatibility with old/new in mind (even if old/new is not merged yet).
For instance, after a old/new mode, if you do a 'bisect start rev1 rev2'
the mode would be bad/good ie the default mode. So if you defined your
terms, we decided it would only be for the following bisection. The next 
'bisect start rev1 rev2' would be in bad/good mode.
But this have to be discuted, do the user have to type 'git bisect terms'
each bisection if he wants to use special terms ?

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

This message's goal is to explained where am I on the bisect 
functions. 

-git bisect replay now work with any terms. 
-I took account of most of the last reviews (coding style, 
double sed ...)
-'git bisect terms' without arguments now display the current 
terms 
-bisect_terms : if a bisection has already started, a more 
verbose message is displayed 
-I solved a merge conflict in bisect.c due to the update of 
master branch. 

To submit a v3 I would need answer about how we rebase the commit 
history and what do we do to simplify the life of the user with the 
terms (see my last mails). 
I was thinking of: 
-a config variable that would say :"as long as I don't reset keep 
the terms of the previous bisection" 
or we could decided that this is the default behaviour of bisect. 

-two config variables to choose default terms 

I may have less time in the next days but I would like to submit a v3. 

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> To submit a v3 I would need answer about how we rebase the commit 
> history and what do we do to simplify the life of the user with the 
> terms (see my last mails). 
> I was thinking of: 
> -a config variable that would say :"as long as I don't reset keep 
> the terms of the previous bisection" 
> or we could decided that this is the default behaviour of bisect. 
>
> -two config variables to choose default terms 

I'd say: keep it simple, try to close this series ASAP. It'll still be
time to send another series once this one has settled.

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

Re: [PATCH v2 7/7] bisect: allows any terms set by user

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>> +        if test -s "$GIT_DIR/TERMS_DEFINED"
>>> +        then
>>> +                terms_defined=1
>>> +                get_terms
>>> +                rm -rf "$GIT_DIR/TERMS_DEFINED"
>>
>>I don't understand why you need to delete this file. I did not review
>>thoroughly so there may be a reason, but you can help the reader with a
>>comment here.
>
> I will just complete Louis' answer. We delete it with backward
> compatibility with old/new in mind (even if old/new is not merged yet).
> For instance, after a old/new mode, if you do a 'bisect start rev1 rev2'
> the mode would be bad/good ie the default mode. So if you defined your
> terms, we decided it would only be for the following bisection.

I would say "for the current bisection", i.e.

$ git bisect start
$ git bisect terms foo bar # Scope starts here
...
The first 'bar' commit is deadbeef. # Scope ends here

$ git bisect terms foo bar
You need to start by "git bisect start"
Do you want me to do it for you [Y/n]?

> The next 'bisect start rev1 rev2' would be in bad/good mode. But this
> have to be discuted, do the user have to type 'git bisect terms' each
> bisection if he wants to use special terms ?

To me, yes. If we allow arbitrary terms, then they will most likely be
specific to one bisect session (e.g. if I bisect "present/absent" once,
it tells me nothing about what I'll want for my next bisection).

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

[PATCH v2 7/7] bisect: allows any terms set by user

[Antoine Delaite]

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

>I would say "for the current bisection", i.e. 
> 
>$ git bisect start 
>$ git bisect terms foo bar # Scope starts here 
>... 
>The first 'bar' commit is deadbeef. # Scope ends here 
> 
>$ git bisect terms foo bar 
>You need to start by "git bisect start" 
>Do you want me to do it for you [Y/n]? 

I personnaly don't like this autostart. In the first version of this patch 
7/7 'git bisect terms' has to be called before the start. I think it is 
better because we can then use 'git bisect start rev1 rev2 ...' 

>> The next 'bisect start rev1 rev2' would be in bad/good mode. But this 
>> have to be discuted, do the user have to type 'git bisect terms' each 
>> bisection if he wants to use special terms ? 
> 
>To me, yes. If we allow arbitrary terms, then they will most likely be 
>specific to one bisect session (e.g. if I bisect "present/absent" once, 
>it tells me nothing about what I'll want for my next bisection). 

Ok. 

Thanks.