Guarantee repositories have unique local paths

Summary:
Ref T4039. Long ago these were more freely editable and there were some security concerns around creating a repository, then setting its local path to point somewhere it shouldn't.

Local paths are no longer editable so there's no real reason we need to provide a uniqueness guarantee anymore, but you could still make a mistake with `bin/repository move-paths` by accident, and it's a little cleaner to pull them out into their own column with a key.

(We still don't -- and, largely can't -- guarantee that two paths aren't //equivalent// since one might be symlinked to the other, or symlinked only on some hosts, or whatever, but the primary value here is as a sanity check that you aren't goofing things up and pointing a bunch of repositories at the same working copy by mistake.)

Test Plan:
  - Ran migrations.
  - Grepped for `local-path`.
  - Listed and moved paths with `bin/repository`.
  - Created a new repository, verified its local path populated correctly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4039

Differential Revision: https://secure.phabricator.com/D15837

resources/sql/autopatches/20160503.repo.01.lpath.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository
+  ADD localPath VARCHAR(128) COLLATE {$COLLATE_TEXT};

resources/sql/autopatches/20160503.repo.02.lpathkey.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository
+  ADD UNIQUE KEY `key_local` (localPath);

resources/sql/autopatches/20160503.repo.03.lpathmigrate.php

@@ -0,0 +1,57 @@
+<?php
+
+$table = new PhabricatorRepository();
+$conn_w = $table->establishConnection('w');
+
+$default_path = PhabricatorEnv::getEnvConfig('repository.default-local-path');
+$default_path = rtrim($default_path, '/');
+
+foreach (new LiskMigrationIterator($table) as $repository) {
+  $local_path = $repository->getLocalPath();
+  if (strlen($local_path)) {
+    // Repository already has a modern, unique local path.
+    continue;
+  }
+
+  $local_path = $repository->getDetail('local-path');
+  if (!strlen($local_path)) {
+    // Repository does not have a local path using the older format.
+    continue;
+  }
+
+  $random = Filesystem::readRandomCharacters(8);
+
+  // Try the configured path first, then a default path, then a path with some
+  // random noise.
+  $paths = array(
+    $local_path,
+    $default_path.'/'.$repository->getID().'/',
+    $default_path.'/'.$repository->getID().'-'.$random.'/',
+  );
+
+  foreach ($paths as $path) {
+    // Set, then get the path to normalize it.
+    $repository->setLocalPath($path);
+    $path = $repository->getLocalPath();
+
+    try {
+      queryfx(
+        $conn_w,
+        'UPDATE %T SET localPath = %s WHERE id = %d',
+        $table->getTableName(),
+        $path,
+        $repository->getID());
+
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Assigned repository "%s" to local path "%s".',
+          $repository->getDisplayName(),
+          $path));
+
+      break;
+    } catch (AphrontDuplicateKeyQueryException $ex) {
+      // Ignore, try the next one.
+    }
+  }
+}

src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php

@@ -631,9 +631,7 @@ final class DiffusionRepositoryEditMainController
       pht('Storage Service'),
       $v_service);
 
-    $view->addProperty(
+    $view->addProperty(pht('Storage Path'), $repository->getLocalPath());
-      pht('Storage Path'),
-      $repository->getHumanReadableDetail('local-path'));
 
     return $view;
   }

src/applications/diffusion/controller/DiffusionRepositoryEditStorageController.php

@@ -15,7 +15,7 @@ final class DiffusionRepositoryEditStorageController
 
     $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/');
 
-    $v_local = $repository->getHumanReadableDetail('local-path');
+    $v_local = $repository->getLocalPath();
     $errors = array();
 
     $crumbs = $this->buildApplicationCrumbs();
