Re: [PATCH v2 2/7] bisect: replace hardcoded "bad|good" by variables
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- Jun 11, 2015, 15:27 UTC
- Message-ID
- <vpq1thi465i.fsf@anie.imag.fr>
- In-Reply-To
- <1433953472-32572-2-git-send-email-antoine.delaite@ensimag.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.
Show 8 quoted lines
> @@ -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=tThere 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.
Show 6 quoted lines
> @@ -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/