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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jun 11, 2015, 15:28 UTC
Message-ID: <vpqy4jq2rjr.fsf@anie.imag.fr>
In-Reply-To: <1433962918-6536-3-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr>

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

Show 7 quoted lines

> -	# 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 ;-) )

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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