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

[Junio C Hamano <gitster@pobox.com>]

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?