Produce a more tailored checkout URI for Subversion

Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.

As it is, the URI instructs the user to check out the whole repository.

Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.

Test Plan:
  - Created a remote SVN repository with "Import Only", saw path include it.
  - Verified no "Clone As" options, no "Clone As" in URI.
  - Switched it to hosted, saw "Clone As" options appear and work properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, staticshock

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

src/applications/diffusion/controller/DiffusionRepositoryController.php

@@ -560,10 +560,16 @@ final class DiffusionRepositoryController extends DiffusionController {
           $uri);
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-        $command = csprintf(
+        if ($repository->isHosted()) {
-          'svn checkout %R %R',
+          $command = csprintf(
-          $uri,
+            'svn checkout %R %R',
-          $repository->getCloneName());
+            $uri,
+            $repository->getCloneName());
+        } else {
+          $command = csprintf(
+            'svn checkout %R',
+            $uri);
+        }
         break;
     }
 

src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php

@@ -36,7 +36,10 @@ final class DiffusionRepositoryEditBasicController
       $v_name = $request->getStr('name');
       $v_desc = $request->getStr('description');
       $v_projects = $request->getArr('projectPHIDs');
-      $v_clone_name = $request->getStr('cloneName');
+
+      if ($repository->isHosted()) {
+        $v_clone_name = $request->getStr('cloneName');
+      }
 
       if (!strlen($v_name)) {
         $e_name = pht('Required');
@@ -99,16 +102,22 @@ final class DiffusionRepositoryEditBasicController
           ->setName('name')
           ->setLabel(pht('Name'))
           ->setValue($v_name)
-          ->setError($e_name))
+          ->setError($e_name));
-      ->appendChild(
+
-        id(new AphrontFormTextControl())
+    if ($repository->isHosted()) {
-          ->setName('cloneName')
+      $form
-          ->setLabel(pht('Clone/Checkout As'))
+        ->appendChild(
-          ->setValue($v_clone_name)
+          id(new AphrontFormTextControl())
-          ->setCaption(
+            ->setName('cloneName')
-            pht(
+            ->setLabel(pht('Clone/Checkout As'))
-              'Optional directory name to use when cloning or checking out '.
+            ->setValue($v_clone_name)
-              'this repository.')))
+            ->setCaption(
+              pht(
+                'Optional directory name to use when cloning or checking out '.
+                'this repository.')));
+    }
+
+    $form
       ->appendChild(
         id(new PhabricatorRemarkupControl())
           ->setName('description')

src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php

@@ -256,11 +256,13 @@ final class DiffusionRepositoryEditMainController
 
     $clone_name = $repository->getDetail('clone-name');
 
-    $view->addProperty(
+    if ($repository->isHosted()) {
-      pht('Clone/Checkout As'),
+      $view->addProperty(
-      $clone_name
+        pht('Clone/Checkout As'),
-        ? $clone_name.'/'
+        $clone_name
-        : phutil_tag('em', array(), $repository->getCloneName().'/'));
+          ? $clone_name.'/'
+          : phutil_tag('em', array(), $repository->getCloneName().'/'));
+    }
 
     $project_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
       $repository->getPHID(),

src/applications/repository/storage/PhabricatorRepository.php

@@ -789,7 +789,28 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
    */
   public function getCloneURIObject() {
     if (!$this->isHosted()) {
-      return $this->getRemoteURIObject();
+      if ($this->isSVN()) {
+        // Make sure we pick up the "Import Only" path for Subversion, so
+        // the user clones the repository starting at the correct path, not
+        // from the root.
+        $base_uri = $this->getSubversionBaseURI();
+        $base_uri = new PhutilURI($base_uri);
+        $path = $base_uri->getPath();
+        if (!$path) {
+          $path = '/';
+        }
+
+        // If the trailing "@" is not required to escape the URI, strip it for
+        // readability.
+        if (!preg_match('/@.*@/', $path)) {
+          $path = rtrim($path, '@');
+        }
+
+        $base_uri->setPath($path);
+        return $base_uri;
+      } else {
+        return $this->getRemoteURIObject();
+      }
     }
 
     // Choose the best URI: pick a read/write URI over a URI which is not

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

@@ -90,6 +90,19 @@ final class PhabricatorRepositoryURITestCase
     $this->assertEqual('git@example.com:path.git',
       $repo->getRemoteURIEnvelope()->openEnvelope());
 
+    // Test SVN "Import Only" paths.
+
+    $repo->setDetail('remote-uri', 'http://example.com/');
+    $repo->setVersionControlSystem($svn);
+    $repo->setDetail('svn-subpath', 'projects/example/');
+
+    $this->assertEqual('http://example.com/', $repo->getRemoteURI());
+    $this->assertEqual(
+      'http://example.com/projects/example/',
+      $repo->getPublicCloneURI());
+    $this->assertEqual('http://example.com/',
+      $repo->getRemoteURIEnvelope()->openEnvelope());
+
   }
 
 }