Changeset View
Standalone View
src/applications/diffusion/controller/DiffusionServeController.php
| Context not available. | |||||
| // This is a pretty funky fix: it would be nice to more precisely detect | // This is a pretty funky fix: it would be nice to more precisely detect | ||||
| // that a request is a `--depth N` clone request, but we don't have any code | // that a request is a `--depth N` clone request, but we don't have any code | ||||
| // to decode protocol frames yet. Instead, look for reasonable evidence | // to decode protocol frames yet. Instead, look for reasonable evidence | ||||
| // in the error and output that we're looking at a `--depth` clone. | // in the output that we're looking at a `--depth` clone. | ||||
| // For evidence this isn't completely crazy, see: | // A valid x-git-upload-pack-result response during packfile negotiation | ||||
| // https://github.com/schacon/grack/pull/7 | // should end with a flush packet ("0000"). As long as that packet | ||||
| // terminates the response body in the response, we'll assume the response | |||||
| // is correct and complete. | |||||
| // See https://git-scm.com/docs/pack-protocol#_packfile_negotiation | |||||
| $stdout_regexp = '(^Content-Type: application/x-git-upload-pack-result)m'; | $stdout_regexp = '(^Content-Type: application/x-git-upload-pack-result)m'; | ||||
| $stderr_regexp = '(The remote end hung up unexpectedly)'; | |||||
| $has_pack = preg_match($stdout_regexp, $stdout); | $has_pack = preg_match($stdout_regexp, $stdout); | ||||
| $is_hangup = preg_match($stderr_regexp, $stderr); | $has_flush_packet = substr($stdout, -5) == "\n0000"; | ||||
epriestley: For clarity and consistency in this codebase, prefer explicit parentheses when using the result… | |||||
epriestleyUnsubmitted Not Done Inline ActionsFor correctness, prefer === over ==. This should also be a lint rule, but would be ambitious at this point, see also "Increase Strictness" in T2312. epriestley: For correctness, prefer `===` over `==`. This should //also// be a lint rule, but would be… | |||||
epriestleyUnsubmitted Not Done Inline Actions$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. epriestley: `$stdout` may be fewer than 5 characters long. In PHP 7 and newer, `substr()` returns `""` in… | |||||
epriestleyUnsubmitted Not Done Inline ActionsThe 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. epriestley: The byte stream has "<length><raw bytes>" sections and //may// naturally contain `\n0000`… | |||||
epriestleyUnsubmitted Not Done Inline ActionsThere 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. epriestley: There is no guarantee that the frame prior to the 0-length frame terminates with "\n". Locally… | |||||
| return $has_pack && $is_hangup; | return $has_pack && $has_flush_packet; | ||||
| } | } | ||||
| private function getCommonEnvironment(PhabricatorUser $viewer) { | private function getCommonEnvironment(PhabricatorUser $viewer) { | ||||
| Context not available. | |||||
For clarity and consistency in this codebase, prefer explicit parentheses when using the result of an expression as a value. That is, this:
...is preferable to:
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)).