@@ -51,11 +51,7 @@ final class DiffusionRepositoryEditStorageController
       ->appendRemarkupInstructions(
         pht(
           "You can not adjust the local path for this repository from the ".
-          "web interface. To edit it, run this command:\n\n  %s",
+          'web interface.'))
-          sprintf(
-            'phabricator/ $ ./bin/repository edit %s --as %s --local-path ...',
-            $repository->getMonogram(),
-            $viewer->getUsername())))
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->addCancelButton($edit_uri, pht('Done')));

src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php

@@ -40,7 +40,7 @@ final class DiffusionRepositoryStorageManagementPanel
       ->setViewer($viewer);
 
     if ($repository->usesLocalWorkingCopy()) {
-      $storage_path = $repository->getHumanReadableDetail('local-path');
+      $storage_path = $repository->getLocalPath();
     } else {
       $storage_path = phutil_tag('em', array(), pht('No Local Working Copy'));
     }

src/applications/repository/conduit/RepositoryCreateConduitAPIMethod.php

@@ -110,7 +110,6 @@ final class RepositoryCreateConduitAPIMethod
       'description'       => $request->getValue('description'),
       'tracking-enabled'  => (bool)$request->getValue('tracking', true),
       'remote-uri'        => $remote_uri,
-      'local-path'        => $local_path,
       'branch-filter'     => array_fill_keys(
         $request->getValue('branchFilter', array()),
         true),
@@ -128,6 +127,8 @@ final class RepositoryCreateConduitAPIMethod
       $repository->setDetail($key, $value);
     }
 
+    $repository->setLocalPath($local_path);
+
     try {
       $repository->save();
     } catch (AphrontDuplicateKeyQueryException $ex) {

src/applications/repository/editor/PhabricatorRepositoryEditor.php

@@ -86,7 +86,7 @@ final class PhabricatorRepositoryEditor
       case PhabricatorRepositoryTransaction::TYPE_REMOTE_URI:
         return $object->getDetail('remote-uri');
       case PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH:
-        return $object->getDetail('local-path');
+        return $object->getLocalPath();
       case PhabricatorRepositoryTransaction::TYPE_HOSTING:
         return $object->isHosted();
       case PhabricatorRepositoryTransaction::TYPE_PROTOCOL_HTTP:
@@ -209,7 +209,7 @@ final class PhabricatorRepositoryEditor
         $object->setDetail('remote-uri', $xaction->getNewValue());
         break;
       case PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH:
-        $object->setDetail('local-path', $xaction->getNewValue());
+        $object->setLocalPath($xaction->getNewValue());
         break;
       case PhabricatorRepositoryTransaction::TYPE_HOSTING:
         return $object->setHosted($xaction->getNewValue());
@@ -706,7 +706,7 @@ final class PhabricatorRepositoryEditor
 
     // If the repository does not have a local path yet, assign it one based
     // on its ID. We can't do this earlier because we won't have an ID yet.
-    $local_path = $object->getDetail('local-path');
+    $local_path = $object->getLocalPath();
     if (!strlen($local_path)) {
       $local_key = 'repository.default-local-path';
 
@@ -716,7 +716,7 @@ final class PhabricatorRepositoryEditor
       $id = $object->getID();
       $local_path = "{$local_root}/{$id}/";
 
-      $object->setDetail('local-path', $local_path);
+      $object->setLocalPath($local_path);
       $object->save();
     }
 

src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php

@@ -65,7 +65,7 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase {
       ->setCallsign($callsign)
       ->setName(pht('Test Repo "%s"', $callsign))
       ->setVersionControlSystem($vcs_type)
-      ->setDetail('local-path', dirname($local).'/'.$callsign)
+      ->setLocalPath(dirname($local).'/'.$callsign)
       ->setDetail('remote-uri', 'file://'.$dir->getPath().'/');
 
     $this->didConstructRepository($repo);

src/applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php

@@ -21,11 +21,6 @@ final class PhabricatorRepositoryManagementEditWorkflow
             'param' => 'user',
             'help' => pht('Edit as user.'),
           ),
-          array(
-            'name' => 'local-path',
-            'param' => 'path',
-            'help' => pht('Edit the local path.'),
-          ),
           array(
             'name' => 'serve-http',
             'param' => 'string',
@@ -83,7 +78,6 @@ final class PhabricatorRepositoryManagementEditWorkflow
 
       $xactions = array();
 
-      $type_local_path = PhabricatorRepositoryTransaction::TYPE_LOCAL_PATH;
       $type_protocol_http =
         PhabricatorRepositoryTransaction::TYPE_PROTOCOL_HTTP;
       $type_protocol_ssh = PhabricatorRepositoryTransaction::TYPE_PROTOCOL_SSH;
@@ -93,11 +87,6 @@ final class PhabricatorRepositoryManagementEditWorkflow
         PhabricatorRepository::SERVE_READWRITE,
       );
 
-      if ($args->getArg('local-path')) {
-        $xactions[] = id(new PhabricatorRepositoryTransaction())
-          ->setTransactionType($type_local_path)
-          ->setNewValue($args->getArg('local-path'));
-      }
       $serve_http = $args->getArg('serve-http');
       if ($serve_http && in_array($serve_http, $allowed_serve_modes)) {
         $xactions[] = id(new PhabricatorRepositoryTransaction())

src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php

@@ -128,14 +128,12 @@ final class PhabricatorRepositoryManagementMovePathsWorkflow
       }
 
       $repo = $row['repository'];
-      $details = $repo->getDetails();
-      $details['local-path'] = $row['dst'];
 
       queryfx(
         $repo->establishConnection('w'),
-        'UPDATE %T SET details = %s WHERE id = %d',
+        'UPDATE %T SET localPath = %s WHERE id = %d',
         $repo->getTableName(),
-        phutil_json_encode($details),
+        $row['dst'],
         $repo->getID());
     }
 

