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

Antoine Delaite [off-list ref] 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 <redacted>
Signed-off-by: Louis Stuber <redacted>
Signed-off-by: Valentin Duperray <redacted>
Signed-off-by: Franck Jonas <redacted>
Signed-off-by: Lucien Kong <redacted>
Signed-off-by: Thomas Nguy <redacted>
Signed-off-by: Huynh Khoi Nguyen Nguyen <redacted>
Signed-off-by: Matthieu Moy <redacted>
---

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?

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

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?

Hi,

Thanks for the review,

Junio C Hamano [off-list ref] writes:

quoted

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

quoted

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

quoted

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.

quoted

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

quoted

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