Re: [PATCH v3 0/3] Improve robustness of putty detection

[Jeff King <peff@peff.net>]

On Sun, Apr 26, 2015 at 03:04:56PM -0700, Junio C Hamano wrote:

> The test scripts are expected to take either 3 or 4 parameters, and
> the extra parameter when it takes 4 is the comma separated list of
> prerequisites.  "bracketed hostnames are still ssh" does not look
> like prerequisites at all to us humans, and the framework should
> also be able to notice that and barf, I would think.
> 
> Perhaps something like this?

I like it. I wondered if we could even recognize a known set of prereqs
(e.g., say "I don't know about the FOO prereq; did you forget to
test_lazy_prereq it?"), but I don't think we can. Some of the prereqs
are set by arbitrary code, and when they are not set, we don't call a
"test_do_not_set_prereq FOO".  Enforcing a sane syntax is almost as
good, and seems pretty easy to implement.

> +test_verify_prereq () {
> +	test -z "$test_prereq" ||
> +	expr >/dev/null "$test_prereq" : '^[A-Z0-9_,!]*$' ||
> +	error "bug in the test script: '$test_prereq' does not look like a prereq"
> +}

The leading "^" is unnecessary in an expr regexp, as such expressions
are always left-anchored. And according to POSIX, even undesirable
due to hysterical raisins. You do still need the '$' to anchor the end.

I was surprised that the regexp does not match the empty string itself
(making the initial "test -z" redundant), but it does not seem to with
my version of expr. Weird. I think it is because the exit value is not
"did it match" but "is the return value of the expression non-zero", and
of course we matched zero characters.

At any rate, we are probably much better off having the initial "test
-z" there as an optimization, anyway. "expr" is not usually a built-in,
and otherwise we add an extra fork to each test invocation (not
something I expect matters under Linux, but I know Windows folks are
very sensitive to it).

-Peff