Merge pull request #259 from NateEag/fix-branches-with-slashes

Correctly support branch names with slashes

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>