launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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

Commit message:
Exclude refs starting with refs/changes/ from Git ref scans.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-ref-scan-exclude-prefixes/+merge/359208

This should help with repositories imported from Gerrit with very large numbers of refs/changes/ refs.

https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/359204 must be deployed to production first.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-ref-scan-exclude-prefixes into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/githosting.py'
--- lib/lp/code/interfaces/githosting.py	2016-05-14 09:54:32 +0000
+++ lib/lp/code/interfaces/githosting.py	2018-11-22 16:50:19 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for communication with the Git hosting service."""
@@ -37,10 +37,11 @@
         :param props: Properties to set.
         """
 
-    def getRefs(path):
+    def getRefs(path, exclude_prefixes=None):
         """Get all refs in this repository.
 
         :param path: Physical path of the repository on the hosting service.
+        :param exclude_prefixes: An optional list of ref prefixes to exclude.
         :return: A dict mapping ref paths to dicts representing the objects
             they point to.
         """

=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py	2018-07-30 18:08:34 +0000
+++ lib/lp/code/model/githosting.py	2018-11-22 16:50:19 +0000
@@ -116,10 +116,12 @@
             raise GitRepositoryScanFault(
                 "Failed to set properties of Git repository: %s" % unicode(e))
 
-    def getRefs(self, path):
+    def getRefs(self, path, exclude_prefixes=None):
         """See `IGitHostingClient`."""
         try:
-            return self._get("/repo/%s/refs" % path)
+            return self._get(
+                "/repo/%s/refs" % path,
+                params={"exclude_prefix": exclude_prefixes})
         except requests.RequestException as e:
             raise GitRepositoryScanFault(
                 "Failed to get refs from Git repository: %s" % unicode(e))

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2018-11-22 07:28:33 +0000
+++ lib/lp/code/model/gitrepository.py	2018-11-22 16:50:19 +0000
@@ -672,7 +672,9 @@
         """See `IGitRepository`."""
         hosting_client = getUtility(IGitHostingClient)
         new_refs = {}
-        for path, info in hosting_client.getRefs(hosting_path).items():
+        exclude_prefixes = config.codehosting.git_exclude_ref_prefixes.split()
+        for path, info in hosting_client.getRefs(
+                hosting_path, exclude_prefixes=exclude_prefixes).items():
             try:
                 new_refs[path] = self._convertRefInfo(info)
             except ValueError as e:

=== modified file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py	2018-05-31 10:23:03 +0000
+++ lib/lp/code/model/tests/test_githosting.py	2018-11-22 16:50:19 +0000
@@ -145,6 +145,16 @@
         self.assertEqual({"refs/heads/master": {}}, refs)
         self.assertRequest("repo/123/refs", method="GET")
 
+    def test_getRefs_exclude_prefixes(self):
+        with self.mockRequests("GET", json={"refs/heads/master": {}}):
+            refs = self.client.getRefs(
+                "123", exclude_prefixes=["refs/changes/", "refs/pull/"])
+        self.assertEqual({"refs/heads/master": {}}, refs)
+        self.assertRequest(
+            "repo/123/refs"
+            "?exclude_prefix=refs%2Fchanges%2F&exclude_prefix=refs%2Fpull%2F",
+            method="GET")
+
     def test_getRefs_failure(self):
         with self.mockRequests("GET", status=400):
             self.assertRaisesWithContent(

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2018-11-21 23:59:18 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2018-11-22 16:50:19 +0000
@@ -1488,6 +1488,23 @@
         self.assertEqual(expected_upsert, refs_to_upsert)
         self.assertEqual(set(), refs_to_remove)
 
+    def test_planRefChanges_excludes_configured_prefixes(self):
+        # planRefChanges excludes some ref prefixes by default, and can be
+        # configured otherwise.
+        repository = self.factory.makeGitRepository()
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        repository.planRefChanges("dummy")
+        self.assertEqual(
+            [{"exclude_prefixes": ["refs/changes/"]}],
+            hosting_fixture.getRefs.extract_kwargs())
+        hosting_fixture.getRefs.calls = []
+        self.pushConfig(
+            "codehosting", git_exclude_ref_prefixes="refs/changes/ refs/pull/")
+        repository.planRefChanges("dummy")
+        self.assertEqual(
+            [{"exclude_prefixes": ["refs/changes/", "refs/pull/"]}],
+            hosting_fixture.getRefs.extract_kwargs())
+
     def test_fetchRefCommits(self):
         # fetchRefCommits fetches detailed tip commit metadata for the
         # requested refs.

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2018-07-30 18:09:23 +0000
+++ lib/lp/services/config/schema-lazr.conf	2018-11-22 16:50:19 +0000
@@ -376,6 +376,9 @@
 # datatype: urlbase
 git_ssh_root: none
 
+# A space-separated list of Git ref prefixes to exclude from scans.
+git_exclude_ref_prefixes: refs/changes/
+
 # The upper limit on the number of bugs to link to a merge proposal based on
 # Git commit metadata.
 related_bugs_from_source_limit: 1000