For clarity and consistency in this codebase, prefer explicit parentheses when using the result of an expression as a value. That is, this:

$x = ($y > $z);

...is preferable to:

$x = $y > $z;

This should be a lint rule, but this change didn't come through arc and missed lint anyway, so it's sort of moot.

The existing code also incorrectly doesn't follow this convention, and returns an expression later on (return x && y instead of return (x && y)).

For correctness, prefer === over ==. This should also be a lint rule, but would be ambitious at this point, see also "Increase Strictness" in T2312.

$stdout may be fewer than 5 characters long. In PHP 7 and newer, substr() returns "" in this case. In versions older than PHP 7, it returns false.

It seems at least plausible that this may emit a warning in some future version of PHP (or possibly a manufactured version of PHP under T2312). Even if it doesn't, the ambiguity of the input and variance of behavior across PHP versions is probably worth avoiding by either testing for the string length before taking a slice of it or using substr_compare() instead, where the semantics of out-of-bounds ranges are less ambiguous.

The byte stream has "<length><raw bytes>" sections and may naturally contain \n0000 outside of a flush packet (that is, none of these bytes are guaranteed to be escaped if they appear elsewhere in the protocol).

I think this is still a better approach than matching error text on stderr. Phabricator can parse significant chunks of the Git protocol to some degree now (see D20381; this capability was built after the code here was written) and we could imagine pursuing a full-power version of this change where we parse the entire protocol. I intend to do this eventually, but I think this change is a fine stopgap in the meantime.

There is no guarantee that the frame prior to the 0-length frame terminates with "\n". Locally, if I fetch a HEAD I already have (git fetch --depth 1 origin HEAD), I get this over the wire (I've added visual newlines for clarity; newlines in the raw wire bytes are represented with \r and \n):

Expires: Fri, 01 Jan 1980 00:00:00 GMT\r\n
Pragma: no-cache\r\n
Cache-Control: no-cache, max-age=0, must-revalidate\r\n
Content-Type: application/x-git-upload-pack-result\r\n
\r\n
0034
shallow 2d34f0e1ea69324a12f7e4a3f277b13f104370a9
0000

The documentation has examples with a reflist frame that does terminate with \n, but the frame format in general is just <length><stuff>, and not all frames terminate like this.

A check against just 0000 (versus \n0000) seems like the simplest adjustment.