Fix many encoding and architecture problems in Diffusion request and URI handling

Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:

  - Tons and tons of duplicated code.
  - Bugs with handling unusual branch and file names.
  - An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
  - Other tools were doing hacky things like passing ":" branch names.

This diff attempts to fix these issues.

  - Make the base class abstract (it was concrete ONLY for "/diffusion/").
  - Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
  - Delete the 300 copies of URI generation code throughout Diffusion.
  - Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
  - Add an appropriate static initializer for other callers.
  - Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
  - Refactor static initializers to be sensibly-sized.
  - Refactor derived DiffusionRequest classes to remove duplicated code.
  - Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
  - Properly encode path names (fixes issues in D1742).
  - Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
  - Fix a couple warnings.
  - Fix a couple lint issues.
  - Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
  - Fix a bug where Git change queries would fail unnecessarily.
  - Provide or improve some documentation.

This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.

This supplants D1742.

Test Plan:
  - Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
  - Used Owners typeaheads and search.
  - Used diffusion.getrecentcommitsbypath method.
  - Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.

{F9185}

Reviewers: nh, vrana, btrahan

Reviewed By: btrahan

CC: aran, epriestley

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

src/__phutil_library_map__.php

@@ -340,6 +340,7 @@ phutil_register_library_map(array(
     'DiffusionSvnRequest' => 'applications/diffusion/request/svn',
     'DiffusionSymbolController' => 'applications/diffusion/controller/symbol',
     'DiffusionSymbolQuery' => 'applications/diffusion/query/symbol',
+    'DiffusionURITestCase' => 'applications/diffusion/request/base/__tests__',
     'DiffusionView' => 'applications/diffusion/view/base',
     'DrydockAllocator' => 'applications/drydock/allocator/resource',
     'DrydockAllocatorWorker' => 'applications/drydock/allocator/worker',
@@ -1188,6 +1189,7 @@ phutil_register_library_map(array(
     'DiffusionSvnLastModifiedQuery' => 'DiffusionLastModifiedQuery',
     'DiffusionSvnRequest' => 'DiffusionRequest',
     'DiffusionSymbolController' => 'DiffusionController',
+    'DiffusionURITestCase' => 'ArcanistPhutilTestCase',
     'DiffusionView' => 'AphrontView',
     'DrydockAllocatorWorker' => 'PhabricatorWorker',
     'DrydockCommandInterface' => 'DrydockInterface',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -247,30 +247,13 @@ class AphrontDefaultApplicationConfiguration
         '' => 'DiffusionHomeController',
         '(?P<callsign>[A-Z]+)/' => array(
           '' => 'DiffusionRepositoryController',
-          'repository/'.
+
-            '(?P<path>[^/]+)/'
+          'repository/(?P<dblob>.*)'    => 'DiffusionRepositoryController',
-              => 'DiffusionRepositoryController',
+          'change/(?P<dblob>.*)'        => 'DiffusionChangeController',
-          'change/'.
+          'history/(?P<dblob>.*)'       => 'DiffusionHistoryController',
-            '(?P<path>.*?)'.
+          'browse/(?P<dblob>.*)'        => 'DiffusionBrowseController',
-            '(?:[;](?P<commit>[a-z0-9]+))?'
+          'lastmodified/(?P<dblob>.*)'  => 'DiffusionLastModifiedController',
-              => 'DiffusionChangeController',
+          'diff/'                       => 'DiffusionDiffController',
-          'history/'.
-            '(?P<path>.*?)'.
-            '(?:[;](?P<commit>[a-z0-9]+))?'
-              => 'DiffusionHistoryController',
-          'browse/'.
-            '(?P<path>.*?)'.
-            '(?:[;](?P<commit>[a-z0-9]+))?'.
-            '(?:[$](?P<line>\d+(?:-\d+)?))?'
-              => 'DiffusionBrowseController',
-          'diff/'.
-            '(?P<path>.*?)'.
-            '(?:[;](?P<commit>[a-z0-9]+))?'
-              => 'DiffusionDiffController',
-          'lastmodified/'.
-            '(?P<path>.*?)'.
-            '(?:[;](?P<commit>[a-z0-9]+))?'
-              => 'DiffusionLastModifiedController',
         ),
         'inline/(?P<phid>[^/]+)/' => 'DiffusionInlineCommentController',
         'services/' => array(

src/applications/conduit/method/diffusion/getrecentcommitsbypath/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php

@@ -45,13 +45,13 @@ final class ConduitAPI_diffusion_getrecentcommitsbypath_Method
   }
 
   protected function execute(ConduitAPIRequest $request) {
-    $results = array();
+    $drequest = DiffusionRequest::newFromDictionary(
+      array(
+        'callsign'  => $request->getValue('callsign'),
+        'path'      => $request->getValue('path'),
+      ));
 
-    $history = DiffusionHistoryQuery::newFromDiffusionRequest(
+    $history = DiffusionHistoryQuery::newFromDiffusionRequest($drequest)
-      DiffusionRequest::newFromAphrontRequestDictionary(
-        $request->getAllParameters()
-      )
-    )
     ->setLimit(self::RESULT_LIMIT)
     ->needDirectChanges(true)
     ->needChildChanges(true)

src/applications/diffusion/controller/base/DiffusionController.php

@@ -21,8 +21,10 @@ abstract class DiffusionController extends PhabricatorController {
   protected $diffusionRequest;
 
   public function willProcessRequest(array $data) {
-    $this->diffusionRequest = DiffusionRequest::newFromAphrontRequestDictionary(
+    if (isset($data['callsign'])) {
-      $data);
+      $drequest = DiffusionRequest::newFromAphrontRequestDictionary($data);
+      $this->diffusionRequest = $drequest;
+    }
   }
 
   public function setDiffusionRequest(DiffusionRequest $request) {
@@ -31,6 +33,9 @@ abstract class DiffusionController extends PhabricatorController {
   }
 
   protected function getDiffusionRequest() {
+    if (!$this->diffusionRequest) {
+      throw new Exception("No Diffusion request object!");
+    }
     return $this->diffusionRequest;
   }
 
@@ -73,37 +78,34 @@ abstract class DiffusionController extends PhabricatorController {
     }
 
     $drequest = $this->getDiffusionRequest();
-    $repository = $drequest->getRepository();
-    $callsign = $repository->getCallsign();
 
-    $branch_uri = $drequest->getBranchURIComponent($drequest->getBranch());
+    foreach ($navs as $action => $name) {
-    $path_uri = $branch_uri.$drequest->getPath();
+      $href = $drequest->generateURI(
+        array(
+          'action' => $action,
+        ));
 
-    $commit_uri = null;
-    $raw_commit = $drequest->getRawCommit();
-    if ($raw_commit) {
-      $commit_uri = ';'.$drequest->getCommitURIComponent($raw_commit);
-    }
-
-    foreach ($navs as $uri => $name) {
       $nav->addNavItem(
         phutil_render_tag(
           'a',
           array(
-            'href'  => "/diffusion/{$callsign}/{$uri}/{$path_uri}{$commit_uri}",
+            'href'  => $href,
             'class' =>
-              ($uri == $selected
+              ($action == $selected
                 ? 'aphront-side-nav-selected'
                 : null),
           ),
           $name));
     }
+
+    // TODO: URI encoding might need to be sorted out for this link.
+
     $nav->addNavItem(
       phutil_render_tag(
         'a',
         array(
           'href'  => '/owners/view/search/'.
-            '?repository='.phutil_escape_uri($callsign).
+            '?repository='.phutil_escape_uri($drequest->getCallsign()).
             '&path='.phutil_escape_uri('/'.$drequest->getPath()),
         ),
         'Search Owners'));
@@ -158,11 +160,18 @@ abstract class DiffusionController extends PhabricatorController {
   }
 
   private function buildCrumbList(array $spec = array()) {
-    $drequest = $this->getDiffusionRequest();
 
     $crumb_list = array();
 
-    $repository = $drequest->getRepository();
+    // On the home page, we don't have a DiffusionRequest.
+    if ($this->diffusionRequest) {
+      $drequest = $this->getDiffusionRequest();
+      $repository = $drequest->getRepository();
+    } else {
+      $drequest = null;
+      $repository = null;
+    }
+
     if ($repository) {
       $crumb_list[] = phutil_render_tag(
         'a',
@@ -231,14 +240,9 @@ abstract class DiffusionController extends PhabricatorController {
         return $crumb_list;
     }
 
-    $branch_uri = $drequest->getBranchURIComponent($drequest->getBranch());
+    $uri_params = array(
-    $view_root_uri = "/diffusion/{$callsign}/{$view}/{$branch_uri}";
+      'action' => $view,
-    $jump_href = $view_root_uri;
+    );
-
-    $view_tail_uri = null;
-    if ($raw_commit) {
-      $view_tail_uri = ';'.$drequest->getCommitURIComponent($raw_commit);
-    }
 
     if (!strlen($path)) {
       $crumb_list[] = $view_name;
@@ -247,7 +251,10 @@ abstract class DiffusionController extends PhabricatorController {
       $crumb_list[] = phutil_render_tag(
         'a',
         array(
-          'href' => $view_root_uri.$view_tail_uri,
+          'href' => $drequest->generateURI(
+            array(
+              'path' => null,
+            ) + $uri_params),
         ),
         $view_name);
 
@@ -263,7 +270,10 @@ abstract class DiffusionController extends PhabricatorController {
         $path_sections[] = phutil_render_tag(
           'a',
           array(
-            'href' => $view_root_uri.$thus_far.$view_tail_uri,
+            'href' => $drequest->generateURI(
+              array(
+                'path' => $thus_far,
+              ) + $uri_params),
           ),
           phutil_escape_html($path_part));
       }
@@ -271,8 +281,6 @@ abstract class DiffusionController extends PhabricatorController {
       $path_sections[] = phutil_escape_html($last);
       $path_sections = '/'.implode('/', $path_sections);
 
-      $jump_href = $view_root_uri.$thus_far.$last;
-
       $crumb_list[] = $path_sections;
     }
 
@@ -282,7 +290,10 @@ abstract class DiffusionController extends PhabricatorController {
       $jump_link = phutil_render_tag(
         'a',
         array(
-          'href' => $jump_href,
+          'href' => $drequest->generateURI(
+            array(
+              'commit' => null,
+            ) + $uri_params),
         ),
         'Jump to HEAD');
       $last_crumb .= " @ {$commit_link} ({$jump_link})";

src/applications/diffusion/controller/commit/DiffusionCommitController.php

@@ -20,6 +20,13 @@ final class DiffusionCommitController extends DiffusionController {
 
   const CHANGES_LIMIT = 100;
 
+  public function willProcessRequest(array $data) {
+    // This controller doesn't use blob/path stuff, just pass the dictionary
+    // in directly instead of using the AphrontRequest parsing mechanism.
+    $drequest = DiffusionRequest::newFromDictionary($data);
+    $this->diffusionRequest = $drequest;
+  }
+
   public function processRequest() {
     $drequest = $this->getDiffusionRequest();
     $request = $this->getRequest();
@@ -192,11 +199,11 @@ final class DiffusionCommitController extends DiffusionController {
           }
         }
 
-        $branch = $drequest->getBranchURIComponent(
+        $references[$key] = $drequest->generateURI(
-          $drequest->getBranch());
+          array(
-        $filename = $changeset->getFilename();
+            'action' => 'rendering-ref',
-        $reference = "{$branch}{$filename};".$drequest->getCommit();
+            'path'   => $changeset->getFilename(),
-        $references[$key] = $reference;
+          ));
       }
 
       // TOOD: Some parts of the views still rely on properties of the

src/applications/diffusion/controller/commit/__init__.php

@@ -19,6 +19,7 @@ phutil_require_module('phabricator', 'applications/diffusion/data/pathchange');
 phutil_require_module('phabricator', 'applications/diffusion/query/contains/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/pathchange/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/pathid/base');
+phutil_require_module('phabricator', 'applications/diffusion/request/base');
 phutil_require_module('phabricator', 'applications/diffusion/view/commentlist');
 phutil_require_module('phabricator', 'applications/diffusion/view/commitchangetable');
 phutil_require_module('phabricator', 'applications/draft/storage/draft');

src/applications/diffusion/controller/diff/DiffusionDiffController.php

@@ -19,15 +19,12 @@
 final class DiffusionDiffController extends DiffusionController {
 
   public function willProcessRequest(array $data) {
-    $request = $this->getRequest();
+    $data = $data + array(
-    if ($request->getStr('ref')) {
+      'dblob' => $this->getRequest()->getStr('ref'),
-      $parts = explode(';', $request->getStr('ref'));
+    );
-      $data['path'] = idx($parts, 0);
+    $drequest = DiffusionRequest::newFromAphrontRequestDictionary($data);
-      $data['commit'] = idx($parts, 1);
-    }
 
-    $this->diffusionRequest = DiffusionRequest::newFromAphrontRequestDictionary(
+    $this->diffusionRequest = $drequest;
-      $data);
   }
 
   public function processRequest() {

src/applications/diffusion/controller/file/DiffusionBrowseFileController.php

@@ -403,22 +403,27 @@ final class DiffusionBrowseFileController extends DiffusionController {
         $targ = null;
       }
 
-      // Create the row display.
+      $href = $drequest->generateURI(
-      $uri_path = $drequest->getUriPath();
+        array(
-      $uri_rev  = $drequest->getStableCommitName();
+          'action' => 'browse',
-      $uri_view = $view
+          'stable' => true,
-        ? '?view='.$view
+        ));
-        : null;
+      $href = (string)$href;
 
-      $l = phutil_render_tag(
+      $query_params = null;
+      if ($view) {
+        $query_params = '?view='.$view;
+      }
+
+      $link = phutil_render_tag(
         'a',
         array(
-          'href' => $uri_path.';'.$uri_rev.'$'.$n.$uri_view,
+          'href' => $href.'$'.$n.$query_params,
         ),
         $n);
 
       $rows[] = $tr.$blame_info.
-        '<th class="diffusion-wide-link">'.$l.'</th>'.
+        '<th class="diffusion-wide-link">'.$link.'</th>'.
         '<td>'.$targ.$line.'</td></tr>';
       ++$n;
     }

src/applications/diffusion/controller/pathcomplete/DiffusionPathCompleteController.php

@@ -44,10 +44,10 @@ final class DiffusionPathCompleteController extends DiffusionController {
       }
     }
 
-    $drequest = DiffusionRequest::newFromAphrontRequestDictionary(
+    $drequest = DiffusionRequest::newFromDictionary(
       array(
-        'callsign'  => $repository->getCallsign(),
+        'repository'  => $repository,
-        'path'      => ':/'.$query_dir,
+        'path'        => $query_dir,
       ));
 
     $browse_query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest);

src/applications/diffusion/controller/pathvalidate/DiffusionPathValidateController.php

@@ -36,10 +36,10 @@ final class DiffusionPathValidateController extends DiffusionController {
     $path = $request->getStr('path');
     $path = ltrim($path, '/');
 
-    $drequest = DiffusionRequest::newFromAphrontRequestDictionary(
+    $drequest = DiffusionRequest::newFromDictionary(
       array(
-        'callsign'  => $repository->getCallsign(),
+        'repository'  => $repository,
-        'path'      => ':/'.$path,
+        'path'        => $path,
       ));
 
     $browse_query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest);

src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,16 +22,12 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery {
     $drequest = $this->getRequest();
     $repository = $drequest->getRepository();
 
-    if (!$drequest->getRawCommit()) {
+    $effective_commit = $this->getEffectiveCommit();
-      $effective_commit = $this->getEffectiveCommit();
+    if (!$effective_commit) {
-      if (!$effective_commit) {
+      return null;
-        return null;
-      }
-      // TODO: This side effect is kind of skethcy.
-      $drequest->setCommit($effective_commit);
-    } else {
-      $effective_commit = $drequest->getCommit();
     }
+    // TODO: This side effect is kind of skethcy.
+    $drequest->setCommit($effective_commit);
 
     $options = array(
       '-M',
@@ -72,6 +68,10 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery {
       }
     }
 
+    if (!$raw_diff) {
+      return null;
+    }
+
     $parser = new ArcanistDiffParser();
 
     $try_encoding = $repository->getDetail('encoding');
@@ -86,10 +86,10 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery {
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 
-    $this->renderingReference =
+    $this->renderingReference = $drequest->generateURI(
-      $drequest->getBranchURIComponent($drequest->getBranch()).
+      array(
-      $drequest->getPath().';'.
+        'action' => 'rendering-ref',
-      $drequest->getCommit();
+      ));
 
     return $changeset;
   }

src/applications/diffusion/query/diff/mercurial/DiffusionMercurialDiffQuery.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,10 +51,10 @@ final class DiffusionMercurialDiffQuery extends DiffusionDiffQuery {
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 
-    $this->renderingReference =
+    $this->renderingReference = $drequest->generateURI(
-      $drequest->getBranchURIComponent($drequest->getBranch()).
+      array(
-      $drequest->getPath().';'.
+        'action' => 'rendering-ref',
-      $drequest->getCommit();
+      ));
 
     return $changeset;
   }

src/applications/diffusion/request/base/DiffusionRequest.php

@@ -17,10 +17,16 @@
  */
 
 /**
- * TODO: This might need to be concrete-extensible, but straighten out the
+ * Contains logic to parse Diffusion requests, which have a complicated URI
- * class hierarchy here.
+ * structure.
+ *
+ *
+ * @task new Creating Requests
+ * @task uri Managing Diffusion URIs
+ *
+ * @group diffusion
  */
-class DiffusionRequest {
+abstract class DiffusionRequest {
 
   protected $callsign;
   protected $path;
@@ -33,60 +39,152 @@ class DiffusionRequest {
   protected $repositoryCommitData;
   protected $stableCommitName;
 
+  abstract protected function getSupportsBranches();
+  abstract protected function didInitialize();
+
+
+/* -(  Creating Requests  )-------------------------------------------------- */
+
+
+  /**
+   * Create a new synthetic request from a parameter dictionary. If you need
+   * a @{class:DiffusionRequest} object in order to issue a DiffusionQuery, you
+   * can use this method to build one.
+   *
+   * Parameters are:
+   *
+   *   - `callsign` Repository callsign. Provide this or `repository`.
+   *   - `repository` Repository object. Provide this or `callsign`.
+   *   - `branch` Optional, branch name.
+   *   - `path` Optional, file path.
+   *   - `commit` Optional, commit identifier.
+   *   - `line` Optional, line range.
+   *
+   * @param   map                 See documentation.
+   * @return  DiffusionRequest    New request object.
+   * @task new
+   */
+  final public static function newFromDictionary(array $data) {
+    if (isset($data['repository']) && isset($data['callsign'])) {
+      throw new Exception(
+        "Specify 'repository' or 'callsign', but not both.");
+    } else if (!isset($data['repository']) && !isset($data['callsign'])) {
+      throw new Exception(
+        "One of 'repository' and 'callsign' is required.");
+    }
+
+    if (isset($data['repository'])) {
+      $object = self::newFromRepository($data['repository']);
+    } else {
+      $object = self::newFromCallsign($data['callsign']);
+    }
+    $object->initializeFromDictionary($data);
+    return $object;
+  }
+
+
+  /**
+   * Create a new request from an Aphront request dictionary. This is an
+   * internal method that you generally should not call directly; instead,
+   * call @{method:newFromDictionary}.
+   *
+   * @param   map                 Map of Aphront request data.
+   * @return  DiffusionRequest    New request object.
+   * @task new
+   */
+  final public static function newFromAphrontRequestDictionary(array $data) {
+    $callsign = phutil_unescape_uri_path_component(idx($data, 'callsign'));
+    $object = self::newFromCallsign($callsign);
+
+    $use_branches = $object->getSupportsBranches();
+    $parsed = self::parseRequestBlob(idx($data, 'dblob'), $use_branches);
+
+    $object->initializeFromDictionary($parsed);
+    return $object;
+  }
+
+
+  /**
+   * Internal.
+   *
+   * @task new
+   */
   final private function __construct() {
     // <private>
   }
 
-  final public static function newFromAphrontRequestDictionary(array $data) {
 
-    $vcs = null;
+  /**
-    $repository = null;
+   * Internal. Use @{method:newFromDictionary}, not this method.
-    $callsign = idx($data, 'callsign');
+   *
-    if ($callsign) {
+   * @param   string              Repository callsign.
-      $repository = id(new PhabricatorRepository())->loadOneWhere(
+   * @return  DiffusionRequest    New request object.
-        'callsign = %s',
+   * @task new
-        $callsign);
+   */
-      if (!$repository) {
+  final private static function newFromCallsign($callsign) {
-        throw new Exception("No such repository '{$callsign}'.");
+    $repository = id(new PhabricatorRepository())->loadOneWhere(
-      }
+      'callsign = %s',
-      $vcs = $repository->getVersionControlSystem();
+      $callsign);
+
+    if (!$repository) {
+      throw new Exception("No such repository '{$callsign}'.");
     }
 
-    switch ($vcs) {
+    return self::newFromRepository($repository);
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+  }
-        $class = 'DiffusionGitRequest';
+
-        break;
+
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+  /**
-        $class = 'DiffusionSvnRequest';
+   * Internal. Use @{method:newFromDictionary}, not this method.
-        break;
+   *
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+   * @param   PhabricatorRepository   Repository object.
-        $class = 'DiffusionMercurialRequest';
+   * @return  DiffusionRequest        New request object.
-        break;
+   * @task new
-      default:
+   */
-        $class = 'DiffusionRequest';
+  final private static function newFromRepository(
-        break;
+    PhabricatorRepository $repository) {
+
+    $map = array(
+      PhabricatorRepositoryType::REPOSITORY_TYPE_GIT => 'DiffusionGitRequest',
+      PhabricatorRepositoryType::REPOSITORY_TYPE_SVN => 'DiffusionSvnRequest',
+      PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL =>
+        'DiffusionMercurialRequest',
+    );
+
+    $class = idx($map, $repository->getVersionControlSystem());
+
+    if (!$class) {
+      throw new Exception("Unknown version control system!");
     }
 
     $object = new $class();
 
-    $object->callsign   = $callsign;
     $object->repository = $repository;
-    $object->line       = idx($data, 'line');
+    $object->callsign   = $repository->getCallsign();
-    $object->commit     = idx($data, 'commit');
-    $object->path       = idx($data, 'path');
-
-    $object->initializeFromAphrontRequestDictionary($data);
 
     return $object;
   }
 
-  protected function initializeFromAphrontRequestDictionary(array $data) {
 
+  /**
+   * Internal. Use @{method:newFromDictionary}, not this method.
+   *
+   * @param map Map of parsed data.
+   * @return void
+   * @task new
+   */
+  final private function initializeFromDictionary(array $data) {
+    $this->path       = idx($data, 'path');
+    $this->commit     = idx($data, 'commit');
+    $this->line       = idx($data, 'line');
+
+    if ($this->getSupportsBranches()) {
+      $this->branch = idx($data, 'branch');
+    }
+
+    $this->didInitialize();
   }
 
-  protected function parsePath($path) {
-    $this->path = $path;
-  }
 
   public function getRepository() {
     return $this->repository;
@@ -100,10 +198,6 @@ class DiffusionRequest {
     return $this->path;
   }
 
-  public function getUriPath() {
-    return '/diffusion/'.$this->getCallsign().'/browse/'.$this->path;
-  }
-
   public function getLine() {
     return $this->line;
   }
@@ -166,12 +260,204 @@ class DiffusionRequest {
     return $this;
   }
 
-  public function getCommitURIComponent($commit) {
+/* -(  Managing Diffusion URIs  )-------------------------------------------- */
-    return $commit;
+
+
+  /**
+   * Generate a Diffusion URI using this request to provide defaults. See
+   * @{method:generateDiffusionURI} for details. This method is the same, but
+   * preserves the request parameters if they are not overridden.
+   *
+   * @param   map         See @{method:generateDiffusionURI}.
+   * @return  PhutilURI   Generated URI.
+   * @task uri
+   */
+  public function generateURI(array $params) {
+    if (empty($params['stable'])) {
+      $default_commit = $this->getRawCommit();
+    } else {
+      $default_commit = $this->getStableCommitName();
+    }
+
+    $params += array(
+      'callsign'  => $this->getCallsign(),
+      'path'      => $this->getPath(),
+      'branch'    => $this->getBranch(),
+      'commit'    => $default_commit,
+    );
+    return self::generateDiffusionURI($params);
   }
 
-  public function getBranchURIComponent($branch) {
+
-    return $branch;
+  /**
+   * Generate a Diffusion URI from a parameter map. Applies the correct encoding
+   * and formatting to the URI. Parameters are:
+   *
+   *   - `action` One of `history`, `browse`, `change`, `lastmodified`,
+   *     `branch` or `revision-ref`. The action specified by the URI.
+   *   - `callsign` Repository callsign.
+   *   - `branch` Optional if action is not `branch`, branch name.
+   *   - `path` Optional, path to file.
+   *   - `commit` Optional, commit identifier.
+   *   - `line` Optional, line range.
+   *   - `params` Optional, query parameters.
+   *
+   * The function generates the specified URI and returns it.
+   *
+   * @param   map         See documentation.
+   * @return  PhutilURI   Generated URI.
+   * @task uri
+   */
+  public static function generateDiffusionURI(array $params) {
+    $action = idx($params, 'action');
+
+    $callsign = idx($params, 'callsign');
+    $path     = idx($params, 'path');
+    $branch   = idx($params, 'branch');
+    $commit   = idx($params, 'commit');
+    $line     = idx($params, 'line');
+
+    if (strlen($callsign)) {
+      $callsign = phutil_escape_uri_path_component($callsign).'/';
+    }
+
+    if (strlen($branch)) {
+      $branch = phutil_escape_uri_path_component($branch).'/';
+    }
+
+    if (strlen($path)) {
+      $path = str_replace(array(';', '$'), array(';;', '$$'), $path);
+      $path = phutil_escape_uri($path);
+    }
+
+    $path = "{$branch}{$path}";
+
+    if (strlen($commit)) {
+      $commit = ';'.phutil_escape_uri($commit);
+    }
+
+    if (strlen($line)) {
+      $line = '$'.phutil_escape_uri($line);
+    }
+
+    $req_callsign = false;
+    $req_branch   = false;
+
+    switch ($action) {
+      case 'history':
+      case 'browse':
+      case 'change':
+      case 'lastmodified':
+        $req_callsign = true;
+        break;
+      case 'branch':
+        $req_callsign = true;
+        $req_branch = true;
+        break;
+    }
+
+    if ($req_callsign && !strlen($callsign)) {
+      throw new Exception(
+        "Diffusion URI action '{$action}' requires callsign!");
+    }
+
+    if ($req_branch && !strlen($branch)) {
+      throw new Exception(
+        "Diffusion URI action '{$action}' requires brnach!");
+    }
+
+    switch ($action) {
+      case 'change':
+      case 'history':
+      case 'browse':
+      case 'lastmodified':
+        $uri = "/diffusion/{$callsign}{$action}/{$path}{$commit}{$line}";
+        break;
+      case 'branch':
+        $uri = "/diffusion/{$callsign}repository/{$path}";
+        break;
+      case 'rendering-ref':
+        // This isn't a real URI per se, it's passed as a query parameter to
+        // the ajax changeset stuff but then we parse it back out as though
+        // it came from a URI.
+        $uri = "{$path}{$commit}";
+        break;
+      default:
+        throw new Exception("Unknown Diffusion URI action '{$action}'!");
+    }
+
+    if ($action == 'rendering-ref') {
+      return $uri;
+    }
+
+    $uri = new PhutilURI($uri);
+    if (idx($params, 'params')) {
+      $uri->setQueryParams($params['params']);
+    }
+
+    return $uri;
+  }
+
+
+  /**
+   * Internal. Public only for unit tests.
+   *
+   * Parse the request URI into components.
+   *
+   * @param   string  URI blob.
+   * @param   bool    True if this VCS supports branches.
+   * @return  map     Parsed URI.
+   *
+   * @task uri
+   */
+  public static function parseRequestBlob($blob, $supports_branches) {
+    $result = array(
+      'branch'  => null,
+      'path'    => null,
+      'commit'  => null,
+      'line'    => null,
+    );
+
+    $matches = null;
+
+    if ($supports_branches) {
+      // Consume the front part of the URI, up to the first "/". This is the
+      // path-component encoded branch name.
+      if (preg_match('@^([^/]+)/@', $blob, $matches)) {
+        $result['branch'] = phutil_unescape_uri_path_component($matches[1]);
+        $blob = substr($blob, strlen($matches[1]) + 1);
+      }
+    }
+
+    // Consume the back part of the URI, up to the first "$". Use a negative
+    // lookbehind to prevent matching '$$'. We double the '$' symbol when
+    // encoding so that files with names like "money/$100" will survive.
+    if (preg_match('@(?<![$])[$]([\d-]+)$@', $blob, $matches)) {
+      $result['line'] = $matches[1];
+      $blob = substr($blob, 0, -(strlen($matches[1]) + 1));
+    }
+
+    // Consume the commit name, stopping on ';;'.
+    if (preg_match('@(?<!;);([a-z0-9]+)$@', $blob, $matches)) {
+      $result['commit'] = $matches[1];
+      $blob = substr($blob, 0, -(strlen($matches[1]) + 1));
+    }
+
+    // Un-double our delimiter characters.
+    if (strlen($blob)) {
+      $result['path'] = str_replace(array(';;', '$$'), array(';', '$'), $blob);
+    }
+
+    $parts = explode('/', $result['path']);
+    foreach ($parts as $part) {
+      // Prevent any hyjinx since we're ultimately shipping this to the
+      // filesystem under a lot of workflows.
+      if ($part == '..') {
+        throw new Exception("Invalid path URI.");
+      }
+    }
+
+    return $result;
   }
 
 }

src/applications/diffusion/request/base/__init__.php

@@ -11,6 +11,8 @@ phutil_require_module('phabricator', 'applications/repository/storage/commit');
 phutil_require_module('phabricator', 'applications/repository/storage/commitdata');
 phutil_require_module('phabricator', 'applications/repository/storage/repository');
 
+phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/diffusion/request/base/__tests__/DiffusionURITestCase.php

@@ -0,0 +1,137 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DiffusionURITestCase extends ArcanistPhutilTestCase {
+
+  public function testBlobDecode() {
+    $map = array(
+      // This is a basic blob.
+      'branch/path.ext;abc$3' => array(
+        'branch'  => 'branch',
+        'path'    => 'path.ext',
+        'commit'  => 'abc',
+        'line'    => '3',
+      ),
+      'branch/path.ext$3' => array(
+        'branch'  => 'branch',
+        'path'    => 'path.ext',
+        'line'    => '3',
+      ),
+      'branch/money;;/$$100'  => array(
+        'branch'  => 'branch',
+        'path'    => 'money;/$100',
+      ),
+      'a%252Fb/' => array(
+        'branch'  => 'a/b',
+      ),
+    );
+
+    foreach ($map as $input => $expect) {
+
+      // Simulate decode effect of the webserver.
+      $input = rawurldecode($input);
+
+      $expect = $expect + array(
+        'branch' => null,
+        'path'   => null,
+        'commit' => null,
+        'line'   => null,
+      );
+      $expect = array_select_keys(
+        $expect,
+        array('branch', 'path', 'commit', 'line'));
+
+      $actual = $this->parseBlob($input);
+
+      $this->assertEqual(
+        $expect,
+        $actual,
+        "Parsing '{$input}'");
+    }
+  }
+
+  public function testBlobDecodeFail() {
+    $this->tryTestCaseMap(
+      array(
+        'branch/path/../../../secrets/secrets.key' => false,
+      ),
+      array($this, 'parseBlob'));
+  }
+
+  public function parseBlob($blob) {
+    return DiffusionRequest::parseRequestBlob(
+      $blob,
+      $supports_branches = true);
+  }
+
+  public function testURIGeneration() {
+    $map = array(
+      '/diffusion/A/browse/branch/path.ext;abc$1' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'branch'    => 'branch',
+        'path'      => 'path.ext',
+        'commit'    => 'abc',
+        'line'      => '1',
+      ),
+      '/diffusion/A/browse/a%252Fb/path.ext' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'branch'    => 'a/b',
+        'path'      => 'path.ext',
+      ),
+      '/diffusion/A/browse/%2B/%20%21' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'path'      => '+/ !',
+      ),
+      '/diffusion/A/browse/money/%24%24100$2' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'path'      => 'money/$100',
+        'line'      => '2',
+      ),
+      '/diffusion/A/browse/path/to/file.ext?view=things' => array(
+        'action'    => 'browse',
+        'callsign'  => 'A',
+        'path'      => 'path/to/file.ext',
+        'params'    => array(
+          'view' => 'things',
+        ),
+      ),
+      '/diffusion/A/repository/master/' => array(
+        'action'    => 'branch',
+        'callsign'  => 'A',
+        'branch'    => 'master',
+      ),
+      'path/to/file.ext;abc' => array(
+        'action'    => 'rendering-ref',
+        'path'      => 'path/to/file.ext',
+        'commit'    => 'abc',
+      ),
+    );
+
+    foreach ($map as $expect => $input) {
+      $actual = DiffusionRequest::generateDiffusionURI($input);
+      $this->assertEqual(
+        $expect,
+        (string)$actual);
+    }
+  }
+
+}

src/applications/diffusion/request/base/__tests__/__init__.php

@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
+
+phutil_require_module('phabricator', 'applications/diffusion/request/base');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('DiffusionURITestCase.php');

src/applications/diffusion/request/git/DiffusionGitRequest.php

@@ -16,29 +16,16 @@
  * limitations under the License.
  */
 
+/**
+ * @group diffusion
+ */
 final class DiffusionGitRequest extends DiffusionRequest {
 
-  protected function initializeFromAphrontRequestDictionary(array $data) {
+  protected function getSupportsBranches() {
-    parent::initializeFromAphrontRequestDictionary($data);
+    return true;
-
+  }
-    $path = $this->path;
-    $parts = explode('/', $path);
-
-    $branch = array_shift($parts);
-    if ($branch != ':') {
-      $this->branch = $this->decodeBranchName($branch);
-    }
-
-    foreach ($parts as $key => $part) {
-      // Prevent any hyjinx since we're ultimately shipping this to the
-      // filesystem under a lot of git workflows.
-      if ($part == '..') {
-        unset($parts[$key]);
-      }
-    }
-
-    $this->path = implode('/', $parts);
 
+  protected function didInitialize() {
     if ($this->repository) {
       $repository = $this->repository;
 
@@ -109,11 +96,6 @@ final class DiffusionGitRequest extends DiffusionRequest {
     throw new Exception("Unable to determine branch!");
   }
 
-  public function getUriPath() {
-    return '/diffusion/'.$this->getCallsign().'/browse/'.
-      $this->getBranchURIComponent($this->branch).$this->path;
-  }
-
   public function getCommit() {
     if ($this->commit) {
       return $this->commit;
@@ -126,26 +108,4 @@ final class DiffusionGitRequest extends DiffusionRequest {
     return substr($this->stableCommitName, 0, 16);
   }
 
-  public function getBranchURIComponent($branch) {
-    return $this->encodeBranchName($branch).'/';
-  }
-
-  private function decodeBranchName($branch) {
-    $branch = str_replace(':', '/', $branch);
-
-    // Backward compatibility for older-style URIs which had an explicit
-    // "origin" remote in the branch name. If a remote is specified, strip it
-    // away.
-    if (strpos($branch, '/') !== false) {
-      $parts = explode('/', $branch);
-      $branch = end($parts);
-    }
-
-    return $branch;
-  }
-
-  private function encodeBranchName($branch) {
-    return str_replace('/', ':', $branch);
-  }
-
 }

src/applications/diffusion/request/mercurial/DiffusionMercurialRequest.php

@@ -16,28 +16,17 @@
  * limitations under the License.
  */
 
-// TODO: This has some minor code duplication vs the Git request that could be
+/**
-// shared.
+ * @group diffusion
+ */
 final class DiffusionMercurialRequest extends DiffusionRequest {
 
-  protected function initializeFromAphrontRequestDictionary(array $data) {
+  protected function getSupportsBranches() {
-    parent::initializeFromAphrontRequestDictionary($data);
+    return true;
+  }
 
-    $path = $this->path;
+  protected function didInitialize() {
-    $parts = explode('/', $path);
+    return;
-
-    $branch = array_shift($parts);
-    if ($branch != ':') {
-      $this->branch = $this->decodeBranchName($branch);
-    }
-
-    foreach ($parts as $key => $part) {
-      if ($part == '..') {
-        unset($parts[$key]);
-      }
-    }
-
-    $this->path = implode('/', $parts);
   }
 
   public function getBranch() {
@@ -50,11 +39,6 @@ final class DiffusionMercurialRequest extends DiffusionRequest {
     throw new Exception("Unable to determine branch!");
   }
 
-  public function getUriPath() {
-    return '/diffusion/'.$this->getCallsign().'/browse/'.
-      $this->getBranchURIComponent($this->branch).$this->path;
-  }
-
   public function getCommit() {
     if ($this->commit) {
       return $this->commit;
@@ -66,16 +50,4 @@ final class DiffusionMercurialRequest extends DiffusionRequest {
     return substr($this->stableCommitName, 0, 16);
   }
 
-  public function getBranchURIComponent($branch) {
-    return $this->encodeBranchName($branch).'/';
-  }
-
-  private function decodeBranchName($branch) {
-    return str_replace(':', '/', $branch);
-  }
-
-  private function encodeBranchName($branch) {
-    return str_replace('/', ':', $branch);
-  }
-
 }

src/applications/diffusion/request/svn/DiffusionSvnRequest.php

@@ -16,22 +16,22 @@
  * limitations under the License.
  */
 
+/**
+ * @group diffusion
+ */
 final class DiffusionSvnRequest extends DiffusionRequest {
 
-  protected function initializeFromAphrontRequestDictionary(array $data) {
+  protected function getSupportsBranches() {
-    parent::initializeFromAphrontRequestDictionary($data);
+    return false;
+  }
 
+  protected function didInitialize() {
     if ($this->path === null) {
       $subpath = $this->repository->getDetail('svn-subpath');
       if ($subpath) {
         $this->path = $subpath;
       }
     }
-
-    if (!strncmp($this->path, ':', 1)) {
-      $this->path = substr($this->path, 1);
-      $this->path = ltrim($this->path, '/');
-    }
   }
 
   public function getStableCommitName() {

src/applications/diffusion/view/base/DiffusionView.php

@@ -41,70 +41,43 @@ abstract class DiffusionView extends AphrontView {
       return $text;
     }
 
-    $drequest = $this->getDiffusionRequest();
+    $href = $this->getDiffusionRequest()->generateURI(
-
+      array(
-    if ($commit_identifier) {
+        'action'  => 'change',
-      $commit = ';'.$commit_identifier;
+        'path'    => $path,
-    } else if ($drequest->getRawCommit()) {
+        'commit'  => $commit_identifier,
-      $commit = ';'.$drequest->getCommitURIComponent($drequest->getRawCommit());
+      ));
-    } else {
-      $commit = null;
-    }
-
-    $repository = $drequest->getRepository();
-    $callsign = $repository->getCallsign();
-
-    $branch = $drequest->getBranchURIComponent($drequest->getBranch());
-    $path = $branch.($path ? $path : $drequest->getPath());
 
     return phutil_render_tag(
       'a',
       array(
-        'href' => "/diffusion/{$callsign}/change/{$path}{$commit}",
+        'href' => $href,
       ),
       $text);
   }
 
   final public function linkHistory($path) {
-    $drequest = $this->getDiffusionRequest();
+    $href = $this->getDiffusionRequest()->generateURI(
-
+      array(
-    if ($drequest->getRawCommit()) {
+        'action' => 'history',
-      $commit = ';'.$drequest->getCommitURIComponent($drequest->getRawCommit());
+        'path'   => $path,
-    } else {
+      ));
-      $commit = null;
-    }
-
-    $repository = $drequest->getRepository();
-    $callsign = $repository->getCallsign();
-
-    $branch = $drequest->getBranchURIComponent($drequest->getBranch());
-    $path = $branch.$path;
-
-    $text = 'History';
 
     return phutil_render_tag(
       'a',
       array(
-        'href' => "/diffusion/{$callsign}/history/{$path}{$commit}",
+        'href' => $href,
       ),
-      $text);
+      'History');
   }
 
   final public function linkBrowse($path, array $details = array()) {
-    $drequest = $this->getDiffusionRequest();
 
-    $raw_commit = idx($details, 'commit', $drequest->getRawCommit());
+    $href = $this->getDiffusionRequest()->generateURI(
-    if ($raw_commit) {
+      array(
-      $commit = ';'.$drequest->getCommitURIComponent($raw_commit);
+        'action' => 'browse',
-    } else {
+        'path'   => $path,
-      $commit = null;
+      ));
-    }
-
-    $repository = $drequest->getRepository();
-    $callsign = $repository->getCallsign();
-
-    $branch = $drequest->getBranchURIComponent($drequest->getBranch());
-    $path = $branch.$path;
 
     if (isset($details['text'])) {
       $text = phutil_escape_html($details['text']);
@@ -115,7 +88,7 @@ abstract class DiffusionView extends AphrontView {
     return phutil_render_tag(
       'a',
       array(
-        'href' => "/diffusion/{$callsign}/browse/{$path}{$commit}",
+        'href' => $href,
       ),
       $text);
   }

src/applications/diffusion/view/base/__init__.php

@@ -11,7 +11,6 @@ phutil_require_module('phabricator', 'applications/repository/constants/reposito
 phutil_require_module('phabricator', 'view/base');
 
 phutil_require_module('phutil', 'markup');
-phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('DiffusionView.php');

src/applications/diffusion/view/branchtable/DiffusionBranchTableView.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -29,18 +29,18 @@ final class DiffusionBranchTableView extends DiffusionView {
     $drequest = $this->getDiffusionRequest();
     $current_branch = $drequest->getBranch();
 
-    $callsign = $drequest->getRepository()->getCallsign();
-
     $rows = array();
     $rowc = array();
     foreach ($this->branches as $branch) {
-      $branch_uri = $drequest->getBranchURIComponent($branch->getName());
-
       $rows[] = array(
         phutil_render_tag(
           'a',
           array(
-            'href' => "/diffusion/{$callsign}/repository/{$branch_uri}",
+            'href' => $drequest->generateURI(
+              array(
+                'action' => 'branch',
+                'branch' => $branch->getName(),
+              )),
           ),
           phutil_escape_html($branch->getName())),
         self::linkCommit(

src/applications/diffusion/view/browsetable/DiffusionBrowseTableView.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -120,13 +120,13 @@ final class DiffusionBrowseTableView extends DiffusionView {
           'author'    => celerity_generate_unique_node_id(),
           'details'   => celerity_generate_unique_node_id(),
         );
-        $uri =
+
-          '/diffusion/'.$repository->getCallsign().'/lastmodified/'.
+        $uri = (string)$request->generateURI(
-          $request->getBranchURIComponent($request->getBranch()).
+          array(
-          $base_path.$path->getPath();
+            'action' => 'lastmodified',
-        if ($request->getRawCommit()) {
+            'path'   => $base_path.$path->getPath(),
-          $uri .= ';'.$request->getRawCommit();
+          ));
-        }
+
         $need_pull[$uri] = $dict;
         foreach ($dict as $k => $uniq) {
           $dict[$k] = '<span id="'.$uniq.'"></span>';

src/applications/owners/query/path/PhabricatorOwnerPathQuery.php

@@ -22,7 +22,12 @@ final class PhabricatorOwnerPathQuery {
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit) {
 
-    $drequest = self::buildDiffusionRequest($repository, $commit);
+    $drequest = DiffusionRequest::newFromAphrontRequestDictionary(
+      array(
+        'repository'  => $repository,
+        'commit'      => $commit->getCommitIdentifier(),
+      ));
+
     $path_query = DiffusionPathChangeQuery::newFromDiffusionRequest(
       $drequest);
     $paths = $path_query->loadChanges();
@@ -38,15 +43,4 @@ final class PhabricatorOwnerPathQuery {
     return $result;
   }
 
-  private static function buildDiffusionRequest(
-    PhabricatorRepository $repository,
-    PhabricatorRepositoryCommit $commit) {
-
-    return DiffusionRequest::newFromAphrontRequestDictionary(
-      array(
-        'callsign'  => $repository->getCallsign(),
-        'commit'    => $commit->getCommitIdentifier(),
-      ));
-  }
-
 }

src/applications/owners/storage/package/PhabricatorOwnersPackage.php

@@ -188,10 +188,10 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO {
         foreach ($paths as $path => $ignored) {
           $path = ltrim($path, '/');
           // build query to validate path
-          $drequest = DiffusionRequest::newFromAphrontRequestDictionary(
+          $drequest = DiffusionRequest::newFromDictionary(
             array(
-              'callsign' => $repository->getCallsign(),
+              'repository'  => $repository,
-              'path'     => ':/'.$path,
+              'path'        => $path,
             ));
           $query = DiffusionBrowseQuery::newFromDiffusionRequest($drequest);
           $query->needValidityOnly(true);

src/applications/repository/storage/symbol/PhabricatorRepositorySymbol.php

@@ -45,18 +45,13 @@ final class PhabricatorRepositorySymbol extends PhabricatorRepositoryDAO {
   }
 
   public function getURI() {
-    $repo = $this->getRepository();
+    return DiffusionRequest::generateDiffusionURI(
-    $file = $this->getPath();
-    $line = $this->getLineNumber();
-
-    $drequest = DiffusionRequest::newFromAphrontRequestDictionary(
       array(
-        'callsign' => $repo->getCallsign(),
+        'action'    => 'browse',
+        'callsign'  => $this->getRepository()->getCallsign(),
+        'path'      => $this->getPath(),
+        'line'      => $this->getLineNumber(),
       ));
-    $branch = $drequest->getBranchURIComponent($drequest->getBranch());
-    $file = $branch.ltrim($file, '/');
-
-    return '/diffusion/'.$repo->getCallsign().'/browse/'.$file.'$'.$line;
   }
 
   public function getPath() {