launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Make GitLookup.getByPath handle trailing path segments on objects which are also traversable.

Requested reviews:
  Colin Watson (cjwatson)
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-defaults/+merge/254587

Make GitLookup.getByPath handle trailing path segments on objects which are also traversable.

This fixes a case I missed in https://code.launchpad.net/~cjwatson/launchpad/git-translate-trailing-path/+merge/254551: for example, for cgit support, /PROJECT/commit/?id=sha1 needs to be translatable to the default repository for PROJECT with some trailing path information, even though the traversal code knows how to traverse from projects.  This requires some adjustment of internal interfaces since we may have to push back a segment.
-- 
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/interfaces/gitlookup.py'
--- lib/lp/code/interfaces/gitlookup.py	2015-03-30 10:00:48 +0000
+++ lib/lp/code/interfaces/gitlookup.py	2015-03-30 14:55:25 +0000
@@ -53,7 +53,8 @@
         :return: A tuple of::
             * an `IPerson`, or None;
             * an `IHasGitRepositories`;
-            * an `IGitRepository`, or None.
+            * an `IGitRepository`, or None;
+            * a trailing path segment, or None.
         """
 
     def traverse_path(path):

=== modified file 'lib/lp/code/model/gitlookup.py'
--- lib/lp/code/model/gitlookup.py	2015-03-30 09:46:03 +0000
+++ lib/lp/code/model/gitlookup.py	2015-03-30 14:55:25 +0000
@@ -261,26 +261,34 @@
         else:
             target = owner
             traversable = adapt(owner, IGitTraversable)
+        trailing = None
         segments_iter = SegmentIterator(segments)
         while traversable is not None:
             try:
                 name = next(segments_iter)
             except StopIteration:
                 break
-            owner, target, repository = traversable.traverse(
-                owner, name, segments_iter)
+            try:
+                owner, target, repository = traversable.traverse(
+                    owner, name, segments_iter)
+            except InvalidNamespace:
+                if target is not None or repository is not None:
+                    # We have some information, so the rest may consist of
+                    # trailing path information.
+                    trailing = name
+                    break
             if repository is not None:
                 break
             traversable = adapt(target, IGitTraversable)
         if target is None or not IHasGitRepositories.providedBy(target):
             raise InvalidNamespace("/".join(segments_iter.traversed))
-        return owner, target, repository
+        return owner, target, repository, trailing
 
     def traverse_path(self, path):
         """See `IGitTraverser`."""
         segments = iter(path.split("/"))
-        owner, target, repository = self.traverse(segments)
-        if list(segments):
+        owner, target, repository, trailing = self.traverse(segments)
+        if trailing or list(segments):
             raise InvalidNamespace(path)
         return owner, target, repository
 
@@ -340,9 +348,10 @@
         """See `IGitLookup`."""
         try:
             if unique_name.startswith("~"):
+                traverser = getUtility(IGitTraverser)
                 segments = iter(unique_name.split("/"))
-                _, _, repository = getUtility(IGitTraverser).traverse(segments)
-                if repository is None or list(segments):
+                _, _, repository, trailing = traverser.traverse(segments)
+                if repository is None or trailing or list(segments):
                     raise InvalidNamespace(unique_name)
                 return repository
         except (InvalidNamespace, NameLookupFailed):
@@ -354,7 +363,7 @@
         traverser = getUtility(IGitTraverser)
         segments = iter(path.split("/"))
         try:
-            owner, target, repository = traverser.traverse(segments)
+            owner, target, repository, trailing = traverser.traverse(segments)
         except (InvalidNamespace, InvalidProductName, NameLookupFailed):
             return None, None
         if repository is None:
@@ -366,4 +375,7 @@
             else:
                 repository = repository_set.getDefaultRepositoryForOwner(
                     owner, target)
-        return repository, "/".join(segments)
+        trailing_segments = list(segments)
+        if trailing:
+            trailing_segments.insert(0, trailing)
+        return repository, "/".join(trailing_segments)

=== modified file 'lib/lp/code/model/tests/test_gitlookup.py'
--- lib/lp/code/model/tests/test_gitlookup.py	2015-03-30 09:46:03 +0000
+++ lib/lp/code/model/tests/test_gitlookup.py	2015-03-30 14:55:25 +0000
@@ -137,6 +137,15 @@
             (repository, "foo/bar"),
             self.lookup.getByPath("%s/foo/bar" % repository.unique_name))
 
+    def test_default_extra_path(self):
+        repository = self.factory.makeGitRepository()
+        with person_logged_in(repository.target.owner):
+            getUtility(IGitRepositorySet).setDefaultRepository(
+                repository.target, repository)
+        self.assertEqual(
+            (repository, "foo/bar"),
+            self.lookup.getByPath("%s/foo/bar" % repository.shortened_path))
+
     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, _).
@@ -502,8 +511,8 @@
             owner=person, target=person, name=u"repository")
         segments = ["~person", "+git", "repository"]
         self.assertEqual(
-            (person, person, repository),
+            (person, person, repository, None),
             self.traverser.traverse(iter(segments)))
         self.assertEqual(
-            (person, person, repository),
+            (person, person, repository, None),
             self.traverser.traverse(iter(segments[1:]), owner=person))

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2015-03-17 16:22:56 +0000
+++ lib/lp/registry/browser/person.py	2015-03-30 14:55:25 +0000
@@ -375,12 +375,15 @@
             num_segments = len(segments)
             iter_segments = iter(segments)
             traverser = getUtility(IGitTraverser)
-            _, target, repository = traverser.traverse(
+            _, target, repository, trailing = traverser.traverse(
                 iter_segments, owner=self.context)
             if repository is None:
                 raise NotFoundError
             # Subtract one because the pillar has already been traversed.
-            for _ in range(num_segments - len(list(iter_segments)) - 1):
+            num_traversed = num_segments - len(list(iter_segments)) - 1
+            if trailing:
+                num_traversed -= 1
+            for _ in range(num_traversed):
                 self.request.stepstogo.consume()
 
             if IProduct.providedBy(target):