Correctly support branch names with slashes

Previously, branches with slashes in the name would consume all slashed
segments in a URL, causing the routes to capture incorrect file paths.

This solution is not particularly elegant - anywhere a route used
both a commit-ish identifier and a path, we collapse those two params
into a single param, and parse that param inside the route.

It seems to be working reasonably reliably, but has not seen extensive
testing.

It is also not particularly pretty. If anyone sees ways to improve it,
please, have at it.

src/GitList/Controller/BlobController.php

@@ -12,8 +12,12 @@ class BlobController implements ControllerProviderInterface
     {
         $route = $app['controllers_factory'];
 
-        $route->get('{repo}/blob/{branch}/{file}', function($repo, $branch, $file) use ($app) {
+        $route->get('{repo}/blob/{commitish_path}', function($repo, $commitish_path) use ($app) {
             $repository = $app['git']->getRepository($app['git.repos'] . $repo);
+
+            list($branch, $file) = $app['util.routing']
+                ->parseCommitishPathParam($commitish_path, $repo);
+
             list($branch, $file) = $app['util.repository']->extractRef($repository, $branch, $file);
 
             $blob = $repository->getBlob("$branch:\"$file\"");
@@ -38,14 +42,18 @@ class BlobController implements ControllerProviderInterface
                 'branches'       => $repository->getBranches(),
                 'tags'           => $repository->getTags(),
             ));
-        })->assert('file', '.+')
-          ->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
+        })->assert('repo', $app['util.routing']->getRepositoryRegex())
+          ->assert('commitish_path', '.+')
           ->bind('blob');
 
-        $route->get('{repo}/raw/{branch}/{file}', function($repo, $branch, $file) use ($app) {
+        $route->get('{repo}/raw/{commitish_path}', function($repo, $commitish_path) use ($app) {
             $repository = $app['git']->getRepository($app['git.repos'] . $repo);
+
+            list($branch, $file) = $app['util.routing']
+                ->parseCommitishPathParam($commitish_path, $repo);
+
             list($branch, $file) = $app['util.repository']->extractRef($repository, $branch, $file);
+
             $blob = $repository->getBlob("$branch:\"$file\"")->output();
 
             $headers = array();
@@ -58,9 +66,8 @@ class BlobController implements ControllerProviderInterface
             }
 
             return new Response($blob, 200, $headers);
-        })->assert('file', '.+')
-          ->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
+        })->assert('repo', $app['util.routing']->getRepositoryRegex())
+          ->assert('commitish_path', $app['util.routing']->getCommitishPathRegex())
           ->bind('blob_raw');
 
         return $route;

src/GitList/Controller/CommitController.php

