launchpad-reviewers team mailing list archive

[Ines Almeida <mp+491274@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:add-githosting-get-diff-stats-endpoint into launchpad:master.

Commit message:
Add getDiffStats endpoint to GitHostingClient

This endpoint calls the new turnip endpoint for getting stats (particularly file stats) for a diff

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/491274
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:add-githosting-get-diff-stats-endpoint into launchpad:master.

diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index d54cbb4..2f907bd 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -105,6 +105,20 @@ class IGitHostingClient(Interface):
             to a list of conflicted paths.
         """
 
+    def getDiffStats(path, old, new, common_ancestor=False, logger=None):
+        """Get the diff between two commits.
+
+        :param path: Physical path of the repository on the hosting service.
+        :param old: The OID of the old commit. Can be empty to get stat
+            against empty tree diff
+        :param new: The OID of the new commit.
+        :param common_ancestor: If True, return the symmetric or common
+            ancestor diff, equivalent to
+            `git diff $(git-merge-base OLD NEW) NEW`.
+        :param logger: An optional logger.
+        :return: A dict with diff stats for the existing diff.
+        """
+
     def detectMerges(path, target, sources, previous_target=None, logger=None):
         """Detect merges of any of 'sources' into 'target'.
 
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index c0dad3d..29136ef 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -243,6 +243,42 @@ class GitHostingClient:
                 "Failed to get diff from Git repository: %s" % str(e)
             )
 
+    def getDiffStats(
+        self,
+        path,
+        old,
+        new,
+        common_ancestor=False,
+        logger=None,
+    ):
+        """See `IGitHostingClient`."""
+        try:
+            # Diff with empty tree case
+            if old is None:
+                old = ""
+
+            # New commit cannot be None
+            if new == "" or new is None:
+                raise GitRepositoryScanFault("'new' commit cannot be None")
+
+            if logger is not None:
+                logger.info(
+                    "Requesting diff for %s from %s to %s" % (path, old, new)
+                )
+
+            separator = "..." if common_ancestor else ".."
+            url = "/repo/%s/compare/%s%s%s/stats" % (
+                path,
+                quote(old),
+                separator,
+                quote(new),
+            )
+            return self._get(url)
+        except requests.RequestException as e:
+            raise GitRepositoryScanFault(
+                "Failed to get diff from Git repository: %s" % str(e)
+            )
+
     def getMergeDiff(self, path, base, head, prerequisite=None, logger=None):
         """See `IGitHostingClient`."""
         try:
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index b3ccf72..38dae61 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -396,6 +396,48 @@ class TestGitHostingClient(TestCase):
                 "b",
             )
 
+    def test_getDiffStats(self):
+        with self.mockRequests("GET", json={"files": {}}):
+            stats = self.client.getDiffStats("123", "a", "b")
+        self.assertEqual({"files": {}}, stats)
+        self.assertRequest("repo/123/compare/a..b/stats", method="GET")
+
+    def test_getDiffStats_common_ancestor(self):
+        with self.mockRequests("GET", json={"files": {}}):
+            stats = self.client.getDiffStats(
+                "123", "a", "b", common_ancestor=True
+            )
+        self.assertEqual({"files": {}}, stats)
+        self.assertRequest("repo/123/compare/a...b/stats", method="GET")
+
+    def test_getDiffStats_empty_tree(self):
+        with self.mockRequests("GET", json={"files": {}}):
+            stats = self.client.getDiffStats("123", None, "b")
+        self.assertEqual({"files": {}}, stats)
+        self.assertRequest("repo/123/compare/..b/stats", method="GET")
+
+    def test_getDiffStats_no_new_commit(self):
+        self.assertRaisesWithContent(
+            GitRepositoryScanFault,
+            "'new' commit cannot be None",
+            self.client.getDiffStats,
+            "123",
+            "a",
+            None,
+        )
+
+    def test_getDiffStats_failure(self):
+        with self.mockRequests("GET", status=400):
+            self.assertRaisesWithContent(
+                GitRepositoryScanFault,
+                "Failed to get diff from Git repository: "
+                "400 Client Error: Bad Request",
+                self.client.getDiffStats,
+                "123",
+                "a",
+                "b",
+            )
+
     def test_getMergeDiff(self):
         with self.mockRequests("GET", json={"patch": ""}):
             diff = self.client.getMergeDiff("123", "a", "b")
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 45d904e..5989007 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -353,6 +353,7 @@ class GitHostingFixture(fixtures.Fixture):
         commits=None,
         log=None,
         diff=None,
+        diff_stats=None,
         merge_diff=None,
         merges=None,
         blob=None,
@@ -380,6 +381,9 @@ class GitHostingFixture(fixtures.Fixture):
         self.getMergeDiff = fake_method_factory(
             result={} if merge_diff is None else merge_diff
         )
+        self.getDiffStats = fake_method_factory(
+            result=({} if diff_stats is None else diff_stats)
+        )
         self.detectMerges = fake_method_factory(
             result=({} if merges is None else merges)
         )