launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/allow-dirty-patches into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1007131 in Launchpad itself: "MalformedHunkHeader: Malformed hunk header.  Does not match format. '<!--\r"
  https://bugs.launchpad.net/launchpad/+bug/1007131

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/allow-dirty-patches/+merge/137117

Allow dirty patches while we generate diffstat output by parsing the patches. This means that things like MalformedHunkHeader and MalformedLine will no longer be raised (and cause OOPSes), but the patch will instead not be counted. I considered this an acceptable tradeoff, and it means that merge proposals that used to get no diffstat counts do now get counts, which could be off a little.

Things like MalformedPatchHeader and more serious exceptions are still raised and will still cause OOPSes, but they could indicate a problem with the code that generates the diff. There is an existing test that tosses a "not a real diff" string into the machinery, and I've verified that this change does not break it.
-- 
https://code.launchpad.net/~stevenk/launchpad/allow-dirty-patches/+merge/137117
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/allow-dirty-patches into lp:launchpad.

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2012-09-19 01:19:35 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2012-11-30 05:34:22 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212,F0401
-
 """Database class for branch merge prosals."""
 
 __metaclass__ = type

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2012-09-28 06:25:44 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2012-11-30 05:34:22 +0000
@@ -1,17 +1,14 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-
 """Job classes related to BranchMergeProposals are in here.
 
 This includes both jobs for the proposals themselves, or jobs that are
 creating proposals, or diffs relating to the proposals.
 """
 
-
 __metaclass__ = type
 
-
 __all__ = [
     'BranchMergeProposalJob',
     'BranchMergeProposalJobSource',
@@ -376,8 +373,7 @@
         with server(get_ro_server(), no_replace=True):
             preview = PreviewDiff.fromBranchMergeProposal(
                 self.branch_merge_proposal)
-        with BranchMergeProposalDelta.monitor(
-            self.branch_merge_proposal):
+        with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
             self.branch_merge_proposal.preview_diff = preview
 
     def getOperationDescription(self):

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2012-10-02 01:25:48 +0000
+++ lib/lp/code/model/diff.py	2012-11-30 05:34:22 +0000
@@ -231,11 +231,6 @@
             diff_content.seek(0)
             diff_content_bytes = diff_content.read(size)
             diff_lines_count = len(diff_content_bytes.strip().split('\n'))
-        # Generation of diffstat is currently failing in some circumstances.
-        # See bug 436325.  Since diffstats are incidental to the whole
-        # process, we don't want failure here to kill the generation of the
-        # diff itself, but we do want to hear about it.  So log an error using
-        # the error reporting utility.
         try:
             diffstat = cls.generateDiffstat(diff_content_bytes)
         except Exception:
@@ -262,7 +257,9 @@
         :return: A map of {filename: (added_line_count, removed_line_count)}
         """
         file_stats = {}
-        for patch in parse_patches(diff_bytes.splitlines(True)):
+        # Set allow_dirty, so we don't raise exceptions for dirty patches.
+        patches = parse_patches(diff_bytes.splitlines(True), allow_dirty=True)
+        for patch in patches:
             if not isinstance(patch, Patch):
                 continue
             path = patch.newname.split('\t')[0]

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_diff.py	2012-11-30 05:34:22 +0000
@@ -259,10 +259,18 @@
             {'foo': (2, 1), 'bar': (0, 3), 'baz': (2, 0)},
             Diff.generateDiffstat(self.diff_bytes))
 
+    def test_generateDiffstat_with_CR(self):
+        diff_bytes = (
+            "--- foo\t2009-08-26 15:53:23.000000000 -0400\n"
+            "+++ foo\t2009-08-26 15:56:43.000000000 -0400\n"
+            "@@ -1,1 +1,1 @@\n"
+            " a\r-b\r c\r+d\r+e\r+f\r")
+        self.assertEqual({'foo': (0, 0)}, Diff.generateDiffstat(diff_bytes))
+
     def test_fromFileSetsDiffstat(self):
         diff = Diff.fromFile(StringIO(self.diff_bytes), len(self.diff_bytes))
-        self.assertEqual({'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)},
-                         diff.diffstat)
+        self.assertEqual(
+            {'bar': (0, 3), 'baz': (2, 0), 'foo': (2, 1)}, diff.diffstat)
 
     def test_fromFileAcceptsBinary(self):
         diff_bytes = "Binary files a\t and b\t differ\n"