launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:py3-git-ref-comparison into launchpad:master.

Commit message:
Tighten up IGitRef __eq__, __ne__, and __hash__

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/397918

As part of Python 3 support, we need to implement proper __eq__, __ne__, and __hash__ methods for StormBase.  Before we can do that, we first need to tighten up the custom implementations for GitRef and GitRefRemote, which need their own specialized rules.

The main functional changes from this commit alone are that GitRefDefault and GitRefFrozen instances can be compared with GitRef instances and hashed, and that GitRefRemote instances now have correct hashes derived only from their repository URL and path.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:py3-git-ref-comparison into launchpad:master.

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 29990e3..bf4b81c 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 #
 # Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
@@ -2137,6 +2137,7 @@ public.distribution                     = SELECT
 public.distroseries                     = SELECT
 public.emailaddress                     = SELECT
 public.gitjob                           = SELECT
+public.gitref                           = SELECT
 public.gitrepository                    = SELECT
 public.gitsubscription                  = SELECT
 public.job                              = SELECT, INSERT, UPDATE
diff --git a/lib/lp/code/interfaces/gitref.py b/lib/lp/code/interfaces/gitref.py
index ef51d99..222bf8e 100644
--- a/lib/lp/code/interfaces/gitref.py
+++ b/lib/lp/code/interfaces/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Git reference ("ref") interfaces."""
@@ -8,6 +8,7 @@ __metaclass__ = type
 __all__ = [
     'IGitRef',
     'IGitRefBatchNavigator',
+    'IGitRefRemote',
     'IGitRefRemoteSet',
     'IGitRefSet',
     ]
@@ -468,6 +469,14 @@ class IGitRef(IGitRefView, IGitRefEdit):
     """A reference in a Git repository."""
 
 
+class IGitRefRemote(Interface):
+    """Marker interface for a reference in a remote Git repository."""
+    # XXX cjwatson 2020-12-16: Implementers of this don't implement IGitRef
+    # correctly: several required properties will raise NotImplementedError.
+    # We may need to split out the parts of IGitRef that don't apply to
+    # remote references, and make this a full (non-marker) interface.
+
+
 class IGitRefSet(Interface):
     def findByReposAndPaths(repos_and_paths):
         """Returns the collection of GitRefs for the given list of tuples
diff --git a/lib/lp/code/model/gitref.py b/lib/lp/code/model/gitref.py
index 14ef25b..efaf6ac 100644
--- a/lib/lp/code/model/gitref.py
+++ b/lib/lp/code/model/gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -65,6 +65,7 @@ from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitlookup import IGitLookup
 from lp.code.interfaces.gitref import (
     IGitRef,
+    IGitRefRemote,
     IGitRefRemoteSet,
     IGitRefSet,
     )
@@ -103,6 +104,22 @@ class GitRefMixin:
 
     repository_url = None
 
+    def __eq__(self, other):
+        return (
+            IGitRef.providedBy(other) and
+            not IGitRefRemote.providedBy(other) and
+            self.repository == other.repository and
+            self.repository_url == other.repository_url and
+            self.path == other.path and
+            self.commit_sha1 == other.commit_sha1)
+
+    def __ne__(self, other):
+        return not self == other
+
+    def __hash__(self):
+        return hash((
+            self.repository, self.repository_url, self.path, self.commit_sha1))
+
     @property
     def display_name(self):
         """See `IGitRef`."""
@@ -488,7 +505,7 @@ class GitRefMixin:
 
 @implementer(IGitRef)
 @provider(IGitRefSet)
-class GitRef(StormBase, GitRefMixin):
+class GitRef(GitRefMixin, StormBase):
     """See `IGitRef`."""
 
     __storm_table__ = 'GitRef'
@@ -676,18 +693,6 @@ class GitRefDatabaseBackedMixin(GitRefMixin):
         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
-
-    def __hash__(self):
-        return hash(self.repository) ^ hash(self.path) ^ hash(self.commit_sha1)
-
     # zope.interface tries to use this during adaptation (e.g. to
     # ITraversable), and we don't want that to attempt a database lookup via
     # __getattr__.
@@ -801,7 +806,7 @@ def _fetch_blob_from_launchpad(repository_url, ref_path, filename):
     return response.content
 
 
-@implementer(IGitRef)
+@implementer(IGitRef, IGitRefRemote)
 @provider(IGitRefRemoteSet)
 class GitRefRemote(GitRefMixin):
     """A reference in a remotely-hosted Git repository.
@@ -820,6 +825,18 @@ class GitRefRemote(GitRefMixin):
         """See `IGitRefRemoteSet`."""
         return cls(repository_url, path)
 
+    def __eq__(self, other):
+        return (
+            IGitRefRemote.providedBy(other) and
+            self.repository_url == other.repository_url and
+            self.path == other.path)
+
+    def __ne__(self, other):
+        return not self == other
+
+    def __hash__(self):
+        return hash((self.repository_url, self.path))
+
     def _unimplemented(self, *args, **kwargs):
         raise NotImplementedError("Not implemented for remote repositories.")
 
@@ -935,12 +952,3 @@ class GitRefRemote(GitRefMixin):
 
     setGrants = _unimplemented
     checkPermissions = _unimplemented
-
-    def __eq__(self, other):
-        return (
-            other is not None and
-            self.repository_url == other.repository_url and
-            self.path == other.path)
-
-    def __ne__(self, other):
-        return not self == other
diff --git a/lib/lp/code/model/tests/test_gitref.py b/lib/lp/code/model/tests/test_gitref.py
index b7140ce..1cef9df 100644
--- a/lib/lp/code/model/tests/test_gitref.py
+++ b/lib/lp/code/model/tests/test_gitref.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git references."""
@@ -73,6 +73,32 @@ class TestGitRef(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
+    def test_comparison(self):
+        refs = self.factory.makeGitRefs(
+            paths=["refs/heads/master", "refs/heads/next"])
+        refs.extend(self.factory.makeGitRefs())
+        dup_refs = [ref.repository.getRefByPath(ref.path) for ref in refs]
+        remote_refs = [self.factory.makeGitRefRemote() for _ in range(2)]
+        refs.extend(remote_refs)
+        dup_refs.extend([
+            self.factory.makeGitRefRemote(
+                repository_url=ref.repository_url, path=ref.path)
+            for ref in remote_refs])
+        for i, ref in enumerate(refs):
+            for j, dup_ref in enumerate(dup_refs):
+                if i == j:
+                    self.assertEqual(ref, dup_ref)
+                    self.assertEqual(dup_ref, ref)
+                else:
+                    self.assertNotEqual(ref, dup_ref)
+                    self.assertNotEqual(dup_ref, ref)
+        ref_set = set(refs) | set(dup_refs)
+        self.assertEqual(5, len(ref_set))
+        for ref in refs:
+            self.assertIn(ref, ref_set)
+        for dup_ref in dup_refs:
+            self.assertIn(dup_ref, ref_set)
+
     def test_display_name(self):
         [master, personal] = self.factory.makeGitRefs(
             paths=["refs/heads/master", "refs/heads/people/foo/bar"])