Merge lp:~jelmer/loggerhead/no-such-file into lp:loggerhead

[Jelmer Vernooij]

Hide file ids in loggerhead URLs.

This has the added benefit of fixing a bug in the download URL generation,
which didn't properly decode file id bytestrings on Python 3.

[lp:~jelmer/loggerhead/no-such-file]

501. By Jelmer Vernooij on 2020-06-04

Fix tests.

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'loggerhead/controllers/annotate_ui.py'
--- loggerhead/controllers/annotate_ui.py	2018-10-20 16:16:22 +0000
+++ loggerhead/controllers/annotate_ui.py	2020-06-04 20:32:36 +0000
@@ -32,7 +32,7 @@
             raise TypeError(revid)
         path = self._history.get_path(revid, file_id)
 
-        tree = self.tree_for(file_id, revid)
+        tree = self.tree_for(path, revid)
 
         change_cache = {}
         last_line_revid = None

=== modified file 'loggerhead/controllers/download_ui.py'
--- loggerhead/controllers/download_ui.py	2018-10-20 15:28:43 +0000
+++ loggerhead/controllers/download_ui.py	2020-06-04 20:32:36 +0000
@@ -22,7 +22,7 @@
 import urllib
 
 from breezy.errors import (
-    NoSuchId,
+    NoSuchFile,
     NoSuchRevision,
     )
 from breezy import osutils, urlutils
@@ -50,17 +50,16 @@
         return args
 
     def __call__(self, environ, start_response):
-        # /download/<rev_id>/<file_id>/[filename]
+        # /download/<rev_id>/<filename>
         h = self._history
         args = self.get_args(environ)
         if len(args) < 2:
             raise httpexceptions.HTTPMovedPermanently(
                 self._branch.absolute_url('/changes'))
         revid = h.fix_revid(args[0])
-        file_id = urlutils.unquote_to_bytes(osutils.safe_utf8(args[1]))
         try:
-            path, filename, content = h.get_file(file_id, revid)
-        except (NoSuchId, NoSuchRevision):
+            path, filename, content = h.get_file(args[1], revid)
+        except (NoSuchFile, NoSuchRevision):
             raise httpexceptions.HTTPNotFound()
         mime_type, encoding = mimetypes.guess_type(filename)
         if mime_type is None:

=== modified file 'loggerhead/controllers/view_ui.py'
--- loggerhead/controllers/view_ui.py	2020-01-19 20:09:08 +0000
+++ loggerhead/controllers/view_ui.py	2020-06-04 20:32:36 +0000
@@ -49,20 +49,19 @@
 
     template_name = 'view'
 
-    def tree_for(self, file_id, revid):
-        if not isinstance(file_id, bytes):
-            raise TypeError(file_id)
+    def tree_for(self, path, revid):
+        if not isinstance(path, str):
+            raise TypeError(path)
         if not isinstance(revid, bytes):
             raise TypeError(revid)
         rev_tree = self._history.revision_tree(revid)
-        file_revid = rev_tree.get_file_revision(rev_tree.id2path(file_id))
+        file_revid = rev_tree.get_file_revision(path)
         return self._history._branch.repository.revision_tree(file_revid)
 
-    def text_lines(self, file_id, revid):
-        path = self._history.get_path(revid, file_id)
+    def text_lines(self, path, revid):
         file_name = os.path.basename(path)
 
-        tree = self.tree_for(file_id, revid)
+        tree = self.tree_for(path, revid)
         file_text = tree.get_file_text(path)
 
         encoding = 'utf-8'
@@ -88,9 +87,9 @@
 
         return hl_lines
 
-    def file_contents(self, file_id, revid):
+    def file_contents(self, path, revid):
         try:
-            file_lines = self.text_lines(file_id, revid)
+            file_lines = self.text_lines(path, revid)
         except BinaryFile:
             # bail out; this isn't displayable text
             return ['(This is a binary file.)']
@@ -158,7 +157,7 @@
             'filename': filename,
             'navigation': navigation,
             'change': change,
-            'contents':  self.file_contents(file_id, revid),
+            'contents':  self.file_contents(path, revid),
             'fileview_active': True,
             'directory_breadcrumbs': directory_breadcrumbs,
             'branch_breadcrumbs': branch_breadcrumbs,

=== modified file 'loggerhead/history.py'
--- loggerhead/history.py	2020-05-06 19:18:04 +0000
+++ loggerhead/history.py	2020-06-04 20:32:36 +0000
@@ -773,14 +773,13 @@
         changes = self.get_file_changes(entry)
         entry.changes = changes
 
-    def get_file(self, file_id, revid):
+    def get_file(self, path, revid):
         """Returns (path, filename, file contents)"""
-        if not isinstance(file_id, bytes):
-            raise TypeError(file_id)
+        if not isinstance(path, str):
+            raise TypeError(path)
         if not isinstance(revid, bytes):
             raise TypeError(revid)
         rev_tree = self._branch.repository.revision_tree(revid)
-        path = rev_tree.id2path(file_id)
         display_path = path
         if not display_path.startswith('/'):
             path = '/' + path

=== modified file 'loggerhead/templates/view.pt'
--- loggerhead/templates/view.pt	2012-02-22 08:28:09 +0000
+++ loggerhead/templates/view.pt	2020-06-04 20:32:36 +0000
@@ -64,7 +64,7 @@
             </li>
             <li id="last">
                 <a tal:attributes="href python:url(['/download',
-                                    revno_url, file_id, filename])"
+                                    revno_url, file_path])"
                 >download file</a>
             </li>
         </ul>

=== modified file 'loggerhead/tests/test_controllers.py'
--- loggerhead/tests/test_controllers.py	2020-01-19 20:09:08 +0000
+++ loggerhead/tests/test_controllers.py	2020-06-04 20:32:36 +0000
@@ -387,7 +387,7 @@
 
     def test_download(self):
         app = self.setUpLoggerhead()
-        response = app.get('/download/1/myfilename-id/myfilename')
+        response = app.get('/download/1/myfilename')
         self.assertEqual(
             b'some\nmultiline\ndata\nwith<htmlspecialchars\n', response.body)
         self.assertThat(
@@ -398,14 +398,14 @@
         app = self.setUpLoggerhead()
         e = self.assertRaises(
             AppError,
-            app.get, '/download/norev/myfilename-id/myfilename')
+            app.get, '/download/norev/myfilename')
         self.assertContainsRe(str(e), '404 Not Found')
 
     def test_download_bad_fileid(self):
         app = self.setUpLoggerhead()
         e = self.assertRaises(
             AppError,
-            app.get, '/download/1/myfilename-notid/myfilename')
+            app.get, '/download/1/notmyfilename')
         self.assertContainsRe(str(e), '404 Not Found')