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

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

[Antoine Delaite]

Hi, 

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

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

>> +static const char *name_bad; 
>> +static const char *name_good; 
> 
>Same remark as PATCH 2. 

After the discussion you had with Christian I think we will 
keep name_bad/good for now. 

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

Should I put an else {} with nothing in beetween? 

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

Should not I introduce BISECT_TERMS in bisect.c and git-bisect.sh 
with the same commit? I did some rebase though and now name_bad and 
name_good appears in the first commit, and read_bisect_terms in the 
second. 

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

I answered to Junio on this too, it is because our function which create 
the bisect_terms file is not called after 
'git bisect start bad_rev good_rev'. 

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

Because after a git bisect start we don't yet what terms are used. 
This line is only for the case 'git bisect start bad_rev good_rev'. 

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

bisect_voc is used to display what commands the user can do, thus the 
builtins tags. I did not find a way to not hardcode them. 

The other points have been taken into account. 

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

[Matthieu Moy]

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

>>> - 
>>> - 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). 
>
> Should I put an else {} with nothing in beetween? 

What you want to avoid is a senario where the if branch is not taken
silently in the future. Two ways to avoid that:

if (!strcmp(name_bad, "bad")) {
	// special case for good/bad
} else {
	die("BUG: terms %s/%s not managed", name_bad, name_good);
}

Think of someone trying to debug the code later: if you encounter a
die("BUG: ..."), gdb will immediately tell you what's going one.
Debugging the absence of something is much more painful.

Alternatively:

if (!strcmp(name_bad, "bad")) {
	// special case for good/bad
} else {
	fprintf("very generic message not saying \"which means that ...\"");
}

In both cases, adding a new pair of terms should look like

 if (!strcmp(name_bad, "bad")) {
 	// special case for good/bad
+} else if(!strcmp(name_bad, "<new-term>")) {
+	// special case for <new-term>
 } else {
 	die("BUG: terms %s/%s not managed", name_bad, name_good);
 }

I have a slight preference for the "else" with a generic message. It
will be dead code for now, but may turn useful if we ever allow
arbitrary pairs of terms.

>
>>> + 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. 
>
> Should not I introduce BISECT_TERMS in bisect.c and git-bisect.sh 
> with the same commit?

Make sure you have a bisectable history. If you use two commits, you
should make sure that the intermediate step is consistant. If the change
is small enough, it's probably better to have a single commit. No strong
opinion on that (at least not before I see the code).

> I did some rebase though and now name_bad and name_good appears in the
> first commit, and read_bisect_terms in the second.
>
>>> --- 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. 
>
> I answered to Junio on this too, it is because our function which create 
> the bisect_terms file is not called after 
> 'git bisect start bad_rev good_rev'.

Then the variable name is not good. If the variable is there to mean "we
did not define the terms yet", then bisect_terms_defined={0,1} would be
much better (probably not the best possible name, though).

>>> +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. 
>
> bisect_voc is used to display what commands the user can do, thus the 
> builtins tags. I did not find a way to not hardcode them.

Well, if you really want to say good/bad, then using just good/bad would
be simpler than $(bisect_voc good)/$(bisect_voc bad), no? bisect_voc is
just the identitity function (or returns the empty string for input
other than good/bad).

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

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