launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-ref-link into lp:launchpad.

Commit message:
Add fmt:link support for GitRef; remove refs/heads/ from the canonical versions of GitRef URLs (the long forms are still acceptable).

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

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-ref-link/+merge/256909

Add fmt:link support for GitRef; remove refs/heads/ from the canonical versions of GitRef URLs (the long forms are still acceptable).

This is part of getting merge proposals working.  Stripping off refs/heads/ isn't required for that, but I think it makes things more pleasant and doesn't introduce any ambiguity that Git doesn't have already.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-ref-link into lp:launchpad.

=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2015-03-13 14:16:05 +0000
+++ lib/lp/app/browser/configure.zcml	2015-04-21 10:55:54 +0000
@@ -736,6 +736,13 @@
       />
 
   <adapter
+      for="lp.code.interfaces.gitref.IGitRef"
+      provides="zope.traversing.interfaces.IPathAdapter"
+      factory="lp.app.browser.tales.GitRefFormatterAPI"
+      name="fmt"
+      />
+
+  <adapter
       for="lp.bugs.interfaces.bugbranch.IBugBranch"
       provides="zope.traversing.interfaces.IPathAdapter"
       factory="lp.app.browser.tales.BugBranchFormatterAPI"

=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py	2015-03-24 13:36:23 +0000
+++ lib/lp/app/browser/tales.py	2015-04-21 10:55:54 +0000
@@ -1683,6 +1683,15 @@
         return {'display_name': self._context.display_name}
 
 
+class GitRefFormatterAPI(CustomizableFormatter):
+    """Adapter for IGitRef objects to a formatted string."""
+
+    _link_summary_template = '%(display_name)s'
+
+    def _link_summary_values(self):
+        return {'display_name': self._context.display_name}
+
+
 class BugBranchFormatterAPI(CustomizableFormatter):
     """Adapter providing fmt support for BugBranch objects"""
 

=== modified file 'lib/lp/app/doc/tales.txt'
--- lib/lp/app/doc/tales.txt	2015-03-24 13:36:23 +0000
+++ lib/lp/app/doc/tales.txt	2015-04-21 10:55:54 +0000
@@ -395,6 +395,7 @@
   * people / teams
   * branches
   * Git repositories
+  * Git references
   * bugs
   * bug subscriptions
   * bug tasks
@@ -524,7 +525,7 @@
 Git repositories
 ................
 
-For Git repositories, fmt:link links to the branch page.
+For Git repositories, fmt:link links to the repository page.
 
     >>> from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
     >>> from lp.services.features.testing import FeatureFixture
@@ -535,6 +536,18 @@
     <a href=".../~eric/fooix/+git/bar">lp:~eric/fooix/+git/bar</a>
 
 
+Git references
+..............
+
+For Git references, fmt:link links to the reference page.
+
+    >>> with FeatureFixture({GIT_FEATURE_FLAG: 'on'}):
+    ...     [ref] = factory.makeGitRefs(
+    ...         repository=repository, paths=[u"master"])
+    ...     print test_tales("ref/fmt:link", ref=ref)
+    <a href=".../~eric/fooix/+git/bar/+ref/master">~eric/fooix/+git/bar:master</a>
+
+
 Bugs
 ....
 

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2015-04-19 12:56:32 +0000
+++ lib/lp/code/browser/configure.zcml	2015-04-21 10:55:54 +0000
@@ -779,7 +779,7 @@
         name="+index"/>
     <browser:url
         for="lp.code.interfaces.gitref.IGitRef"
-        path_expression="string:+ref/${path}"
+        path_expression="string:+ref/${name}"
         attribute_to_parent="repository"
         rootsite="code"/>
     <browser:pages

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2015-04-16 04:00:33 +0000
+++ lib/lp/code/interfaces/gitref.py	2015-04-21 10:55:54 +0000
@@ -51,6 +51,10 @@
         description=_(
             "The full path of this reference, e.g. refs/heads/master.")))
 
