r88960 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r88959‎ | r88960 | r88961 >
Date:08:45, 27 May 2011
Author:happydog
Status:ok (Comments)
Tags:
Comment:
[CodeReview] Follow-up to r83345. I've updated svnImport.php to use CodeRepository::getDiffErrorMessage() when reporting the diff caching result, as it was basically duplicating the code from that function inline.

I've also tweaked getDiffErrorMessage() in the following ways:
* We now allow for the fact that the argument might actually be a valid diff, and in these cases we return an empty string (though I've added a TODO, for someone to check, as this may not be the best value to return).
* I've updated the function argument name to reflect the fact that it's not necessarily an error code any more.
* Added a case statement to handle DIFFRESULT_NothingToCompare, which was missing.
* Updated the strings to use the versions from svnImport, as these were a little more informative.
* Changed order of varname/type in documentation, as per wiki (http://www.mediawiki.org/wiki/Manual:Coding_conventions#Inline), due to Doxygen bug (https://bugzilla.gnome.org/show_bug.cgi?id=613185), and updated.
Modified paths:
  • /trunk/extensions/CodeReview/backend/CodeRepository.php (modified) (history)
  • /trunk/extensions/CodeReview/svnImport.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/backend/CodeRepository.php
——@@ -523,21 +523,31 @@
524524
525525 /**
526526 * @static
527 - * @param int $error
528 - * @return string
 527+ * @param $diff int (error code) or string (diff text), as returned from getDiff()
 528+ * @return string (error message, or empty string if valid diff)
529529 */
530 - public static function getDiffErrorMessage( $error ) {
531 - switch( $error ) {
532 - case self::DIFFRESULT_BadRevision:
533 - return 'Bad revision specified.';
534 - case self::DIFFRESULT_TooManyPaths:
535 - return 'Too many paths returned to diff';
536 - case self::DIFFRESULT_NoDataReturned:
537 - return 'No data returned for diff';
538 - case self::DIFFRESULT_NotInCache:
539 - return 'Not in cache';
540 - default:
541 - return 'Unknown';
 530+ public static function getDiffErrorMessage( $diff ) {
 531+ global $wgCodeReviewMaxDiffPaths;
 532+
 533+ if ( is_integer( $diff ) ) {
 534+ switch( $diff ) {
 535+ case self::DIFFRESULT_BadRevision:
 536+ return 'Bad revision';
 537+ case self::DIFFRESULT_NothingToCompare:
 538+ return 'Nothing to compare';
 539+ case self::DIFFRESULT_TooManyPaths:
 540+ return 'Too many paths ($wgCodeReviewMaxDiffPaths = '
 541+ . $wgCodeReviewMaxDiffPaths . ')';
 542+ case self::DIFFRESULT_NoDataReturned:
 543+ return 'No data returned - no diff data, or connection lost';
 544+ case self::DIFFRESULT_NotInCache:
 545+ return 'Not in cache';
 546+ default:
 547+ return 'Unknown reason!';
 548+ }
542549 }
 550+
 551+ // TODO: Should this return "", $diff or a message string, e.g. "OK"?
 552+ return "";
543553 }
544554 }
Index: trunk/extensions/CodeReview/svnImport.php
——@@ -50,7 +50,7 @@
5151 * @param $start Int Revision to begin the import from (Default: null, means last stored revision);
5252 */
5353 private function importRepo( $repoName, $start = null, $cacheSize = 0 ) {
54 - global $wgCodeReviewImportBatchSize, $wgCodeReviewMaxDiffPaths;
 54+ global $wgCodeReviewImportBatchSize;
5555
5656 $repo = CodeRepository::newFromName( $repoName );
5757
——@@ -154,25 +154,7 @@
155155 $diff = $repo->getDiff( $row->cr_id ); // trigger caching
156156 $msg = "Diff r{$row->cr_id} ";
157157 if ( is_integer( $diff ) ) {
158 - $msg .= "Skipped: ";
159 - switch ($diff) {
160 - case CodeRepository::DIFFRESULT_BadRevision:
161 - $msg .= "Bad revision";
162 - break;
163 - case CodeRepository::DIFFRESULT_NothingToCompare:
164 - $msg .= "Nothing to compare";
165 - break;
166 - case CodeRepository::DIFFRESULT_TooManyPaths:
167 - $msg .= "Too many paths (\$wgCodeReviewMaxDiffPaths = "
168 - . $wgCodeReviewMaxDiffPaths . ")";
169 - break;
170 - case CodeRepository::DIFFRESULT_NoDataReturned:
171 - $msg .= "No data returned - no diff data, or connection lost.";
172 - break;
173 - default:
174 - $msg .= "Unknown reason!";
175 - break;
176 - }
 158+ $msg .= "Skipped: " . CodeRepository::getDiffErrorMessage( $diff );
177159 } else {
178160 $msg .= "done";
179161 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r83345Give a bit more information about why diff failedreedy00:45, 6 March 2011

Comments

#Comment by Nikerabbit (talk | contribs)   11:10, 27 May 2011

Why did you add a todo?

#Comment by HappyDog (talk | contribs)   12:15, 27 May 2011

I reckon there's a use case for returning $diff, e.g. if we updated the code so we store the error-codes in the DB (which might be a good idea, to stop repeated requests for revisions that are no good) then we can output a diff simply by calling this function: If it's a valid diff, you get the diff output, and if not you get the appropriate error string.

But then I thought that might mess up in other places, so I used "OK" to report that the diff was alright.

Then I decided that a blank string was better, so it could be checked for and handled in the appropriate manner, depending on the calling function.

I added the TODO, because I'm really not sure if that's the right decision, and would appreciate some input.

Status & tagging log