src/applications/repository/storage/PhabricatorRepository.php

@@ -62,6 +62,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   protected $credentialPHID;
   protected $almanacServicePHID;
   protected $spacePHID;
+  protected $localPath;
 
   private $commitCount = self::ATTACHABLE;
   private $mostRecentCommit = self::ATTACHABLE;
@@ -107,6 +108,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
         'pushPolicy' => 'policy',
         'credentialPHID' => 'phid?',
         'almanacServicePHID' => 'phid?',
+        'localPath' => 'text128?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'callsign' => array(
@@ -123,6 +125,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
           'columns' => array('repositorySlug'),
           'unique' => true,
         ),
+        'key_local' => array(
+          'columns' => array('localPath'),
+          'unique' => true,
+        ),
       ),
     ) + parent::getConfiguration();
   }
@@ -216,6 +222,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return $monograms;
   }
 
+  public function setLocalPath($path) {
+    // Convert any extra slashes ("//") in the path to a single slash ("/").
+    $path = preg_replace('(//+)', '/', $path);
+
+    return parent::setLocalPath($path);
+  }
+
   public function getDetail($key, $default = null) {
     return idx($this->details, $key, $default);
   }
@@ -279,10 +292,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       ));
   }
 
-  public function getLocalPath() {
-    return $this->getDetail('local-path');
-  }
-
   public function getSubversionBaseURI($commit = null) {
     $subpath = $this->getDetail('svn-subpath');
     if (!strlen($subpath)) {

src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php

@@ -70,12 +70,12 @@ final class PhabricatorRepositoryTestCase
 
     $repo->setDetail('hosting-enabled', true);
 
-    $repo->setDetail('local-path', '/var/repo/SVN');
+    $repo->setLocalPath('/var/repo/SVN');
     $this->assertEqual(
       'file:///var/repo/SVN',
       $repo->getSubversionPathURI());
 
-    $repo->setDetail('local-path', '/var/repo/SVN/');
+    $repo->setLocalPath('/var/repo/SVN/');
     $this->assertEqual(
       'file:///var/repo/SVN',
       $repo->getSubversionPathURI());