| Index: trunk/extensions/CodeReview/CodeReview.php |
| — | — | @@ -114,6 +114,12 @@ |
| 115 | 115 | |
| 116 | 116 | $wgGroupPermissions['steward']['repoadmin'] = true; // temp |
| 117 | 117 | |
| | 118 | +// Constants returned from CodeRepository::getDiff() when no diff can be calculated. |
| | 119 | +define("DIFFRESULT_BadRevision", 0); |
| | 120 | +define("DIFFRESULT_NothingToCompare", 1); |
| | 121 | +define("DIFFRESULT_TooManyPaths", 2); |
| | 122 | +define("DIFFRESULT_NoDataReturned", 3); |
| | 123 | + |
| 118 | 124 | // If you can't directly access the remote SVN repo, you can set this |
| 119 | 125 | // to an offsite proxy running this fun little proxy tool: |
| 120 | 126 | // http://svn.wikimedia.org/viewvc/mediawiki/trunk/tools/codereview-proxy/ |
| Index: trunk/extensions/CodeReview/backend/CodeRepository.php |
| — | — | @@ -259,29 +259,54 @@ |
| 260 | 260 | * @param int $rev Revision ID |
| 261 | 261 | * @param $useCache 'skipcache' to avoid caching |
| 262 | 262 | * 'cached' to *only* fetch if cached |
| | 263 | + * @return string|int The diff text on success, a DIFFRESULT_* constant on failure. |
| 263 | 264 | */ |
| 264 | 265 | public function getDiff( $rev, $useCache = '' ) { |
| 265 | | - global $wgMemc; |
| | 266 | + global $wgMemc, $wgCodeReviewMaxDiffPaths; |
| 266 | 267 | wfProfileIn( __METHOD__ ); |
| 267 | 268 | |
| | 269 | + $data = null; |
| | 270 | + |
| 268 | 271 | $rev1 = $rev - 1; |
| 269 | 272 | $rev2 = $rev; |
| 270 | 273 | |
| | 274 | + // Check that a valid revision was specified. |
| 271 | 275 | $revision = $this->getRevision( $rev ); |
| 272 | | - if ( $revision == null || !$revision->isDiffable() ) { |
| | 276 | + if ( $revision == null ) { |
| | 277 | + $data = DIFFRESULT_BadRevision; |
| | 278 | + } |
| | 279 | + // Check that there is at least one, and at most $wgCodeReviewMaxDiffPaths |
| | 280 | + // paths changed in this revision. |
| | 281 | + else { |
| | 282 | + $paths = $revision->getModifiedPaths(); |
| | 283 | + if ( !$paths->numRows() ) { |
| | 284 | + $data = DIFFRESULT_NothingToCompare; |
| | 285 | + } |
| | 286 | + elseif ( $wgCodeReviewMaxDiffPaths > 0 && $paths->numRows() > $wgCodeReviewMaxDiffPaths ) { |
| | 287 | + $data = DIFFRESULT_TooManyPaths; |
| | 288 | + } |
| | 289 | + } |
| | 290 | + |
| | 291 | + // If an error has occurred, return it. |
| | 292 | + if ( $data !== null ) { |
| 273 | 293 | wfProfileOut( __METHOD__ ); |
| 274 | | - return false; |
| | 294 | + return $data; |
| 275 | 295 | } |
| 276 | 296 | |
| 277 | | - # Try memcached... |
| | 297 | + // Set up the cache key, which will be used both to check if already in the |
| | 298 | + // cache, and to write the final result to the cache. |
| 278 | 299 | $key = wfMemcKey( 'svn', md5( $this->path ), 'diff', $rev1, $rev2 ); |
| | 300 | + |
| | 301 | + // If not set to explicitly skip the cache, get the current diff from memcached |
| | 302 | + // directly. |
| 279 | 303 | if ( $useCache === 'skipcache' ) { |
| 280 | 304 | $data = null; |
| 281 | 305 | } else { |
| 282 | 306 | $data = $wgMemc->get( $key ); |
| 283 | 307 | } |
| 284 | 308 | |
| 285 | | - # Try the database... |
| | 309 | + // If the diff hasn't already been retrieved from the cache, see if we can get |
| | 310 | + // it from the DB. |
| 286 | 311 | if ( !$data && $useCache != 'skipcache' ) { |
| 287 | 312 | $dbr = wfGetDB( DB_SLAVE ); |
| 288 | 313 | $row = $dbr->selectRow( 'code_rev', |
| — | — | @@ -292,7 +317,7 @@ |
| 293 | 318 | if ( $row ) { |
| 294 | 319 | $flags = explode( ',', $row->cr_flags ); |
| 295 | 320 | $data = $row->cr_diff; |
| 296 | | - // If the text was fetched without an error, convert it |
| | 321 | + // If the text was fetched without an error, convert it |
| 297 | 322 | if ( $data !== false && in_array( 'gzip', $flags ) ) { |
| 298 | 323 | # Deal with optional compression of archived pages. |
| 299 | 324 | # This can be done periodically via maintenance/compressOld.php, and |
| — | — | @@ -302,22 +327,36 @@ |
| 303 | 328 | } |
| 304 | 329 | } |
| 305 | 330 | |
| 306 | | - # Generate the diff as needed... |
| | 331 | + // If the data was not already in the cache or in the DB, we need to retrieve |
| | 332 | + // it from SVN. |
| 307 | 333 | if ( !$data && $useCache !== 'cached' ) { |
| 308 | 334 | $svn = SubversionAdaptor::newFromRepo( $this->path ); |
| 309 | 335 | $data = $svn->getDiff( '', $rev1, $rev2 ); |
| | 336 | + |
| | 337 | + // If $data is blank, report the error that no data was returned. |
| | 338 | + // TODO: Currently we can't tell the difference between an SVN/connection |
| | 339 | + // failure and an empty diff. See if we can remedy this! |
| | 340 | + if ($data == "") { |
| | 341 | + $data = DIFFRESULT_NoDataReturned; |
| | 342 | + } |
| | 343 | + // Otherwise, store the resulting diff to both the temporary cache and |
| | 344 | + // permanent DB storage. |
| | 345 | + else { |
| 310 | 346 | // Store to cache |
| 311 | | - $wgMemc->set( $key, $data, 3600 * 24 * 3 ); |
| | 347 | + $wgMemc->set( $key, $data, 3600 * 24 * 3 ); |
| | 348 | + |
| 312 | 349 | // Permanent DB storage |
| 313 | | - $storedData = $data; |
| 314 | | - $flags = Revision::compressRevisionText( $storedData ); |
| 315 | | - $dbw = wfGetDB( DB_MASTER ); |
| 316 | | - $dbw->update( 'code_rev', |
| 317 | | - array( 'cr_diff' => $storedData, 'cr_flags' => $flags ), |
| 318 | | - array( 'cr_repo_id' => $this->id, 'cr_id' => $rev ), |
| 319 | | - __METHOD__ |
| 320 | | - ); |
| | 350 | + $storedData = $data; |
| | 351 | + $flags = Revision::compressRevisionText( $storedData ); |
| | 352 | + $dbw = wfGetDB( DB_MASTER ); |
| | 353 | + $dbw->update( 'code_rev', |
| | 354 | + array( 'cr_diff' => $storedData, 'cr_flags' => $flags ), |
| | 355 | + array( 'cr_repo_id' => $this->id, 'cr_id' => $rev ), |
| | 356 | + __METHOD__ |
| | 357 | + ); |
| | 358 | + } |
| 321 | 359 | } |
| | 360 | + |
| 322 | 361 | wfProfileOut( __METHOD__ ); |
| 323 | 362 | return $data; |
| 324 | 363 | } |
| Index: trunk/extensions/CodeReview/svnImport.php |
| — | — | @@ -50,7 +50,7 @@ |
| 51 | 51 | * @param $start Int Revision to begin the import from (Default: null, means last stored revision); |
| 52 | 52 | */ |
| 53 | 53 | private function importRepo( $repoName, $start = null, $cacheSize = 0 ) { |
| 54 | | - global $wgCodeReviewImportBatchSize; |
| | 54 | + global $wgCodeReviewImportBatchSize, $wgCodeReviewMaxDiffPaths; |
| 55 | 55 | |
| 56 | 56 | $repo = CodeRepository::newFromName( $repoName ); |
| 57 | 57 | |
| — | — | @@ -136,6 +136,14 @@ |
| 137 | 137 | $options['LIMIT'] = $cacheSize; |
| 138 | 138 | } |
| 139 | 139 | |
| | 140 | + // Get all rows for this repository that don't already have a diff filled in. |
| | 141 | + // This is LIMITed according to the $cacheSize setting, above, so only the |
| | 142 | + // rows that we plan to pre-cache are returned. |
| | 143 | + // TODO: This was optimised in order to skip rows that already have a diff, |
| | 144 | + // which is mostly what is required, but there may be situations where |
| | 145 | + // you want to re-calculate diffs (e.g. if $wgCodeReviewMaxDiffPaths |
| | 146 | + // changes). If these situations arise we will either want to revert |
| | 147 | + // this behaviour, or add a --force flag or something. |
| 140 | 148 | $res = $dbw->select( 'code_rev', 'cr_id', |
| 141 | 149 | array( 'cr_repo_id' => $repo->getId(), 'cr_diff IS NULL OR cr_diff = ""' ), |
| 142 | 150 | __METHOD__, |
| — | — | @@ -143,8 +151,33 @@ |
| 144 | 152 | ); |
| 145 | 153 | foreach ( $res as $row ) { |
| 146 | 154 | $repo->getRevision( $row->cr_id ); |
| 147 | | - $repo->getDiff( $row->cr_id ); // trigger caching |
| 148 | | - $this->output( "Diff r{$row->cr_id} done\n" ); |
| | 155 | + $diff = $repo->getDiff( $row->cr_id ); // trigger caching |
| | 156 | + $msg = "Diff r{$row->cr_id} "; |
| | 157 | + if (is_integer($diff)) { |
| | 158 | + $msg .= "Skipped: "; |
| | 159 | + switch ($diff) { |
| | 160 | + case DIFFRESULT_BadRevision: |
| | 161 | + $msg .= "Bad revision"; |
| | 162 | + break; |
| | 163 | + case DIFFRESULT_NothingToCompare: |
| | 164 | + $msg .= "Nothing to compare"; |
| | 165 | + break; |
| | 166 | + case DIFFRESULT_TooManyPaths: |
| | 167 | + $msg .= "Too many paths (\$wgCodeReviewMaxDiffPaths = " |
| | 168 | + . $wgCodeReviewMaxDiffPaths . ")"; |
| | 169 | + break; |
| | 170 | + case DIFFRESULT_NoDataReturned: |
| | 171 | + $msg .= "No data returned - no diff data, or connection lost."; |
| | 172 | + break; |
| | 173 | + default: |
| | 174 | + $msg .= "Unknown reason!"; |
| | 175 | + break; |
| | 176 | + } |
| | 177 | + } |
| | 178 | + else { |
| | 179 | + $msg .= "done"; |
| | 180 | + } |
| | 181 | + $this->output( $msg . "\n" ); |
| 149 | 182 | } |
| 150 | 183 | } |
| 151 | 184 | else { |
| Index: trunk/extensions/CodeReview/api/ApiCodeDiff.php |
| — | — | @@ -40,7 +40,7 @@ |
| 41 | 41 | |
| 42 | 42 | $diff = $repo->getDiff( $params['rev'] ); |
| 43 | 43 | |
| 44 | | - if ( strval( $diff ) === '' ) { |
| | 44 | + if ( !is_string( $diff ) ) { |
| 45 | 45 | // FIXME: Are we sure we don't want to throw an error here? |
| 46 | 46 | $html = 'Failed to load diff.'; |
| 47 | 47 | } elseif ( strlen( $diff ) > $wgCodeReviewMaxDiffSize ) { |
| Index: trunk/extensions/CodeReview/ui/CodeRevisionView.php |
| — | — | @@ -365,7 +365,7 @@ |
| 366 | 366 | $cache = ''; |
| 367 | 367 | } |
| 368 | 368 | $diff = $this->mRepo->getDiff( $this->mRev->getId(), $cache ); |
| 369 | | - if ( !$diff && $deferDiffs ) { |
| | 369 | + if ( is_integer($diff) && $deferDiffs ) { |
| 370 | 370 | // We'll try loading it by AJAX... |
| 371 | 371 | return $this->stubDiffLoader(); |
| 372 | 372 | } elseif ( strlen( $diff ) > $wgCodeReviewMaxDiffSize ) { |