+    name = Attribute(
+        "A shortened version of the full path to this reference, with any "
+        "leading refs/heads/ removed.")
+
     commit_sha1 = exported(TextLine(
         title=_("Commit SHA-1"), required=True, readonly=True,
         description=_(

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2015-04-16 04:00:33 +0000
+++ lib/lp/code/model/gitref.py	2015-04-21 10:55:54 +0000
@@ -12,7 +12,6 @@
     DateTime,
     Int,
     Reference,
-    Store,
     Unicode,
     )
 from zope.interface import implements
@@ -33,10 +32,12 @@
 
     @property
     def display_name(self):
-        return self.path.split("/", 2)[-1]
+        """See `IGitRef`."""
+        return self.identity
 
     @property
-    def _branch_name(self):
+    def name(self):
+        """See `IGitRef`."""
         if self.path.startswith("refs/heads/"):
             return self.path[len("refs/heads/"):]
         else:
@@ -44,34 +45,42 @@
 
     @property
     def identity(self):
-        return "%s:%s" % (self.repository.shortened_path, self._branch_name)
+        """See `IGitRef`."""
+        return "%s:%s" % (self.repository.shortened_path, self.name)
 
     @property
     def unique_name(self):
-        return "%s:%s" % (self.repository.unique_name, self._branch_name)
+        """See `IGitRef`."""
+        return "%s:%s" % (self.repository.unique_name, self.name)
 
     @property
     def owner(self):
+        """See `IGitRef`."""
         return self.repository.owner
 
     @property
     def target(self):
+        """See `IGitRef`."""
         return self.repository.target
 
     @property
     def subscribers(self):
+        """See `IGitRef`."""
         return self.repository.subscribers
 
     def subscribe(self, person, notification_level, max_diff_lines,
                   code_review_level, subscribed_by):
+        """See `IGitRef`."""
         return self.repository.subscribe(
             person, notification_level, max_diff_lines, code_review_level,
             subscribed_by)
 
     def getSubscription(self, person):
+        """See `IGitRef`."""
         return self.repository.getSubscription(person)
 
     def getNotificationRecipients(self):
+        """See `IGitRef`."""
         return self.repository.getNotificationRecipients()
 
 
@@ -123,6 +132,7 @@
     implements(IGitRef)
 
     def __init__(self, repository, path, commit_sha1):
+        self.repository_id = repository.id
         self.repository = repository
         self.path = path
         self.commit_sha1 = commit_sha1
@@ -130,7 +140,7 @@
     @property
     def _self_in_database(self):
         """Return the equivalent database-backed record of self."""
-        ref = Store.of(GitRef).get(GitRef, (self.repository, self.path))
+        ref = IStore(GitRef).get(GitRef, (self.repository_id, self.path))
         if ref is None:
             raise NotFoundError(
                 "Repository '%s' does not currently contain a reference named "
@@ -141,4 +151,16 @@
         return getattr(self._self_in_database, name)
 
     def __setattr__(self, name, value):
-        return setattr(self._self_in_database, name, value)
+        if name in ("repository_id", "repository", "path", "commit_sha1"):
+            self.__dict__[name] = value
+        else:
+            setattr(self._self_in_database, name, value)
+
+    def __eq__(self, other):
+        return (
+            self.repository == other.repository and
+            self.path == other.path and
+            self.commit_sha1 == other.commit_sha1)
+
+    def __ne__(self, other):
+        return not self == other

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-04-17 00:01:15 +0000
+++ lib/lp/code/model/gitrepository.py	2015-04-21 10:55:54 +0000
@@ -362,10 +362,17 @@
             GitRef.path.startswith(u"refs/heads/")).order_by(GitRef.path)
 
     def getRefByPath(self, path):
-        return Store.of(self).find(
-            GitRef,
-            GitRef.repository_id == self.id,
-            GitRef.path == path).one()
+        paths = [path]
+        if not path.startswith(u"refs/heads/"):
+            paths.append(u"refs/heads/%s" % path)
+        for try_path in paths:
+            ref = Store.of(self).find(
+                GitRef,
+                GitRef.repository_id == self.id,
+                GitRef.path == try_path).one()
+            if ref is not None:
+                return ref
+        return None
 
     @staticmethod
     def _convertRefInfo(info):

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2015-04-17 00:01:15 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2015-04-21 10:55:54 +0000
@@ -537,6 +537,13 @@
                 object_type=GitObjectType.BLOB,
                 ))
 
+    def test_getRefByPath_without_leading_refs_heads(self):
+        [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/master"])
+        self.assertEqual(
+            ref, ref.repository.getRefByPath(u"refs/heads/master"))
+        self.assertEqual(ref, ref.repository.getRefByPath(u"master"))
+        self.assertIsNone(ref.repository.getRefByPath(u"other"))
+
     def test_planRefChanges(self):
         # planRefChanges copes with planning changes to refs in a repository
         # where some refs have been created, some deleted, and some changed.