Support slightly prettier repository URIs in Diffusion

Summary: Fixes T4245. When a repository has a short name, use `/source/shortname/` as its primary URI.

Test Plan:
  - Cloned Git repositories from shortnames via HTTP and SSH.
  - Cloned Mercurial repositories from shortnames via HTTP and SSH.
  - Cloned Subversion repositories from shortnames via SSH.
  - Browsed Git, Mercurial and Subversion repositories.
  - Added and removed short names to various repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

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

src/applications/diffusion/application/PhabricatorDiffusionApplication.php

@@ -46,6 +46,53 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
   }
 
   public function getRoutes() {
+    $repository_routes = array(
+      '/' => array(
+        '' => 'DiffusionRepositoryController',
+        'repository/(?P<dblob>.*)' => 'DiffusionRepositoryController',
+        'change/(?P<dblob>.*)' => 'DiffusionChangeController',
+        'history/(?P<dblob>.*)' => 'DiffusionHistoryController',
+        'browse/(?P<dblob>.*)' => 'DiffusionBrowseController',
+        'lastmodified/(?P<dblob>.*)' => 'DiffusionLastModifiedController',
+        'diff/' => 'DiffusionDiffController',
+        'tags/(?P<dblob>.*)' => 'DiffusionTagListController',
+        'branches/(?P<dblob>.*)' => 'DiffusionBranchTableController',
+        'refs/(?P<dblob>.*)' => 'DiffusionRefTableController',
+        'lint/(?P<dblob>.*)' => 'DiffusionLintController',
+        'commit/(?P<commit>[a-z0-9]+)/branches/'
+          => 'DiffusionCommitBranchesController',
+        'commit/(?P<commit>[a-z0-9]+)/tags/'
+          => 'DiffusionCommitTagsController',
+        'commit/(?P<commit>[a-z0-9]+)/edit/'
+          => 'DiffusionCommitEditController',
+        'manage/(?:(?P<panel>[^/]+)/)?'
+          => 'DiffusionRepositoryManagePanelsController',
+        'uri/' => array(
+          'view/(?P<id>[0-9]\d*)/' => 'DiffusionRepositoryURIViewController',
+          'disable/(?P<id>[0-9]\d*)/'
+            => 'DiffusionRepositoryURIDisableController',
+          $this->getEditRoutePattern('edit/')
+            => 'DiffusionRepositoryURIEditController',
+          'credential/(?P<id>[0-9]\d*)/(?P<action>edit|remove)/'
+            => 'DiffusionRepositoryURICredentialController',
+        ),
+        'edit/' => array(
+          'activate/' => 'DiffusionRepositoryEditActivateController',
+          'dangerous/' => 'DiffusionRepositoryEditDangerousController',
+          'delete/' => 'DiffusionRepositoryEditDeleteController',
+          'update/' => 'DiffusionRepositoryEditUpdateController',
+          'testautomation/' => 'DiffusionRepositoryTestAutomationController',
+        ),
+        'pathtree/(?P<dblob>.*)' => 'DiffusionPathTreeController',
+      ),
+
+      // NOTE: This must come after the rules above; it just gives us a
+      // catch-all for serving repositories over HTTP. We must accept requests
+      // without the trailing "/" because SVN commands don't necessarily
+      // include it.
+      '(?:/.*)?' => 'DiffusionRepositoryDefaultController',
+    );
+
     return array(
       '/(?:'.
         'r(?P<repositoryCallsign>[A-Z]+)'.
@@ -54,6 +101,9 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
       ')(?P<commit>[a-f0-9]+)'
         => 'DiffusionCommitController',
 
+      '/source/(?P<repositoryShortName>[^/.]+)(?P<dotgit>\.git)?'
+        => $repository_routes,
+
       '/diffusion/' => array(
         $this->getQueryRoutePattern()
           => 'DiffusionRepositoryListController',
@@ -63,57 +113,8 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
           '(?:query/(?P<queryKey>[^/]+)/)?' => 'DiffusionPushLogListController',
           'view/(?P<id>\d+)/' => 'DiffusionPushEventViewController',
         ),
-        '(?:'.
+        '(?P<repositoryCallsign>[A-Z]+)' => $repository_routes,
-          '(?P<repositoryCallsign>[A-Z]+)'.
+        '(?P<repositoryID>[1-9]\d*)' => $repository_routes,
-          '|'.
-          '(?P<repositoryID>[1-9]\d*)'.
-        ')/' => array(
-          '' => 'DiffusionRepositoryController',
-
-          'repository/(?P<dblob>.*)'    => 'DiffusionRepositoryController',
-          'change/(?P<dblob>.*)'        => 'DiffusionChangeController',
-          'history/(?P<dblob>.*)'       => 'DiffusionHistoryController',
-          'browse/(?P<dblob>.*)'        => 'DiffusionBrowseController',
-          'lastmodified/(?P<dblob>.*)'  => 'DiffusionLastModifiedController',
-          'diff/'                       => 'DiffusionDiffController',
-          'tags/(?P<dblob>.*)'          => 'DiffusionTagListController',
-          'branches/(?P<dblob>.*)'      => 'DiffusionBranchTableController',
-          'refs/(?P<dblob>.*)'          => 'DiffusionRefTableController',
-          'lint/(?P<dblob>.*)'          => 'DiffusionLintController',
-          'commit/(?P<commit>[a-z0-9]+)/branches/'
-            => 'DiffusionCommitBranchesController',
-          'commit/(?P<commit>[a-z0-9]+)/tags/'
-            => 'DiffusionCommitTagsController',
-          'commit/(?P<commit>[a-z0-9]+)/edit/'
-            => 'DiffusionCommitEditController',
-          'manage/(?:(?P<panel>[^/]+)/)?'
-            => 'DiffusionRepositoryManagePanelsController',
-          'uri/' => array(
-            'view/(?P<id>[0-9]\d*)/' => 'DiffusionRepositoryURIViewController',
-            'disable/(?P<id>[0-9]\d*)/'
-              => 'DiffusionRepositoryURIDisableController',
-            $this->getEditRoutePattern('edit/')
-              => 'DiffusionRepositoryURIEditController',
-            'credential/(?P<id>[0-9]\d*)/(?P<action>edit|remove)/'
-              => 'DiffusionRepositoryURICredentialController',
-          ),
-          'edit/' => array(
-            'activate/' => 'DiffusionRepositoryEditActivateController',
-            'dangerous/' => 'DiffusionRepositoryEditDangerousController',
-            'delete/' => 'DiffusionRepositoryEditDeleteController',
-            'update/' => 'DiffusionRepositoryEditUpdateController',
-            'testautomation/' => 'DiffusionRepositoryTestAutomationController',
-          ),
-          'pathtree/(?P<dblob>.*)' => 'DiffusionPathTreeController',
-        ),
-
-        // NOTE: This must come after the rule above; it just gives us a
-        // catch-all for serving repositories over HTTP. We must accept
-        // requests without the trailing "/" because SVN commands don't
-        // necessarily include it.
-        '(?:(?P<repositoryCallsign>[A-Z]+)|(?P<repositoryID>[1-9]\d*))'.
-          '(?:/.*)?'
-          => 'DiffusionRepositoryDefaultController',
 
         'inline/' => array(
           'edit/(?P<phid>[^/]+)/' => 'DiffusionInlineCommentController',

src/applications/diffusion/controller/DiffusionController.php

@@ -90,6 +90,11 @@ abstract class DiffusionController extends PhabricatorController {
   protected function getRepositoryIdentifierFromRequest(
     AphrontRequest $request) {
 
+    $short_name = $request->getURIData('repositoryShortName');
+    if (strlen($short_name)) {
+      return $short_name;
+    }
+
     $identifier = $request->getURIData('repositoryCallsign');
     if (strlen($identifier)) {
       return $identifier;

src/applications/diffusion/controller/DiffusionLastModifiedController.php

@@ -16,6 +16,7 @@ final class DiffusionLastModifiedController extends DiffusionController {
     $drequest = $this->getDiffusionRequest();
 
     $paths = $request->getStr('paths');
+
     try {
       $paths = phutil_json_decode($paths);
     } catch (PhutilJSONParserException $ex) {

src/applications/diffusion/controller/DiffusionServeController.php

@@ -88,6 +88,13 @@ final class DiffusionServeController extends DiffusionController {
       }
     }
 
+    // If the request was for a path like "/source/libphutil.git" but the
+    // repository is not a Git repository, reject the request.
+    $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;
+    if ($request->getURIData('dotgit') && ($vcs !== $type_git)) {
+      return null;
+    }
+
     return $vcs;
   }
 
@@ -607,7 +614,9 @@ final class DiffusionServeController extends DiffusionController {
     $request = $this->getRequest();
     $request_path = $request->getRequestURI()->getPath();
 
-    $info = PhabricatorRepository::parseRepositoryServicePath($request_path);
+    $info = PhabricatorRepository::parseRepositoryServicePath(
+      $request_path,
+      $repository->getVersionControlSystem());
     $base_path = $info['path'];
 
     // For Git repositories, strip an optional directory component if it

src/applications/diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php

@@ -15,7 +15,9 @@ final class DiffusionGitLFSAuthenticateWorkflow
   }
 
   protected function identifyRepository() {
-    return $this->loadRepositoryWithPath($this->getLFSPathArgument());
+    return $this->loadRepositoryWithPath(
+      $this->getLFSPathArgument(),
+      PhabricatorRepositoryType::REPOSITORY_TYPE_GIT);
   }
 
   private function getLFSPathArgument() {

src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php

@@ -17,7 +17,9 @@ abstract class DiffusionGitSSHWorkflow
   protected function identifyRepository() {
     $args = $this->getArgs();
     $path = head($args->getArg('dir'));
-    return $this->loadRepositoryWithPath($path);
+    return $this->loadRepositoryWithPath(
+      $path,
+      PhabricatorRepositoryType::REPOSITORY_TYPE_GIT);
   }
 
   protected function waitForGitClient() {

src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php

@@ -27,7 +27,9 @@ final class DiffusionMercurialServeSSHWorkflow
   protected function identifyRepository() {
     $args = $this->getArgs();
     $path = $args->getArg('repository');
-    return $this->loadRepositoryWithPath($path);
+    return $this->loadRepositoryWithPath(
+      $path,
+      PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL);
   }
 
   protected function executeRepositoryOperations() {

src/applications/diffusion/ssh/DiffusionSSHWorkflow.php

@@ -161,18 +161,19 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
     }
   }
 
-  protected function loadRepositoryWithPath($path) {
+  protected function loadRepositoryWithPath($path, $vcs) {
     $viewer = $this->getUser();
 
-    $info = PhabricatorRepository::parseRepositoryServicePath($path);
+    $info = PhabricatorRepository::parseRepositoryServicePath($path, $vcs);
     if ($info === null) {
       throw new Exception(
         pht(
-          'Unrecognized repository path "%s". Expected a path like "%s" '.
+          'Unrecognized repository path "%s". Expected a path like "%s", '.
-          'or "%s".',
+          '"%s", or "%s".',
           $path,
           '/diffusion/X/',
-          '/diffusion/123/'));
+          '/diffusion/123/',
+          '/source/thaumaturgy.git'));
     }
 
     $identifier = $info['identifier'];

src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php

@@ -117,7 +117,9 @@ final class DiffusionSubversionServeSSHWorkflow
           $uri = $struct[2]['value'];
           $path = $this->getPathFromSubversionURI($uri);
 
-          return $this->loadRepositoryWithPath($path);
+          return $this->loadRepositoryWithPath(
+            $path,
+            PhabricatorRepositoryType::REPOSITORY_TYPE_SVN);
         }
       }
 

src/applications/repository/storage/PhabricatorRepository.php

@@ -563,6 +563,11 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public function getURI() {
+    $short_name = $this->getRepositorySlug();
+    if (strlen($short_name)) {
+      return "/source/{$short_name}/";
+    }
+
     $callsign = $this->getCallsign();
     if (strlen($callsign)) {
       return "/diffusion/{$callsign}/";
@@ -573,7 +578,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public function getPathURI($path) {
-    return $this->getURI().$path;
+    return $this->getURI().ltrim($path, '/');
   }
 
   public function getCommitURI($identifier) {
@@ -586,14 +591,22 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return "/R{$id}:{$identifier}";
   }
 
-  public static function parseRepositoryServicePath($request_path) {
+  public static function parseRepositoryServicePath($request_path, $vcs) {
+
     // NOTE: In Mercurial over SSH, the path will begin without a leading "/",
     // so we're matching it optionally.
 
+    if ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) {
+      $maybe_git = '(?:\\.git)?';
+    } else {
+      $maybe_git = null;
+    }
+
     $patterns = array(
       '(^'.
-        '(?P<base>/?diffusion/(?P<identifier>[A-Z]+|[0-9]\d*))'.
+        '(?P<base>/?(?:diffusion|source)/(?P<identifier>[^/.]+))'.
-        '(?P<path>(?:/.*)?)'.
+        $maybe_git.
+        '(?P<path>(?:/|.*)?)'.
       '\z)',
     );
 
@@ -624,28 +637,15 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   public function getCanonicalPath($request_path) {
     $standard_pattern =
       '(^'.
-        '(?P<prefix>/diffusion/)'.
+        '(?P<prefix>/(?:diffusion|source)/)'.
         '(?P<identifier>[^/]+)'.
         '(?P<suffix>(?:/.*)?)'.
       '\z)';
 
     $matches = null;
     if (preg_match($standard_pattern, $request_path, $matches)) {
-      $prefix = $matches['prefix'];
-
-      $callsign = $this->getCallsign();
-      if ($callsign) {
-        $identifier = $callsign;
-      } else {
-        $identifier = $this->getID();
-      }
-
       $suffix = $matches['suffix'];
-      if (!strlen($suffix)) {
+      return $this->getPathURI($suffix);
-        $suffix = '/';
-      }
-
-      return $prefix.$identifier.$suffix;
     }
 
     $commit_pattern =
@@ -724,18 +724,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       return $this->getCommitURI($commit);
     }
 
-
-    $identifier = $this->getID();
-
-    $callsign = $this->getCallsign();
-    if ($callsign !== null) {
-      $identifier = $callsign;
-    }
-
-    if (strlen($identifier)) {
-      $identifier = phutil_escape_uri_path_component($identifier);
-    }
-
     if (strlen($path)) {
       $path = ltrim($path, '/');
       $path = str_replace(array(';', '$'), array(';;', '$$'), $path);
@@ -766,13 +754,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       case 'lint':
       case 'pathtree':
       case 'refs':
-        $uri = "/diffusion/{$identifier}/{$action}/{$path}{$commit}{$line}";
+        $uri = $this->getPathURI("/{$action}/{$path}{$commit}{$line}");
         break;
       case 'branch':
         if (strlen($path)) {
-          $uri = "/diffusion/{$identifier}/repository/{$path}";
+          $uri = $this->getPathURI("/repository/{$path}");
         } else {
-          $uri = "/diffusion/{$identifier}/";
+          $uri = $this->getPathURI();
         }
         break;
       case 'external':
@@ -2108,9 +2096,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     $has_callsign = ($this->getCallsign() !== null);
     $has_shortname = ($this->getRepositorySlug() !== null);
 
-    // TODO: For now, never enable these because they don't work yet.
-    $has_shortname = false;
-
     $identifier_map = array(
       PhabricatorRepositoryURI::BUILTIN_IDENTIFIER_CALLSIGN => $has_callsign,
       PhabricatorRepositoryURI::BUILTIN_IDENTIFIER_SHORTNAME => $has_shortname,

src/applications/repository/storage/PhabricatorRepositoryURI.php

@@ -125,8 +125,8 @@ final class PhabricatorRepositoryURI
         $other_uris = $repository->getURIs();
 
         $identifier_value = array(
-          self::BUILTIN_IDENTIFIER_CALLSIGN => 3,
+          self::BUILTIN_IDENTIFIER_SHORTNAME => 3,
-          self::BUILTIN_IDENTIFIER_SHORTNAME => 2,
+          self::BUILTIN_IDENTIFIER_CALLSIGN => 2,
           self::BUILTIN_IDENTIFIER_ID => 1,
         );