launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-translate-trailing-path into lp:launchpad.

Commit message:
Make GitLookup.getByPath and GitAPI.translatePath return extra trailing segments from a path, to support external code browsers.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1032731 in Launchpad itself: "Support for Launchpad-hosted Git repositories"
  https://bugs.launchpad.net/launchpad/+bug/1032731

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-translate-trailing-path/+merge/254551

Make GitLookup.getByPath and GitAPI.translatePath return extra trailing segments from a path, to support external code browsers.  We'll need this so that you can traverse to https://git.launchpad.net/path/to/repository/with/more/segments and have our cgit integration work out how to translate the leading part of the path to a physical repository.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-translate-trailing-path into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitlookup.py'
--- lib/lp/code/interfaces/gitlookup.py	2015-03-12 15:21:27 +0000
+++ lib/lp/code/interfaces/gitlookup.py	2015-03-30 09:59:40 +0000
@@ -139,5 +139,8 @@
                 PROJECT
                 DISTRO/+source/SOURCE
 
-        :return: An `IGitRepository`, or None.
+        :return: A tuple of (`IGitRepository`, extra_path), or (None, None).
+            'extra_path' may be used by applications that need to traverse a
+            leading part of a path as a repository, such as external code
+            browsers.
         """

=== modified file 'lib/lp/code/model/gitlookup.py'
--- lib/lp/code/model/gitlookup.py	2015-03-12 15:21:27 +0000
+++ lib/lp/code/model/gitlookup.py	2015-03-30 09:59:40 +0000
@@ -331,7 +331,10 @@
         path = self.uriToPath(uri)
         if path is None:
             return None
-        return self.getByPath(path)
+        path, extra_path = self.getByPath(path)
+        if extra_path:
+            return None
+        return path
 
     def getByUniqueName(self, unique_name):
         """See `IGitLookup`."""
@@ -349,16 +352,18 @@
     def getByPath(self, path):
         """See `IGitLookup`."""
         traverser = getUtility(IGitTraverser)
+        segments = iter(path.split("/"))
         try:
-            owner, target, repository = traverser.traverse_path(path)
+            owner, target, repository = traverser.traverse(segments)
         except (InvalidNamespace, InvalidProductName, NameLookupFailed):
-            return None
-        if repository is not None:
-            return repository
-        if IPerson.providedBy(target):
-            return None
-        repository_set = getUtility(IGitRepositorySet)
-        if owner is None:
-            return repository_set.getDefaultRepository(target)
-        else:
-            return repository_set.getDefaultRepositoryForOwner(owner, target)
+            return None, None
+        if repository is None:
+            if IPerson.providedBy(target):
+                return None, None
+            repository_set = getUtility(IGitRepositorySet)
+            if owner is None:
+                repository = repository_set.getDefaultRepository(target)
+            else:
+                repository = repository_set.getDefaultRepositoryForOwner(
+                    owner, target)
+        return repository, "/".join(segments)

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-03-24 13:37:54 +0000
+++ lib/lp/code/model/gitrepository.py	2015-03-30 09:59:40 +0000
@@ -602,8 +602,8 @@
 
     def getByPath(self, user, path):
         """See `IGitRepositorySet`."""
-        repository = getUtility(IGitLookup).getByPath(path)
-        if repository is None:
+        repository, extra_path = getUtility(IGitLookup).getByPath(path)
+        if repository is None or extra_path:
             return None
         # removeSecurityProxy is safe here since we're explicitly performing
         # a permission check.

=== modified file 'lib/lp/code/model/tests/test_gitlookup.py'
--- lib/lp/code/model/tests/test_gitlookup.py	2015-03-12 15:21:27 +0000
+++ lib/lp/code/model/tests/test_gitlookup.py	2015-03-30 09:59:40 +0000
@@ -100,7 +100,7 @@
     def test_project(self):
         repository = self.factory.makeGitRepository()
         self.assertEqual(
-            repository, self.lookup.getByPath(repository.unique_name))
+            (repository, ""), self.lookup.getByPath(repository.unique_name))
 
     def test_project_default(self):
         repository = self.factory.makeGitRepository()
@@ -108,13 +108,13 @@
             getUtility(IGitRepositorySet).setDefaultRepository(
                 repository.target, repository)
         self.assertEqual(
-            repository, self.lookup.getByPath(repository.shortened_path))
+            (repository, ""), self.lookup.getByPath(repository.shortened_path))
 
     def test_package(self):
         dsp = self.factory.makeDistributionSourcePackage()
         repository = self.factory.makeGitRepository(target=dsp)
         self.assertEqual(
-            repository, self.lookup.getByPath(repository.unique_name))
+            (repository, ""), self.lookup.getByPath(repository.unique_name))
 
     def test_package_default(self):
         dsp = self.factory.makeDistributionSourcePackage()
@@ -123,31 +123,37 @@
             getUtility(IGitRepositorySet).setDefaultRepository(
                 repository.target, repository)
         self.assertEqual(
-            repository, self.lookup.getByPath(repository.shortened_path))
+            (repository, ""), self.lookup.getByPath(repository.shortened_path))
 
     def test_personal(self):
         owner = self.factory.makePerson()
         repository = self.factory.makeGitRepository(owner=owner, target=owner)
         self.assertEqual(
-            repository, self.lookup.getByPath(repository.unique_name))
+            (repository, ""), self.lookup.getByPath(repository.unique_name))
+
+    def test_extra_path(self):
+        repository = self.factory.makeGitRepository()
+        self.assertEqual(
+            (repository, "foo/bar"),
+            self.lookup.getByPath("%s/foo/bar" % repository.unique_name))
 
     def test_invalid_namespace(self):
         # If `getByPath` is given a path to something with no default Git
-        # repository, such as a distribution, it returns None.
+        # repository, such as a distribution, it returns (None, _).
         distro = self.factory.makeDistribution()
-        self.assertIsNone(self.lookup.getByPath(distro.name))
+        self.assertIsNone(self.lookup.getByPath(distro.name)[0])
 
     def test_no_default_git_repository(self):
         # If `getByPath` is given a path to something that could have a Git
-        # repository but doesn't, it returns None.
+        # repository but doesn't, it returns (None, _).
         project = self.factory.makeProduct()
-        self.assertIsNone(self.lookup.getByPath(project.name))
+        self.assertIsNone(self.lookup.getByPath(project.name)[0])
 
     def test_bare_person(self):
         # If `getByPath` is given a path to a person but nothing further, it
-        # returns None even if the person exists.
+        # returns (None, _) even if the person exists.
         owner = self.factory.makePerson()
-        self.assertIsNone(self.lookup.getByPath("~%s" % owner.name))
+        self.assertIsNone(self.lookup.getByPath("~%s" % owner.name)[0])
 
 
 class TestGetByUrl(TestCaseWithFactory):
@@ -179,6 +185,12 @@
         self.assertUrlMatches(
             "git://git.launchpad.dev/~aa/bb/+git/cc/", repository)
 
+    def test_getByUrl_with_trailing_segments(self):
+        # URLs with trailing segments beyond the repository are rejected.
+        self.makeProjectRepository()
+        self.assertIsNone(
+            self.lookup.getByUrl("git://git.launchpad.dev/~aa/bb/+git/cc/foo"))
+
     def test_getByUrl_with_git(self):
         # getByUrl recognises LP repositories for git URLs.
         repository = self.makeProjectRepository()

=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py	2015-03-16 12:12:35 +0000
+++ lib/lp/code/xmlrpc/git.py	2015-03-30 09:59:40 +0000
@@ -69,7 +69,7 @@
             config.codehosting.internal_git_api_endpoint)
 
     def _performLookup(self, path):
-        repository = getUtility(IGitLookup).getByPath(path)
+        repository, extra_path = getUtility(IGitLookup).getByPath(path)
         if repository is None:
             return None
         try:
@@ -77,7 +77,11 @@
         except Unauthorized:
             raise faults.PermissionDenied()
         writable = check_permission("launchpad.Edit", repository)
-        return {"path": hosting_path, "writable": writable}
+        return {
+            "path": hosting_path,
+            "writable": writable,
+            "trailing": extra_path,
+            }
 
     def _getGitNamespaceExtras(self, path, requester):
         """Get the namespace, repository name, and callback for the path.

=== modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
--- lib/lp/code/xmlrpc/tests/test_git.py	2015-03-16 12:12:35 +0000
+++ lib/lp/code/xmlrpc/tests/test_git.py	2015-03-30 09:59:40 +0000
@@ -84,13 +84,15 @@
             requester, path.lstrip("/"))
         self.assertIsNotNone(repository)
         self.assertEqual(
-            {"path": repository.getInternalPath(), "writable": True},
+            {"path": repository.getInternalPath(), "writable": True,
+             "trailing": ""},
             translation)
         translation = self.git_api.translatePath(
             path, "write", requester.id, True)
         login(ANONYMOUS)
         self.assertEqual(
-            {"path": repository.getInternalPath(), "writable": True},
+            {"path": repository.getInternalPath(), "writable": True,
+             "trailing": ""},
             translation)
         # But we cannot create another one without the feature flag.
         message = "You do not have permission to create Git repositories."
@@ -181,14 +183,16 @@
         return fault.faultString[len(prefix):].rstrip(".")
 
     def assertTranslates(self, requester, path, repository, writable,
-                         permission="read", can_authenticate=False):
+                         permission="read", can_authenticate=False,
+                         trailing=""):
         if requester not in (LAUNCHPAD_ANONYMOUS, LAUNCHPAD_SERVICES):
             requester = requester.id
         translation = self.git_api.translatePath(
             path, permission, requester, can_authenticate)
         login(ANONYMOUS)
         self.assertEqual(
-            {"path": repository.getInternalPath(), "writable": writable},
+            {"path": repository.getInternalPath(), "writable": writable,
+             "trailing": trailing},
             translation)
 
     def assertCreates(self, requester, path, can_authenticate=False):
@@ -204,7 +208,8 @@
         self.assertIsNotNone(repository)
         self.assertEqual(requester, repository.registrant)
         self.assertEqual(
-            {"path": repository.getInternalPath(), "writable": True},
+            {"path": repository.getInternalPath(), "writable": True,
+             "trailing": ""},
             translation)
         self.assertEqual(
             [("create", repository.getInternalPath())],
@@ -355,8 +360,9 @@
     def test_translatePath_repository_with_trailing_segments(self):
         requester = self.factory.makePerson()
         repository = self.factory.makeGitRepository()
-        path = u"/%s/junk" % repository.unique_name
-        self.assertPathTranslationError(requester, path)
+        path = u"/%s/foo/bar" % repository.unique_name
+        self.assertTranslates(
+            requester, path, repository, False, trailing="foo/bar")
 
     def test_translatePath_no_such_repository(self):
         requester = self.factory.makePerson()