Merge ~pappacena/launchpad:refactoring-gitrepo-getRefByPath into launchpad:master

[Thiago F. Pappacena]

Refactoring GitRepository.getRefByPath method to do less database queries.

[Colin Watson (cjwatson)]

Given that we only want the results for the first matching path, it would be possible to be more precise by doing a query that compiles down to something along the lines of this (for id == 10, path == 'master'):

  SELECT GitRef.*
  FROM GitRef, (VALUES (0, 'master'), (1, 'refs/heads/master')) AS lookup (index, path)
  WHERE GitRef.repository = 10 AND GitRef.path = lookup.path
  ORDER BY lookup.index
  LIMIT 1;

But given that there are always either one or two elements in path, it seems unlikely to be worth very much bother to fine-tune! So feel free to go ahead with your version if you want. I mention it mainly because this sort of approach can be handy in larger cases.

review: Approve

[Thiago F. Pappacena (pappacena)]

I like this approach too, cjwatson. I will consider it for future similar optimizations.

diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index d88a384..416a4f3 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -596,11 +596,13 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         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()
+        refs = Store.of(self).find(
+            GitRef,
+            GitRef.repository_id == self.id,
+            GitRef.path.is_in(paths))
+        refs_by_path = {r.path: r for r in refs}
+        for path in paths:
+            ref = refs_by_path.get(path)
             if ref is not None:
                 return ref
         return None
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 8a9bcc8..165980b 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -170,6 +170,7 @@ from lp.testing import (
     logout,
     person_logged_in,
     record_two_runs,
+    StormStatementRecorder,
     TestCaseWithFactory,
     verifyObject,
     )
@@ -3918,6 +3919,17 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
         self.assertEqual(200, response.status)
         self.assertIsNone(response.jsonBody())
 
+    def test_getRefByPath_query_count(self):
+        repository_db = self.factory.makeGitRepository()
+        ref_dbs = self.factory.makeGitRefs(
+            repository=repository_db, paths=[
+                "refs/heads/devel", "refs/heads/master"])
+
+        with StormStatementRecorder() as recorder:
+            ref = repository_db.getRefByPath("master")
+            self.assertEqual(ref_dbs[1], ref)
+            self.assertEqual(1, recorder.count)
+
     def test_subscribe(self):
         # A user can subscribe to a repository.
         repository_db = self.factory.makeGitRepository()