@@ -12,13 +12,16 @@ class CommitController implements ControllerProviderInterface
     {
         $route = $app['controllers_factory'];
 
-        $route->get('{repo}/commits/{branch}/{file}', function($repo, $branch, $file) use ($app) {
+        $route->get('{repo}/commits/{commitish_path}', function($repo, $commitish_path) use ($app) {
             $repository = $app['git']->getRepository($app['git.repos'] . $repo);
 
-            if ($branch === null) {
-                $branch = $repository->getHead();
+            if ($commitish_path === null) {
+                $commitish_path = $repository->getHead();
             }
 
+            list($branch, $file) = $app['util.routing']
+                ->parseCommitishPathParam($commitish_path, $repo);
+
             list($branch, $file) = $app['util.repository']->extractRef($repository, $branch, $file);
 
             $type = $file ? "$branch -- \"$file\"" : $branch;
@@ -45,10 +48,8 @@ class CommitController implements ControllerProviderInterface
                 'file'           => $file,
             ));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
-          ->assert('file', '.+')
-          ->value('branch', null)
-          ->value('file', '')
+          ->assert('commitish_path', $app['util.routing']->getCommitishPathRegex())
+          ->value('commitish_path', null)
           ->bind('commits');
 
         $route->post('{repo}/commits/{branch}/search', function(Request $request, $repo, $branch = '') use ($app) {
@@ -73,7 +74,7 @@ class CommitController implements ControllerProviderInterface
                 'query'          => $query
             ));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
+          ->assert('branch', $app['util.routing']->getBranchRegex())
           ->bind('searchcommits');
 
         $route->get('{repo}/commit/{commit}', function($repo, $commit) use ($app) {
@@ -90,9 +91,12 @@ class CommitController implements ControllerProviderInterface
           ->assert('commit', '[a-f0-9^]+')
           ->bind('commit');
 
-        $route->get('{repo}/blame/{branch}/{file}', function($repo, $branch, $file) use ($app) {
+        $route->get('{repo}/blame/{branch_file}', function($repo, $branch_file) use ($app) {
             $repository = $app['git']->getRepository($app['git.repos'] . $repo);
 
+            list($branch, $file) = $app['util.routing']
+                ->parseCommitishPathParam($branch_file, $repo);
+
             list($branch, $file) = $app['util.repository']->extractRef($repository, $branch, $file);
 
             $blames = $repository->getBlame("$branch -- \"$file\"");
@@ -106,8 +110,7 @@ class CommitController implements ControllerProviderInterface
                 'blames'         => $blames,
             ));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('file', '.+')
-          ->assert('branch', '[\w-._\/]+')
+          ->assert('branch_file', $app['util.routing']->getCommitishPathRegex())
           ->bind('blame');
 
         return $route;

src/GitList/Controller/MainController.php

@@ -46,7 +46,7 @@ class MainController implements ControllerProviderInterface
                 'authors'         => $authors,
             ));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
+          ->assert('branch', $app['util.routing']->getBranchRegex())
           ->value('branch', null)
           ->bind('stats');
 
@@ -62,7 +62,7 @@ class MainController implements ControllerProviderInterface
 
             return new Response($html, 200, array('Content-Type' => 'application/rss+xml'));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
+          ->assert('branch', $app['util.routing']->getBranchRegex())
           ->bind('rss');
 
         return $route;

src/GitList/Controller/TreeController.php

@@ -13,12 +13,14 @@ class TreeController implements ControllerProviderInterface
     {
         $route = $app['controllers_factory'];
 
-        $route->get('{repo}/tree/{branch}/{tree}/', $treeController = function($repo, $branch = '', $tree = '') use ($app) {
+        $route->get('{repo}/tree/{commitish_path}/', $treeController = function($repo, $commitish_path = '') use ($app) {
             $repository = $app['git']->getRepository($app['git.repos'] . $repo);
-            if (!$branch) {
-                $branch = $repository->getHead();
+            if (!$commitish_path) {
+                $commitish_path = $repository->getHead();
             }
 
+            list($branch, $tree) = $app['util.routing']->parseCommitishPathParam($commitish_path, $repo);
+
             list($branch, $tree) = $app['util.repository']->extractRef($repository, $branch, $tree);
             $files = $repository->getTree($tree ? "$branch:\"$tree\"/" : $branch);
             $breadcrumbs = $app['util.view']->getBreadcrumbs($tree);
@@ -42,8 +44,7 @@ class TreeController implements ControllerProviderInterface
                 'readme'         => $app['util.repository']->getReadme($repo, $branch),
             ));
         })->assert('repo', $app['util.routing']->getRepositoryRegex())
-          ->assert('branch', '[\w-._\/]+')
-          ->assert('tree', '.+')
+          ->assert('commitish_path', $app['util.routing']->getCommitishPathRegex())
           ->bind('tree');
 
         $route->post('{repo}/tree/{branch}/search', function(Request $request, $repo, $branch = '', $tree = '') use ($app) {

src/GitList/Git/Repository.php

@@ -10,6 +10,20 @@ use Symfony\Component\Filesystem\Filesystem;
 
 class Repository extends BaseRepository
 {
+    /**
+     * Return TRUE if the repo contains this commit.
+     *
+     * @param $commitHash Hash of commit whose existence we want to check
+     * @return boolean    Whether or not the commit exists in this repo
+     */
+    public function hasCommit($commitHash)
+    {
+        $logs = $this->getClient()->run($this, "show $commitHash");
+        $logs = explode("\n", $logs);
+
+        return strpos($logs[0], 'commit') === 0;
+    }
+
     /**
      * Show the data from a specific commit
      *
@@ -313,4 +327,23 @@ class Repository extends BaseRepository
         $fs->mkdir(dirname($output));
         $this->getClient()->run($this, "archive --format=$format --output=$output $tree");
     }
+
+    /**
+     * Return TRUE if $path exists in $branch; return FALSE otherwise.
+     *
+     * @param string $commitish Commitish reference; branch, tag, SHA1, etc.
+     * @param string $path Path whose existence we want to verify.
+     *
+     * GRIPE Arguably belongs in Gitter, as it's generally useful functionality.
+     * Also, this really may not be the best way to do this.
+     */
+    public function pathExists($commitish, $path) {
+        $output = $this->getClient()->run($this, "ls-tree $commitish $path");
+
+        if (strlen($output) > 0) {
+            return TRUE;
+        }
+
+        return FALSE;
+    }
 }

src/GitList/Util/Routing.php

@@ -13,6 +13,93 @@ class Routing
         $this->app = $app;
     }
 
+    /* @brief Return $commitish, $path parsed from $commitish_path, based on
+     * what's in $repo. Raise a 404 if $branchpath does not represent a
+     * valid branch and path.
+     *
+     * A helper for parsing routes that use commit-ish names and paths
+     * separated by /, since route regexes are not enough to get that right.
+     */
+    public function parseCommitishPathParam($commitish_path, $repo) {
+        $app = $this->app;
+        $repository = $app['git']->getRepository($app['git.repos'] . $repo);
+
+        $commitish = null;
+        $path = null;
+
+        $slash_pos = strpos($commitish_path, '/');
+        if (strlen($commitish_path) >= 40 &&
+            ($slash_pos === FALSE ||
+             $slash_pos === 40)) {
+            // We may have a commit hash as our commitish.
+            $hash = substr($commitish_path, 0, 40);
+            if ($repository->hasCommit($hash)) {
+                $commitish = $hash;
+            }
+        }
+
+        if ($commitish === null) {
+            // DEBUG Can you have a repo with no branches? How should we handle
+            // that?
+            $branches = $repository->getBranches();
+
+            $tags = $repository->getTags();
+            if ($tags !== null && count($tags) > 0) {
+                $branches = array_merge($branches, $tags);
+            }
+
+            $matched_branch = null;
+            $matched_branch_name_len = 0;
+            foreach ($branches as $branch) {
+                if (strpos($commitish_path, $branch) === 0 &&
+                    strlen($branch) > $matched_branch_name_len) {
+                    $matched_branch = $branch;
+                    $matched_branch_name_len = strlen($matched_branch);
+                }
+            }
+
+            $commitish = $matched_branch;
+        }
+
+        if ($commitish === null) {
+            $app->abort(404, "'$branch_path' does not appear to contain a " .
+                             "commit-ish for '$repo.'");
+        }
+
+        $commitish_len = strlen($commitish);
+        $path = substr($commitish_path, $commitish_len);
+        if (strpos($path, '/') === 0) {
+            $path = substr($path, 1);
+        }
+
+        $commit_has_path = $repository->pathExists($commitish, $path);
+        if ($commit_has_path !== TRUE) {
+            $app->abort(404, "\"$path\" does not exist in \"$commitish\".");
+        }
+
+        return array($commitish, $path);
+    }
+
+    public function getBranchRegex() {
+        static $branch_regex = null;
+
+        if ($branch_regex === null) {
+            $branch_regex = '[\w-._\/]+';
+        }
+
+        return $branch_regex;
+    }
+
+    public function getCommitishPathRegex() {
+        static $commitish_path_regex = null;
+
+        if ($commitish_path_regex === null) {
+            $commitish_path_regex = '.+';
+        }
+
+        return $commitish_path_regex;
+    }
+
     public function getRepositoryRegex()
     {
         static $regex = null;

views/breadcrumb.twig

@@ -2,7 +2,7 @@
     <li><a href="{{ path('repository', {repo: repo}) }}">{{ repo }}</a></li>
     {% for breadcrumb in breadcrumbs %}
         <span class="divider">/</span>
-        <li{% if loop.last %} class="active"{% endif %}>{% if not loop.last %}<a href="{{ path('tree', {repo: repo, branch: branch, tree: breadcrumb.path}) }}">{{ breadcrumb.dir }}</a>{% endif %}{% if loop.last %}{{ breadcrumb.dir }}{% endif %}</li>
+        <li{% if loop.last %} class="active"{% endif %}>{% if not loop.last %}<a href="{{ path('tree', {repo: repo, commitish_path: branch ~ '/' ~ breadcrumb.path}) }}">{{ breadcrumb.dir }}</a>{% endif %}{% if loop.last %}{{ breadcrumb.dir }}{% endif %}</li>
     {% endfor %}
 
     {% block extra %}{% endblock %}

views/commit.twig

@@ -30,8 +30,8 @@
             <div class="meta"><a name="{{ loop.index }}">{{ diff.file }}</div>
 
             <div class="btn-group pull-right">
-                <a href="{{ path('commits', {repo: repo, branch: commit.hash, file: diff.file}) }}" class="btn btn-small"><i class="icon-list-alt"></i> History</a>
-                <a href="{{ path('blob', {repo: repo, branch: commit.hash, file: diff.file}) }}" class="btn btn-small"><i class="icon-file"></i> View file @ {{ commit.shortHash }}</a>
+                <a href="{{ path('commits', {repo: repo, commitish_path: commit.hash ~ '/' ~ diff.file}) }}" class="btn btn-small"><i class="icon-list-alt"></i> History</a>
+                <a href="{{ path('blob', {repo: repo, commitish_path: commit.hash ~'/' ~ diff.file}) }}" class="btn btn-small"><i class="icon-file"></i> View file @ {{ commit.shortHash }}</a>
             </div>
         </div>
 

views/file.twig

@@ -12,9 +12,9 @@
             <div class="meta"></div>
 
             <div class="btn-group pull-right">
-                <a href="{{ path('blob_raw', {repo: repo, branch: branch, file: file}) }}" class="btn btn-small"><i class="icon-file"></i> Raw</a>
-                <a href="{{ path('blame', {repo: repo, branch: branch, file: file}) }}" class="btn btn-small"><i class="icon-bullhorn"></i> Blame</a>
-                <a href="{{ path('commits', {repo: repo, branch: branch, file: file}) }}" class="btn btn-small"><i class="icon-list-alt"></i> History</a>
+                <a href="{{ path('blob_raw', {repo: repo, commitish_path: branch ~ '/' ~ file}) }}" class="btn btn-small"><i class="icon-file"></i> Raw</a>
+                <a href="{{ path('blame', {repo: repo, branch_file: branch ~ '/' ~ file}) }}" class="btn btn-small"><i class="icon-bullhorn"></i> Blame</a>
+                <a href="{{ path('commits', {repo: repo, commitish_path: branch ~ '/' ~ file}) }}" class="btn btn-small"><i class="icon-list-alt"></i> History</a>
             </div>
         </div>
         {% if fileType == 'image' %}

views/menu.twig

@@ -1,5 +1,5 @@
 <ul class="nav nav-tabs">
     <li{% if page == 'files' %} class="active"{% endif %}><a href="{{ path('branch', {repo: repo, branch: branch}) }}">Files</a></li>
-    <li{% if page in ['commits', 'searchcommits'] %} class="active"{% endif %}><a href="{{ path('commits', {repo: repo, branch: branch}) }}">Commits</a></li>
+    <li{% if page in ['commits', 'searchcommits'] %} class="active"{% endif %}><a href="{{ path('commits', {repo: repo, commitish_path: branch}) }}">Commits</a></li>
     <li{% if page == 'stats' %} class="active"{% endif %}><a href="{{ path('stats', {repo: repo, branch: branch}) }}">Stats</a></li>
 </ul>

views/tree.twig

@@ -32,7 +32,7 @@
                     {% if not parent %}
                         <a href="{{ path('branch', {repo: repo, branch: branch}) }}">..</a>
                     {% else %}
-                        <a href="{{ path('tree', {repo: repo, branch: branch, tree: parent}) }}">..</a>
+                        <a href="{{ path('tree', {repo: repo, commitish_path: branch ~ '/' ~ parent}) }}">..</a>
                     {% endif %}
                 </td>
                 <td></td>
@@ -43,9 +43,9 @@
             <tr>
                 <td><i class="{{ file.type == "folder" or file.type == "symlink" ? "icon-folder-open" : "icon-file" }} icon-spaced"></i> <a href="
                     {%- if file.type == "folder" or file.type == "symlink" -%}
-                        {{ path('tree', {repo: repo, branch: branch, tree: path ~ (file.type == "symlink" ? file.path : file.name)}) }}
+                        {{ path('tree', {repo: repo, commitish_path: branch ~ '/' ~ path ~ (file.type == "symlink" ? file.path : file.name)}) }}
                     {%- else -%}
-                        {{ path('blob', {repo: repo, branch: branch, file: path ~ (file.type == "symlink" ? file.path : file.name)}) }}
+                        {{ path('blob', {repo: repo, commitish_path: branch ~ '/' ~ path ~ (file.type == "symlink" ? file.path : file.name)}) }}
                     {%- endif -%}
                 ">{{ file.name }}</a></td>
                 <td>{{ file.mode }}</td>