r62508 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r62507‎ | r62508 | r62509 >
Date:09:59, 15 February 2010
Author:happydog
Status:resolved (Comments)
Tags:
Comment:
CodeReview: Added a canConnect() function to the SubversionAdaptor class. This returns a boolean value indicating whether it is possible to connect to the repository. I have also updated svnImport.php to check this before attempting an update, so more meaningful error messages are output if a connection is not possible (e.g. the network is down).

I have only filled in the contents of this function for the SubversionShell class, as that is the one I am familiar with and am able to test. The other two sub-classes of (SubversionPecl and SubversionProxy) just have dummy functions for now, which always return true. I'll leave it for someone else to fill those in.
Modified paths:
  • /trunk/extensions/CodeReview/backend/Subversion.php (modified) (history)
  • /trunk/extensions/CodeReview/svnImport.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/backend/Subversion.php
@@ -19,6 +19,8 @@
2020 $this->mRepo = $repo;
2121 }
2222
 23+ abstract function canConnect();
 24+
2325 abstract function getFile( $path, $rev = null );
2426
2527 abstract function getDiff( $path, $rev1, $rev2 );
@@ -54,6 +56,12 @@
5557 * Using the SVN PECL extension...
5658 */
5759 class SubversionPecl extends SubversionAdaptor {
 60+
 61+ function canConnect() {
 62+ // TODO!
 63+ return true;
 64+ }
 65+
5866 function getFile( $path, $rev = null ) {
5967 return svn_cat( $this->mRepo . $path, $rev );
6068 }
@@ -94,6 +102,22 @@
95103 * Using the thingy-bobber
96104 */
97105 class SubversionShell extends SubversionAdaptor {
 106+
 107+ function canConnect() {
 108+ $command = sprintf(
 109+ "svn info %s %s",
 110+ $this->getExtraArgs(),
 111+ wfEscapeShellArg( $this->mRepo ) );
 112+
 113+ $Result = wfShellExec( $command );
 114+ if ( $Result == "" )
 115+ return false;
 116+ elseif ( strpos( $Result, "No repository found" ) !== false )
 117+ return false;
 118+ else
 119+ return true;
 120+ }
 121+
98122 function getFile( $path, $rev = null ) {
99123 if ( $rev )
100124 $path .= "@$rev";
@@ -282,6 +306,11 @@
283307 $this->mTimeout = $timeout;
284308 }
285309
 310+ function canConnect() {
 311+ // TODO!
 312+ return true;
 313+ }
 314+
286315 function getFile( $path, $rev = null ) {
287316 throw new MWException( "NYI" );
288317 }
Index: trunk/extensions/CodeReview/svnImport.php
@@ -31,6 +31,10 @@
3232 }
3333
3434 echo "Syncing repo {$args[0]} from r$start to HEAD...\n";
 35+
 36+if ( !$svn->canConnect() )
 37+ die( "Unable to connect to repository.\n" );
 38+
3539 while ( true ) {
3640 $log = $svn->getLog( '', $start, $start + $chunkSize - 1 );
3741 if ( empty( $log ) ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r62816CodeReview: From Tim Starling, in response to r62508 (http://www.mediawiki.or...happydog10:42, 22 February 2010

Comments

#Comment by Tim Starling (talk | contribs)   07:15, 22 February 2010

Variable names should start with a lower-case letter, e.g. "$result" instead of "$Result". Put braces around conditional blocks.

#Comment by HappyDog (talk | contribs)   10:44, 22 February 2010

Fixed in r62816. I notice that the second point (braces) is often not adhered to, so apologies if I didn't pick up on this one.

#Comment by HappyDog (talk | contribs)   10:47, 22 February 2010

btw I'm not sure whether I should be changing status to 'new' or 'resolved' after fixing the above. I suspect 'resolved' and that any further comments will be added to r62816, but am setting to 'new' for now so that someone notices this comment and responds. That way I'll know what to do next time! :-)

#Comment by 😂 (talk | contribs)   10:58, 22 February 2010

In general it's best to leave your statuses for someone else to change, someone will look at it eventually :)

I've set it to resolved now, though.

Status & tagging log