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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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