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

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

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

> -	if (!strcmp(refname, "bad")) {
> +	char good_prefix[256];
> +	strcpy(good_prefix, name_good);
> +	strcat(good_prefix, "-");

You are silently adding a restriction here: name_good must be small
enough to fit in a 256-bytes array. It's not a terrible restriction, but
what may happen if you break it is a real issue.

Either you have to enforce this restriction somewhere, or you should not
have the restriction at all. I'd vote for the second. strbuf is your
friend here.

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

There are other restrictions here: $NAME_BAD must be an acceptable ref
name, and you're not quoting $NAME_BAD hence it must not contain shell
meta-characters (The requirements for ref names almost imply that, but
'foo/bar{a,b}' is accepted and will trigger some expansion if your
/bin/sh is bash for example).

Being an acceptable ref name is a constraint you have to check (Junio
already mentionned check-ref-format). I think quoting variables makes
sense too.

> @@ -421,7 +426,7 @@ bisect_replay () {
>  		start)
>  			cmd="bisect_start $rev"
>  			eval "$cmd" ;;
> -		good|bad|skip)
> +		$NAME_GOOD|$NAME_BAD|skip)

$NAME_GOOD and $NAME_BAD need quoting if you're not sure they don't
contain shell metacharacters.

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