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

From: Jeff King <peff@peff.net>
Date: Apr 28, 2015, 04:15 UTC
Message-ID: <20150428041521.GB24580@peff.net>
In-Reply-To: <xmqq4mo2zgtz.fsf@gitster.dls.corp.google.com>

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

Show 7 quoted lines

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

Show 5 quoted lines

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