From 395c5907ff6191afa263e557ce44164e032e15b6 Mon Sep 17 00:00:00 2001
From: Bryan Tong Minh <btongminh@users.mediawiki.org>
Date: Fri, 4 Mar 2011 21:57:23 +0000
Subject: [PATCH] Fix and make file moving better resilient against errors by
 first copying the files, then doing the db updates and finally cleaning up
 the source. * FsRepo::cleanupBatch now accepts either pairs, virtual urls and
 real paths * Add missing begin() and commit() to LocalFileMoveBatch *
 Actually set an error after incrementing failCount because otherwise the
 status is still good * Changed some isOk() to isGood() and set ->ok
 explicitly to false after rollback

---
 includes/filerepo/FSRepo.php    | 28 ++++++++++-----
 includes/filerepo/LocalFile.php | 61 +++++++++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/includes/filerepo/FSRepo.php b/includes/filerepo/FSRepo.php
index d992986969..b7b066e267 100644
--- a/includes/filerepo/FSRepo.php
+++ b/includes/filerepo/FSRepo.php
@@ -262,16 +262,28 @@ class FSRepo extends FileRepo {
 	}
 	
 	/**
-	 * Deletes a batch of (zone, rel) pairs. It will try to delete each pair,
-	 * but ignores any errors doing so.
+	 * Deletes a batch of files. Each file can be a (zone, rel) pairs, a
+	 * virtual url or a real path. It will try to delete each file, but 
+	 * ignores any errors that may occur
 	 * 
-	 * @param $pairs array Pair of (zone, rel) pairs to delete
+	 * @param $pairs array List of files to delete
 	 */
-	function cleanupBatch( $pairs ) {
-		foreach ( $pairs as $pair ) {
-			list( $zone, $rel ) = $pair;
-			$root = $this->getZonePath( $zone );
-			$path = "$root/$rel";
+	function cleanupBatch( $files ) {
+		foreach ( $files as $file ) {
+			if ( is_array( $file ) ) {
+				// This is a pair, extract it
+				list( $zone, $rel ) = $file;
+				$root = $this->getZonePath( $zone );
+				$path = "$root/$rel";
+			} else {
+				if ( self::isVirtualUrl( $file ) ) {
+					// This is a virtual url, resolve it 
+					$path = $this->resolveVirtualUrl( $file );
+				} else {
+					// This is a full file name
+					$path = $file;
+				}
+			}
 			
 			wfSuppressWarnings();
 			unlink( $path );
diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php
index ebc0efef78..6e57ca08bf 100644
--- a/includes/filerepo/LocalFile.php
+++ b/includes/filerepo/LocalFile.php
@@ -2043,16 +2043,32 @@ class LocalFileMoveBatch {
 		$triplets = $this->getMoveTriplets();
 
 		$triplets = $this->removeNonexistentFiles( $triplets );
-		$statusDb = $this->doDBUpdates();
-		wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
-		$statusMove = $repo->storeBatch( $triplets, FSRepo::DELETE_SOURCE );
+		
+		// Copy the files into their new location
+		$statusMove = $repo->storeBatch( $triplets );
 		wfDebugLog( 'imagemove', "Moved files for {$this->file->name}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
-
-		if ( !$statusMove->isOk() ) {
+		if ( !$statusMove->isGood() ) {
 			wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
-			$this->db->rollback();
+			$this->cleanupTarget( $triplets );
+			$statusMove->ok = false;
+			return $statusMove;
 		}
 
+		$this->db->begin();
+		$statusDb = $this->doDBUpdates();
+		wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
+		if ( !$statusDb->isGood() ) {
+			$this->db->rollback();
+			// Something went wrong with the DB updates, so remove the target files
+			$this->cleanupTarget( $triplets );
+			$statusDb->ok = false;
+			return $statusDb;
+		}
+		$this->db->commit();
+		
+		// Everything went ok, remove the source files
+		$this->cleanupSource( $triplets );
+		
 		$status->merge( $statusDb );
 		$status->merge( $statusMove );
 
@@ -2082,6 +2098,8 @@ class LocalFileMoveBatch {
 			$status->successCount++;
 		} else {
 			$status->failCount++;
+			$status->fatal( 'imageinvalidfilename' );
+			return $status;
 		}
 
 		// Update old images
@@ -2099,6 +2117,9 @@ class LocalFileMoveBatch {
 		$total = $this->oldCount;
 		$status->successCount += $affected;
 		$status->failCount += $total - $affected;
+		if ( $status->failCount ) {
+			$status->error( 'imageinvalidfilename' );
+		}
 
 		return $status;
 	}
@@ -2143,4 +2164,32 @@ class LocalFileMoveBatch {
 
 		return $filteredTriplets;
 	}
+	
+	/**
+	 * Cleanup a partially moved array of triplets by deleting the target 
+	 * files. Called if something went wrong half way.
+	 */
+	function cleanupTarget( $triplets ) {
+		// Create dest pairs from the triplets
+		$pairs = array();
+		foreach ( $triplets as $triplet ) {
+			$pairs[] = array( $triplet[1], $triplet[2] );
+		}
+		
+		$this->file->repo->cleanupBatch( $pairs );
+	}
+	
+	/**
+	 * Cleanup a fully moved array of triplets by deleting the source files.
+	 * Called at the end of the move process if everything else went ok. 
+	 */
+	function cleanupSource( $triplets ) {
+		// Create source file names from the triplets
+		$files = array();
+		foreach ( $triplets as $triplet ) {
+			$files[] = $triplet[0];
+		}
+		
+		$this->file->repo->cleanupBatch( $files );
+	}
 }
-- 
2.20.1