Improve regexp to detect duplicate folders when repairing unmerged shares

lib/private/Repair/RepairUnmergedShares.php

@@ -148,6 +148,10 @@ private function getSharesWithUser($shareType, $shareWiths) {
 		return $groupedShares;
 	}
 
+	private function isPotentialDuplicateName($name) {
+		return (preg_match('/\(\d+\)(\.[^\.]+)?$/', $name) === 1);
+	}
+
 	/**
 	 * Decide on the best target name based on all group shares and subshares,
 	 * goal is to increase the likeliness that the chosen name matches what
@@ -169,18 +173,12 @@ private function findBestTargetName($groupShares, $subShares) {
 		$pickedShare = null;
 		// sort by stime, this also properly sorts the direct user share if any
 		@usort($subShares, function($a, $b) {
-			if ($a['stime'] < $b['stime']) {
-				return -1;
-			} else if ($a['stime'] > $b['stime']) {
-				return 1;
-			}
-
-			return 0;
+			return ((int)$a['stime'] - (int)$b['stime']);
 		});
 
 		foreach ($subShares as $subShare) {
 			// skip entries that have parenthesis with numbers
-			if (preg_match('/\([0-9]*\)/', $subShare['file_target']) === 1) {
+			if ($this->isPotentialDuplicateName($subShare['file_target'])) {
 				continue;
 			}
 			// pick any share found that would match, the last being the most recent

tests/lib/Repair/RepairUnmergedSharesTest.php

@@ -28,6 +28,8 @@
 use OCP\Migration\IRepairStep;
 use Test\TestCase;
 use OC\Share20\DefaultShareProvider;
+use OCP\IUserManager;
+use OCP\IGroupManager;
 
 /**
  * Tests for repairing invalid shares
@@ -47,6 +49,12 @@ class RepairUnmergedSharesTest extends TestCase {
 	/** @var int */
 	private $lastShareTime;
 
+	/** @var IUserManager */
+	private $userManager;
+
+	/** @var IGroupManager */
+	private $groupManager;
+
 	protected function setUp() {
 		parent::setUp();
 
@@ -61,45 +69,14 @@ protected function setUp() {
 		$this->connection = \OC::$server->getDatabaseConnection();
 		$this->deleteAllShares();
 
-		$user1 = $this->getMock('\OCP\IUser');
-		$user1->expects($this->any())
-			->method('getUID')
-			->will($this->returnValue('user1'));
-
-		$user2 = $this->getMock('\OCP\IUser');
-		$user2->expects($this->any())
-			->method('getUID')
-			->will($this->returnValue('user2'));
-
-		$users = [$user1, $user2];
-
-		$groupManager = $this->getMock('\OCP\IGroupManager');
-		$groupManager->expects($this->any())
-			->method('getUserGroupIds')
-			->will($this->returnValueMap([
-				// owner
-				[$user1, ['samegroup1', 'samegroup2']],
-				// recipient
-				[$user2, ['recipientgroup1', 'recipientgroup2']],
-			]));
-
-		$userManager = $this->getMock('\OCP\IUserManager');
-		$userManager->expects($this->once())
-			->method('countUsers')
-			->will($this->returnValue([2]));
-		$userManager->expects($this->once())
-			->method('callForAllUsers')
-			->will($this->returnCallback(function(\Closure $closure) use ($users) {
-				foreach ($users as $user) {
-					$closure($user);
-				}
-			}));
+		$this->userManager = $this->getMock('\OCP\IUserManager');
+		$this->groupManager = $this->getMock('\OCP\IGroupManager');
 
 		// used to generate incremental stimes
 		$this->lastShareTime = time();
 
 		/** @var \OCP\IConfig $config */
-		$this->repair = new RepairUnmergedShares($config, $this->connection, $userManager, $groupManager);
+		$this->repair = new RepairUnmergedShares($config, $this->connection, $this->userManager, $this->groupManager);
 	}
 
 	protected function tearDown() {
@@ -510,6 +487,38 @@ public function sharesDataProvider() {
 	 * @dataProvider sharesDataProvider
 	 */
 	public function testMergeGroupShares($shares, $expectedShares) {
+		$user1 = $this->getMock('\OCP\IUser');
+		$user1->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('user1'));
+
+		$user2 = $this->getMock('\OCP\IUser');
+		$user2->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('user2'));
+
+		$users = [$user1, $user2];
+
+		$this->groupManager->expects($this->any())
+			->method('getUserGroupIds')
+			->will($this->returnValueMap([
+				// owner
+				[$user1, ['samegroup1', 'samegroup2']],
+				// recipient
+				[$user2, ['recipientgroup1', 'recipientgroup2']],
+			]));
+
+		$this->userManager->expects($this->once())
+			->method('countUsers')
+			->will($this->returnValue([2]));
+		$this->userManager->expects($this->once())
+			->method('callForAllUsers')
+			->will($this->returnCallback(function(\Closure $closure) use ($users) {
+				foreach ($users as $user) {
+					$closure($user);
+				}
+			}));
+
 		$shareIds = [];
 
 		foreach ($shares as $share) {
@@ -536,5 +545,30 @@ public function testMergeGroupShares($shares, $expectedShares) {
 			$this->assertEquals($expectedShare[1], $share['permissions']);
 		}
 	}
+
+	public function duplicateNamesProvider() {
+		return [
+			// matching
+			['filename (1).txt', true],
+			['folder (2)', true],
+			['filename (1)(2).txt', true],
+			// non-matching
+			['filename ().txt', false],
+			['folder ()', false],
+			['folder (1x)', false],
+			['folder (x1)', false],
+			['filename (a)', false],
+			['filename (1).', false],
+			['filename (1).txt.txt', false],
+			['filename (1)..txt', false],
+		];
+	}
+
+	/**
+	 * @dataProvider duplicateNamesProvider
+	 */
+	public function testIsPotentialDuplicateName($name, $expectedResult) {
+		$this->assertEquals($expectedResult, $this->invokePrivate($this->repair, 'isPotentialDuplicateName', [$name]));
+	}
 }