Merge lp:~gz/bzr-builddeb/merge_changelog_invalid_version_893495 into lp:bzr-builddeb

[Martin Packman]

Handle dpkg-mergechangelogs aborting by falling back to other merge hooks rather than thinking it's a successful merge resulting in a zero byte file. This can be hit with invalid versions in one of the changelogs, and newer dpkg versions have become stricter on this front.

Also resolved some bad encoding handling while there, as it was part of the original bug report and gets in the way of one of the neater way of testing (non-ascii versions are rejected in my older dpkg version as well).

The test is written in such a way as it should keep passing if my dpkg patch is accepted and the merge starts succeeding it will continue to pass, regardless of the version installed. That's the plan, at least.

[Jelmer Vernooij (jelmer)]

Thanks, that's a reasonable way of handling it.

+ Depending on the dpkg version, bad version may be tolerated or not.
"a bad version" / "bad versions" ?

review: Approve (code)

=== modified file 'merge_changelog.py'
--- merge_changelog.py	2011-10-25 10:55:57 +0000
+++ merge_changelog.py	2011-12-13 23:30:29 +0000
@@ -26,6 +26,7 @@
 
 from bzrlib import (
     merge,
+    osutils,
     )
 
 
@@ -77,7 +78,9 @@
             # don't decorate the messages at all, as dpkg-mergechangelogs
             # warnings are already prefixed with "dpkg-mergechangelogs:
             # warning:" which makes the origin of the messages quite clear.
-            _logger.warning('%s', stderr)
+            encoding = osutils.get_user_encoding()
+            # Errors are output using the locale, and log needs unicode.
+            _logger.warning('%s', stderr.decode(encoding, "replace"))
         if retcode == 1:
             # dpkg-mergechangelogs reports a conflict.  Unfortunately it uses
             # slightly non-standard conflict markers (<http://pad.lv/815700>:
@@ -96,6 +99,12 @@
                 return match_text[0] * 7
             stdout = re.sub('(?m)^[<=>]{6}$', replace_func, stdout)
             return 'conflicted', stdout
+        elif retcode != 0:
+            # dpkg-mergechangelogs exited with an error. There is probably no
+            # output at all, but regardless the merge should fall back to
+            # another method.
+            _logger.warning("dpkg-mergechangelogs failed with status %d", retcode)
+            return 'not_applicable', stdout
         else:
             return 'success', stdout
     finally:

=== modified file 'tests/test_merge_changelog.py'
--- tests/test_merge_changelog.py	2011-10-14 14:15:43 +0000
+++ tests/test_merge_changelog.py	2011-12-13 23:30:29 +0000
@@ -134,12 +134,6 @@
 # Backports from current testtools so that we remain compatible with testtools
 # 0.9.2 (the version in lucid).
 UTF8_TEXT = ContentType('text', 'plain', {'charset': 'utf8'})
-def text_content(text):
-    """Create a `Content` object from some text.
-
-    This is useful for adding details which are short strings.
-    """
-    return Content(UTF8_TEXT, lambda: [text.encode('utf8')])
 
 
 class TestMergeChangelog(tests.TestCase):
@@ -162,7 +156,8 @@
         warnings_log = self.logged_warnings.getvalue()
         if warnings_log:
             self.addDetail(
-                'merge_changelog warnings', text_content(warnings_log))
+                'merge_changelog warnings',
+                Content(UTF8_TEXT, lambda: [warnings_log]))
 
     def assertMergeChangelog(self, expected_lines, this_lines, other_lines,
                              base_lines=[], conflicted=False):
@@ -265,6 +260,40 @@
                                   conflicted=True
                                   )
 
+    def test_invalid_version(self):
+        """An invalid version in the changelog should be handled sanely
+        
+        Depending on the dpkg version, bad version may be tolerated or not.
+        If the script aborts the merge should result in a success with zero
+        bytes in the merged file. See lp:893495 for such an issue.
+        """
+        invalid_changelog = """\
+psuedo-prog (\xc2\xa7) unstable; urgency=low
+
+  * New project released!!!!
+
+ -- Barry Foo <[e-mail]>  Thu, 28 Jan 2010 10:00:44 +0000
+
+""".splitlines(True)
+        # invalid_changelog has a bad version, with non-ascii characters.
+        status, lines = merge_changelog.merge_changelog(
+            this_lines=v_112_1 + invalid_changelog,
+            other_lines=v_111_2 + invalid_changelog,
+            base_lines=invalid_changelog)
+        log = self.logged_warnings.getvalue()
+        self.assertContainsRe(log, "(?m)\xc2\xa7 is not a valid version$")
+        if "dpkg-mergechangelogs: error:" in log:
+            # Script version aborts on invalid versions so merge must fail
+            self.assertEqual('not_applicable', status)
+            self.assertContainsRe(log,
+                "(?m)dpkg-mergechangelogs failed with status \\d+$")
+        else:
+            # Script version tolerates invalid versions and produces output
+            self.assertEqual('success', status)
+            self.assertEqualDiff(
+                "".join(v_112_1 + v_111_2 + invalid_changelog),
+                "".join(lines))
+
 
 class TestChangelogHook(tests.TestCaseWithMemoryTransport):