Merge lp:~abentley/launchpad/diffstat-count into lp:launchpad

[Aaron Bentley]

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

= Summary =
This allows Launchpad to automatically know the overall number of lines
added or removed by a patch.

== Proposed fix ==
Calculate totals using the diffstat array, and store them in the
database. In the event that a diffstat cannot be calculated, store None
in these fields.

== Pre-implementation notes ==
Pre-implementation was with thumper

== Implementation details ==
As a drive-by, if a diffstat cannot be calculated, Diff.diffstat is set
to None rather than {}. None is already its default value.

== Tests ==
bin/test test_diff -t TestDiff

== Demo and Q/A ==
There is no user-visible change.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/model/diff.py
  lib/lp/code/model/tests/test_diff.py
  lib/lp/code/stories/webservice/xx-branchmergeproposal.txt

== Pylint notices ==

lib/lp/code/model/diff.py
    17: [F0401] Unable to import 'lazr.delegates' (No module named
delegates)

^^^ bogus
    166: [W0703, Diff.fromFile] Catch "Exception"
^^^ The block does generic exception handling.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrEt4kACgkQ0F+nu1YWqI0UegCfWdcLy0KKD2rhXIAV0J4IdvO4
71AAn1gxQWugD8zeDYmUVoiTZ03/QTwd
=VU6P
-----END PGP SIGNATURE-----

[Paul Hummer (rockstar)]

review: Approve

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2009-09-25 03:42:50 +0000
+++ lib/lp/code/model/diff.py	2009-10-01 14:15:26 +0000
@@ -50,6 +50,9 @@
                     in simplejson.loads(self._diffstat).items())
 
     def _set_diffstat(self, diffstat):
+        if diffstat is None:
+            self._diffstat = None
+            return
         # diffstats should be mappings of path to line counts.
         assert isinstance(diffstat, dict)
         self._diffstat = simplejson.dumps(diffstat)
@@ -163,9 +166,18 @@
         except Exception:
             getUtility(IErrorReportingUtility).raising(sys.exc_info())
             # Set the diffstat to be empty.
-            diffstat = {}
+            diffstat = None
+            added_lines_count = None
+            removed_lines_count = None
+        else:
+            added_lines_count = 0
+            removed_lines_count = 0
+            for path, (added, removed) in diffstat.items():
+                added_lines_count += added
+                removed_lines_count += removed
         return cls(diff_text=diff_text, diff_lines_count=diff_lines_count,
-                   diffstat=diffstat)
+                   diffstat=diffstat, added_lines_count=added_lines_count,
+                   removed_lines_count=removed_lines_count)
 
     @staticmethod
     def generateDiffstat(diff_bytes):

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2009-09-25 03:42:50 +0000
+++ lib/lp/code/model/tests/test_diff.py	2009-10-01 14:15:26 +0000
@@ -59,7 +59,7 @@
         # There's a conflict because the source branch added a line "d", but
         # the target branch added the line "c" in the same place.
         self.assertIn(
-            '+<<<<<<< TREE\n c\n+=======\n+d\n+>>>>>>> MERGE-SOURCE\n',
+            '+<<<''<<<< TREE\n c\n+=======\n+d\n+>>>>>''>> MERGE-SOURCE\n',
             diff_text)
 
 
@@ -140,6 +140,28 @@
         "+d\n"
         "+e\n")
 
+    diff_bytes_2 = (
+        "--- bar	2009-08-26 15:53:34.000000000 -0400\n"
+        "+++ bar	1969-12-31 19:00:00.000000000 -0500\n"
+        "@@ -1,3 +0,0 @@\n"
+        "-a\n"
+        "-b\n"
+        "-c\n"
+        "--- baz	1969-12-31 19:00:00.000000000 -0500\n"
+        "+++ baz	2009-08-26 15:53:57.000000000 -0400\n"
+        "@@ -0,0 +1,2 @@\n"
+        "+a\n"
+        "+b\n"
+        "--- foo	2009-08-26 15:53:23.000000000 -0400\n"
+        "+++ foo	2009-08-26 15:56:43.000000000 -0400\n"
+        "@@ -1,3 +1,5 @@\n"
+        " a\n"
+        "-b\n"
+        " c\n"
+        "+d\n"
+        "+e\n"
+        "+f\n")
+
     def test_generateDiffstat(self):
         self.assertEqual(
             {'foo': (2, 1), 'bar': (0, 3), 'baz': (2, 0)},
@@ -150,6 +172,13 @@
         self.assertEqual({'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)},
                          diff.diffstat)
 
+    def test_fromFileSets_added_removed(self):
+        """fromFile sets added_lines_count, removed_lines_count."""
+        diff = Diff.fromFile(
+            StringIO(self.diff_bytes_2), len(self.diff_bytes_2))
+        self.assertEqual(5, diff.added_lines_count)
+        self.assertEqual(4, diff.removed_lines_count)
+
     def test_fromFile_withError(self):
         # If the diff is formatted such that generating the diffstat fails, we
         # want to record an oops but continue.
@@ -157,7 +186,9 @@
         diff_bytes = "not a real diff"
         diff = Diff.fromFile(StringIO(diff_bytes), len(diff_bytes))
         self.assertNotEqual(last_oops_id, errorlog.globalErrorUtility.lastid)
-        self.assertEqual({}, diff.diffstat)
+        self.assertIs(None, diff.diffstat)
+        self.assertIs(None, diff.added_lines_count)
+        self.assertIs(None, diff.removed_lines_count)
 
 
 class TestStaticDiff(TestCaseWithFactory):

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2009-09-15 20:45:30 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2009-10-01 14:15:26 +0000
@@ -222,7 +222,7 @@
     >>> preview_diff = webservice.get(
     ...     merge_proposal['preview_diff_link']).jsonBody()
     >>> pprint_entry(preview_diff)
-    added_lines_count: None
+    added_lines_count: 0
     branch_merge_proposal_link:
       u'http://.../~source/fooix/fix-it/+merge/1'
     conflicts: u'oh, no conflicts'
@@ -231,7 +231,7 @@
     diff_text_link:
       u'http://.../~source/fooix/fix-it/+merge/1/+preview-diff/diff_text'
     diffstat: {u'fooix.txt': [0, 0]}
-    removed_lines_count: None
+    removed_lines_count: 0
     resource_type_link: u'http://.../#preview_diff'
     self_link:
       u'http://.../~source/fooix/fix-it/+merge/1/+preview-diff'