1 files changed, 72 insertions, 39 deletions

diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php
index 9089f4d7..cc23fa31 100644
--- a/includes/filerepo/LocalRepo.php
+++ b/includes/filerepo/LocalRepo.php
@@ -10,19 +10,20 @@
 /**
  * A repository that stores files in the local filesystem and registers them
  * in the wiki's own database. This is the most commonly used repository class.
+ *
  * @ingroup FileRepo
  */
-class LocalRepo extends FSRepo {
-	var $fileFactory = array( 'LocalFile', 'newFromTitle' );
-	var $fileFactoryKey = array( 'LocalFile', 'newFromKey' );
-	var $oldFileFactory = array( 'OldLocalFile', 'newFromTitle' );
-	var $oldFileFactoryKey = array( 'OldLocalFile', 'newFromKey' );
-	var $fileFromRowFactory = array( 'LocalFile', 'newFromRow' );
-	var $oldFileFromRowFactory = array( 'OldLocalFile', 'newFromRow' );
+class LocalRepo extends FileRepo {
+	var $fileFactory           = array( 'LocalFile'   , 'newFromTitle' );
+	var $fileFactoryKey        = array( 'LocalFile'   , 'newFromKey'   );
+	var $fileFromRowFactory    = array( 'LocalFile'   , 'newFromRow'   );
+	var $oldFileFactory        = array( 'OldLocalFile', 'newFromTitle' );
+	var $oldFileFactoryKey     = array( 'OldLocalFile', 'newFromKey'   );
+	var $oldFileFromRowFactory = array( 'OldLocalFile', 'newFromRow'   );
 
 	/**
 	 * @throws MWException
-	 * @param  $row
+	 * @param $row
 	 * @return File
 	 */
 	function newFileFromRow( $row ) {
@@ -55,6 +56,7 @@ class LocalRepo extends FSRepo {
 	 * @return FileRepoStatus
 	 */
 	function cleanupDeletedBatch( $storageKeys ) {
+		$backend = $this->backend; // convenience
 		$root = $this->getZonePath( 'deleted' );
 		$dbw = $this->getMasterDB();
 		$status = $this->newGood();
@@ -63,25 +65,14 @@ class LocalRepo extends FSRepo {
 			$hashPath = $this->getDeletedHashPath( $key );
 			$path = "$root/$hashPath$key";
 			$dbw->begin();
-			$inuse = $dbw->selectField( 'filearchive', '1',
-				array( 'fa_storage_group' => 'deleted', 'fa_storage_key' => $key ),
-				__METHOD__, array( 'FOR UPDATE' ) );
-			if( !$inuse ) {
-				$sha1 = self::getHashFromKey( $key );
-				$ext = substr( $key, strcspn( $key, '.' ) + 1 );
-				$ext = File::normalizeExtension($ext);
-				$inuse = $dbw->selectField( 'oldimage', '1',
-					array( 'oi_sha1' => $sha1,
-						'oi_archive_name ' . $dbw->buildLike( $dbw->anyString(), ".$ext" ),
-						$dbw->bitAnd('oi_deleted', File::DELETED_FILE) => File::DELETED_FILE ),
-					__METHOD__, array( 'FOR UPDATE' ) );
-			}
-			if ( !$inuse ) {
+			// Check for usage in deleted/hidden files and pre-emptively
+			// lock the key to avoid any future use until we are finished.
+			$deleted = $this->deletedFileHasKey( $key, 'lock' );
+			$hidden = $this->hiddenFileHasKey( $key, 'lock' );
+			if ( !$deleted && !$hidden ) { // not in use now
 				wfDebug( __METHOD__ . ": deleting $key\n" );
-				wfSuppressWarnings();
-				$unlink = unlink( $path );
-				wfRestoreWarnings();
-				if ( !$unlink ) {
+				$op = array( 'op' => 'delete', 'src' => $path );
+				if ( !$backend->doOperation( $op )->isOK() ) {
 					$status->error( 'undelete-cleanup-error', $path );
 					$status->failCount++;
 				}
@@ -95,6 +86,45 @@ class LocalRepo extends FSRepo {
 	}
 
 	/**
+	 * Check if a deleted (filearchive) file has this sha1 key
+	 *
+	 * @param $key String File storage key (base-36 sha1 key with file extension)
+	 * @param $lock String|null Use "lock" to lock the row via FOR UPDATE
+	 * @return bool File with this key is in use
+	 */
+	protected function deletedFileHasKey( $key, $lock = null ) {
+		$options = ( $lock === 'lock' ) ? array( 'FOR UPDATE' ) : array();
+
+		$dbw = $this->getMasterDB();
+		return (bool)$dbw->selectField( 'filearchive', '1',
+			array( 'fa_storage_group' => 'deleted', 'fa_storage_key' => $key ),
+			__METHOD__, $options
+		);
+	}
+
+	/**
+	 * Check if a hidden (revision delete) file has this sha1 key
+	 *
+	 * @param $key String File storage key (base-36 sha1 key with file extension)
+	 * @param $lock String|null Use "lock" to lock the row via FOR UPDATE
+	 * @return bool File with this key is in use
+	 */
+	protected function hiddenFileHasKey( $key, $lock = null ) {
+		$options = ( $lock === 'lock' ) ? array( 'FOR UPDATE' ) : array();
+
+		$sha1 = self::getHashFromKey( $key );
+		$ext = File::normalizeExtension( substr( $key, strcspn( $key, '.' ) + 1 ) );
+
+		$dbw = $this->getMasterDB();
+		return (bool)$dbw->selectField( 'oldimage', '1',
+			array( 'oi_sha1' => $sha1,
+				'oi_archive_name ' . $dbw->buildLike( $dbw->anyString(), ".$ext" ),
+				$dbw->bitAnd( 'oi_deleted', File::DELETED_FILE ) => File::DELETED_FILE ),
+			__METHOD__, $options
+		);
+	}
+
+	/**
 	 * Gets the SHA1 hash from a storage key
 	 *
 	 * @param string $key
@@ -108,16 +138,12 @@ class LocalRepo extends FSRepo {
 	 * Checks if there is a redirect named as $title
 	 *
 	 * @param $title Title of file
+	 * @return bool
 	 */
-	function checkRedirect( $title ) {
+	function checkRedirect( Title $title ) {
 		global $wgMemc;
 
-		if( is_string( $title ) ) {
-			$title = Title::newFromText( $title );
-		}
-		if( $title instanceof Title && $title->getNamespace() == NS_MEDIA ) {
-			$title = Title::makeTitle( NS_FILE, $title->getText() );
-		}
+		$title = File::normalizeTitle( $title, 'exception' );
 
 		$memcKey = $this->getSharedCacheKey( 'image_redirect', md5( $title->getDBkey() ) );
 		if ( $memcKey === false ) {
@@ -161,6 +187,7 @@ class LocalRepo extends FSRepo {
 	/**
 	 * Function link Title::getArticleID().
 	 * We can't say Title object, what database it should use, so we duplicate that function here.
+	 *
 	 * @param $title Title
 	 */
 	protected function getArticleID( $title ) {
@@ -169,20 +196,23 @@ class LocalRepo extends FSRepo {
 		}
 		$dbr = $this->getSlaveDB();
 		$id = $dbr->selectField(
-			'page',	// Table
-			'page_id',	//Field
-			array(	//Conditions
+			'page', // Table
+			'page_id',  //Field
+			array(  //Conditions
 				'page_namespace' => $title->getNamespace(),
 				'page_title' => $title->getDBkey(),
 			),
-			__METHOD__	//Function name
+			__METHOD__  //Function name
 		);
 		return $id;
 	}
 
 	/**
-	 * Get an array or iterator of file objects for files that have a given 
+	 * Get an array or iterator of file objects for files that have a given
 	 * SHA-1 content hash.
+	 *
+	 * @param $hash String a sha1 hash to look for
+	 * @return Array
 	 */
 	function findBySha1( $hash ) {
 		$dbr = $this->getSlaveDB();
@@ -219,6 +249,8 @@ class LocalRepo extends FSRepo {
 	 * Get a key on the primary cache for this repository.
 	 * Returns false if the repository's cache is not accessible at this site. 
 	 * The parameters are the parts of the key, as for wfMemcKey().
+	 *
+	 * @return string
 	 */
 	function getSharedCacheKey( /*...*/ ) {
 		$args = func_get_args();
@@ -229,8 +261,9 @@ class LocalRepo extends FSRepo {
 	 * Invalidates image redirect cache related to that image
 	 *
 	 * @param $title Title of page
+	 * @return void
 	 */
-	function invalidateImageRedirect( $title ) {
+	function invalidateImageRedirect( Title $title ) {
 		global $wgMemc;
 		$memcKey = $this->getSharedCacheKey( 'image_redirect', md5( $title->getDBkey() ) );
 		if ( $memcKey ) {