Merge lp:~spiv/bzr-builddeb/use-dpkg-mergechangelogs into lp:bzr-builddeb

[Andrew Bennetts]

Use dpkg-mergechangelogs rather than custom logic to merge debian/changelog files.

This switches merge_changelog to simply call out to dpkg-mergechangelogs rather than use its own NIH logic.

As discussed on the bug and UDD list this required updating some tests for the new behaviour, but everyone seems confident that the new behaviour is actually better.

Fixing the tests revealed two very minor drawbacks to dpkg-mergechangelogs:

 1) when merging invalid changelogs it seems to lose the final line of text
 2) the conflict markers aren't quite standard: “<<<<<<” rather than “<<<<<<<” (6 vs. 7 chars).

1) is mitigated greatly by the fact that dpkg-mergechangelogs emits warnings when dealing with invalid changelogs, so developers will be encouraged to check and fix their data anyway. And I imagine invalid changelogs aren't too common, mostly people use tools to help them write them, and the format isn't *that* onerous.

2) is largely cosmetic. I can imagine it might impede using external tools to help deal with conflicts (like meld?) but with some luck those tools are tolerant of the difference, and that drawback is greatly outweighed by the benefit of doing better merges in the first place.

I've updated the README to list installing dpkg-dev as a requirement. For the intended audience of bzr-builddeb that seems pretty reasonable :)

[Jelmer Vernooij (jelmer)]

I think (1) is an acceptable regression given how many other regressions this mp fixes. We should probably file a bug against dpkg-dev about it.

For (2), would it be possible to do postprocessing on the file to fix up the conflict markers to be more standard? It seems like a minor thing though, so perhaps a bug report would also be nice for this.

Either way, I think this is a huge improvement over the previous situation.

[tweak]
Can you add the right entries for these bugs to debian/changelog? The dch command should make this easy ("dch 'Fix the foobar. LP: #...'").

review: Approve (code)

[Andrew Bennetts (spiv)]

Jelmer Vernooij wrote:
> I think (1) is an acceptable regression given how many other regressions this
> mp fixes. We should probably file a bug against dpkg-dev about it.

Yeah, I think so.

> For (2), would it be possible to do postprocessing on the file to fix up the
> conflict markers to be more standard? It seems like a minor thing though, so
> perhaps a bug report would also be nice for this.

Hmm! I guess we could! We know when dpkg-mergechangelogs thinks there's a
conflict, and I doubt those lines could appear in a valid changelog file. I'll
do that!

> [tweak]
> Can you add the right entries for these bugs to debian/changelog? The dch command should make this easy ("dch 'Fix the foobar. LP: #...'").

Ah, I wondered where the news file was, silly me :)

[lp:~spiv/bzr-builddeb/use-dpkg-mergechangelogs]

585. By Andrew Bennetts on 2011-07-25

Workaround bug 815700 in dpkg-mergechangelogs (it emits slightly non-standard conflict markers), and link to bug 815704 in an XXX comment in a test.

586. By Andrew Bennetts on 2011-07-25

Update debian/changelog.

[Andrew Bennetts (spiv)]

It turns out I'm not in the bzr-builddeb-hackers team so can't push directly to lp:bzr-builddeb.

Jelmer, could you push lp:~spiv/bzr-builddeb/trunk-merge-of-use-dpkg-mergechangelogs to lp:bzr-builddeb? (Unless someone else gets there first, of course!) It's just trunk plus a revision merging my changes.

=== modified file 'README'
--- README	2011-01-10 21:59:19 +0000
+++ README	2011-07-25 08:06:14 +0000
@@ -24,6 +24,11 @@
 
 .. _python-debian: http://bzr.debian.org/pkg-python-debian/trunk/
 
+It also requires the ``dpkg-dev`` package to be installed (for the
+``dpkg-mergechangelogs`` tool)::
+
+  apt-get install dpkg-dev
+
 This plugin can be installed in two ways. As you are probably using a Debian
 system you can probably just use the Debian packages. The other way is to 
 branch it in to ``~/.bazaar/plugins/builddeb``, i.e::
 
=== modified file 'debian/changelog'
--- debian/changelog	2011-07-20 19:28:00 +0000
+++ debian/changelog	2011-07-25 08:06:14 +0000
@@ -1,9 +1,18 @@
 bzr-builddeb (2.7.7) UNRELEASED; urgency=low
 
+  [ Jelmer Vernooij ]
   * Build type now defaults to normal mode when used in an empty tree.
     LP: #776528
 
- -- Jelmer Vernooij <[e-mail]>  Wed, 20 Jul 2011 21:27:48 +0200
+  [ Andrew Bennetts ]
+  * Use dpkg-mergechangelogs(1) to merge changelog files.  Fixes LP:
+    #718944 (preserve the existing ordering of changelog entries when
+    merging); LP: #552950 (Additional changelog entries are modified
+    when merging another branch); LP: #517093 (3-way changelog file
+    merge doesn't do textual merging on sections); and LP: #517090 (3-
+    way changelog file merge doesn't support deletions).
+
+ -- Andrew Bennetts <[e-mail]>  Mon, 25 Jul 2011 16:38:09 +1000
 
 bzr-builddeb (2.7.6) unstable; urgency=low
 
 
=== modified file 'merge_changelog.py'
--- merge_changelog.py	2010-05-28 20:47:13 +0000
+++ merge_changelog.py	2011-07-25 08:06:14 +0000
@@ -16,13 +16,24 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import logging
+import os.path
 import re
+import shutil
+import subprocess
+import tempfile
 
 from bzrlib import (
     merge,
     )
 
 
+# A logger in the 'bzr' hierarchy.  By default messages will be propagated to
+# the standard bzr logger, but tests can easily intercept just this logger if
+# they wish.
+_logger = logging.getLogger('bzr.plugins.builddeb.merge_changelog')
+
+
 class ChangeLogFileMerge(merge.ConfigurableFileMerger):
 
     name_prefix = 'deb_changelog'
@@ -30,123 +41,56 @@
 
     def merge_text(self, params):
         return merge_changelog(params.this_lines, params.other_lines,
-                               params.base_lines)
-
-
-# Regular expression for top of debian/changelog
-CL_RE = re.compile(r'^(\w[-+0-9a-z.]*) \(([^\(\) \t]+)\)((\s+[-0-9a-z]+)+)\;',
-                   re.IGNORECASE)
+            params.base_lines)
+
 
 def merge_changelog(this_lines, other_lines, base_lines=[]):
     """Merge a changelog file."""
-    try:
-       from debian import changelog
-    except ImportError:
-        # Prior to 0.1.15 the debian module was called debian_bundle
-        from debian_bundle import changelog
-
-    try:
-        left_cl = read_changelog(this_lines)
-        right_cl = read_changelog(other_lines)
-        # BASE lines don't end up in the output, so we allow strict=False
-        base_cl = read_changelog(base_lines, strict=False)
-    except changelog.ChangelogParseError:
-        return ('not_applicable', None)
-
-    content = []
-    def step(iterator):
-        try:
-            return iterator.next()
-        except StopIteration:
-            return None
-    left_blocks = dict((b.version, b) for b in left_cl._blocks)
-    right_blocks = dict((b.version, b) for b in right_cl._blocks)
-    # Unfortunately, while version objects implement __eq__ they *don't*
-    # implement __hash__, which means we can't do dict lookups properly, so
-    # instead, we fall back on the version string instead of the object.
-    # Make sure never to try to use right_version in left_blocks because of
-    # this.
-    # We lazily parse the base data, in case we never need it
-    base_blocks = dict((b.version.full_version, b) for b in base_cl._blocks)
-    left_order = iter(sorted(left_blocks.keys(), reverse=True))
-    right_order = iter(sorted(right_blocks.keys(), reverse=True))
-    left_version = step(left_order)
-    right_version = step(right_order)
-
-    # TODO: Do we want to support the ability to delete a section? We could do
-    #       a first-pass algorithm that checks the versions in base versus the
-    #       versions in this and other, to determine what versions should be in
-    #       the output. For now, we just assume that if a version is present in
-    #       any of this or other, then we want it in the output.
-    conflict_status = 'success'
-
-    while left_version is not None or right_version is not None:
-        if (left_version is None or
-            (right_version is not None and right_version > left_version)):
-            next_content = str(right_blocks[right_version])
-            right_version = step(right_order)
-        elif (right_version is None or
-            (left_version is not None and left_version > right_version)):
-            next_content = str(left_blocks[left_version])
-            left_version = step(left_order)
+    # Write the BASE, THIS and OTHER versions to files in a temporary
+    # directory, and use dpkg-mergechangelogs to merge them.
+    tmpdir = tempfile.mkdtemp('deb_changelog_merge')
+    try:
+        def writelines(filename, lines):
+            with open(filename, 'w') as f:
+                for line in lines:
+                    f.write(line)
+        base_filename = os.path.join(tmpdir, 'changelog.base')
+        this_filename = os.path.join(tmpdir, 'changelog.this')
+        other_filename = os.path.join(tmpdir, 'changelog.other')
+        writelines(base_filename, base_lines)
+        writelines(this_filename, this_lines)
+        writelines(other_filename, other_lines)
+        proc = subprocess.Popen(['dpkg-mergechangelogs', base_filename,
+            this_filename, other_filename], stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE)
+        stdout, stderr = proc.communicate()
+        retcode = proc.returncode
+        if stderr:
+            # Relay the warning from dpkg-mergechangelogs to the user.  We
+            # 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)
+        if retcode == 1:
+            # dpkg-mergechangelogs reports a conflict.  Unfortunately it uses
+            # slightly non-standard conflict markers (<http://pad.lv/815700>:
+            # "<<<<<<" rather than "<<<<<<<", i.e. 6 chars instead of 7), so we
+            # correct that here to make the results of this plugin as
+            # consistent with regular bzr usage as possible.  Note that
+            # conflict markers are never valid lines in a changelog file, so
+            # it's reasonable for us to assume that any line that looks like a
+            # conflict marker is a conflict marker (rather than valid content).
+            # At worst a conflicted merge of an invalid changelog file that
+            # already contained a non-standard conflict marker will have that
+            # conflict marker made standard, which is more like a feature than
+            # a bug!
+            def replace_func(match_obj):
+                match_text = match_obj.group(0)
+                return match_text[0] * 7
+            stdout = re.sub(
+                '^[<=>]{6}$', replace_func, stdout, flags=re.MULTILINE)
+            return 'conflicted', stdout
         else:
-            assert left_version == right_version
-            # Same version, step both
-            # TODO: Conflict if left_version != right
-            # Note: See above comment why we can't use
-            #       right_blocks[left_version] even though they *should* be
-            #       equivalent
-            left_content = str(left_blocks[left_version])
-            right_content = str(right_blocks[right_version])
-            if left_content == right_content:
-                # Identical content
-                next_content = left_content
-            else:
-                # Sides disagree, compare with base
-                base_content = str(base_blocks.get(left_version.full_version,
-                                                   ''))
-                if left_content == base_content:
-                    next_content = right_content
-                elif right_content == base_content:
-                    next_content = left_content
-                else:
-                    # TODO: We could use merge3.Merge3 to try a line-based
-                    #       textual merge on the content. However, for now I'm
-                    #       just going to conflict on the whole region
-                    # Conflict names taken from merge.py
-                    next_content = ('<<<<<<< TREE\n'
-                                    + left_content
-                                    + '=======\n'
-                                    + right_content
-                                    + '>>>>>>> MERGE-SOURCE\n'
-                                   )
-                    conflict_status = 'conflicted'
-            next_block = left_blocks[left_version]
-            left_version = step(left_order)
-            right_version = step(right_order)
-        content.append(next_content)
-
-    return conflict_status, content
-
-
-def read_changelog(lines, strict=True):
-    """Return a parsed changelog file."""
-    try:
-        from debian import changelog
-    except ImportError:
-        # Prior to 0.1.15 the debian module was called debian_bundle
-        from debian_bundle import changelog
-    # Note: There appears to be a bug in Changelog if you pass it an iterable
-    #       of lines (like a file obj, or a list of lines). Specifically, it
-    #       does not strip trailing newlines, and it adds ones back in, so you
-    #       get doubled blank lines... :(
-    #       So we just ''.join() the lines and don't worry about it
-    # Note: There is also a bug that the Changelog constructor suppresses parse
-    #       errors, so we want to always call parse_changelog separately
-    content = ''.join(lines)
-    cl = changelog.Changelog()
-    if content:
-        # We get a warning if we try to parse an empty changelog file, which in
-        # strict mode is an error, so only parse when we have content
-        cl.parse_changelog(content, strict=strict)
-    return cl
+            return 'success', stdout
+    finally:
+        shutil.rmtree(tmpdir)
 
=== modified file 'tests/test_merge_changelog.py'
--- tests/test_merge_changelog.py	2010-05-23 15:29:38 +0000
+++ tests/test_merge_changelog.py	2011-07-25 08:06:14 +0000
@@ -19,7 +19,7 @@
 
 """Tests for the merge_changelog code."""
 
-import warnings
+import logging
 
 try:
     from debian import changelog
@@ -27,6 +27,9 @@
     # Prior to 0.1.15 the debian module was called debian_bundle
     from debian_bundle import changelog
 
+from testtools.content_type import ContentType
+from testtools.content import Content
+
 from bzrlib import (
     memorytree,
     merge,
@@ -70,6 +73,38 @@
 """.splitlines(True)
 
 
+# Merge of 2b and 2c using 2 as the base (b adds a line, c adds a line and
+# deletes a line).
+v_111_2bc = """\
+psuedo-prog (1.1.1-2) unstable; urgency=low
+
+  * New upstream release.
+  * Yet another content for 1.1.1-2
+  * But more is better
+
+ -- Joe Foo <[e-mail]>  Thu, 28 Jan 2010 10:45:44 +0000
+
+""".splitlines(True)
+
+
+# Merge of 2b and 2c using an empty base. (As calculated by
+# dpkg-mergechangelogs.)
+v_111_2bc_empty_base = """\
+psuedo-prog (1.1.1-2) unstable; urgency=low
+
+  * New upstream release.
+<<<<<<<
+  * Awesome bug fixes.
+=======
+  * Yet another content for 1.1.1-2
+>>>>>>>
+  * But more is better
+
+ -- Joe Foo <[e-mail]>  Thu, 28 Jan 2010 10:45:44 +0000
+
+""".splitlines(True)
+
+
 v_112_1 = """\
 psuedo-prog (1.1.2-1) unstable; urgency=low
 
@@ -92,15 +127,37 @@
 """.splitlines(True)
 
 
-class TestReadChangelog(tests.TestCase):
+# 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.
 
-    def test_read_changelog(self):
-        cl = merge_changelog.read_changelog(v_112_1)
-        self.assertEqual(1, len(cl._blocks))
+    This is useful for adding details which are short strings.
+    """
+    return Content(UTF8_TEXT, lambda: [text.encode('utf8')])
 
 
 class TestMergeChangelog(tests.TestCase):
 
+    def setUp(self):
+        super(tests.TestCase, self).setUp()
+        # Intercept warnings from merge_changelog's logger: this is where 
+        self.logged_warnings = self.make_utf8_encoded_stringio()
+        self.addCleanup(self.addMergeChangelogWarningsDetail)
+        handler = logging.StreamHandler(self.logged_warnings)
+        handler.setLevel(logging.WARNING)
+        logger = logging.getLogger('bzr.plugins.builddeb.merge_changelog')
+        logger.addHandler(handler)
+        self.addCleanup(logger.removeHandler, handler)
+        self.overrideAttr(logger, 'propagate', False)
+
+    def addMergeChangelogWarningsDetail(self):
+        warnings_log = self.logged_warnings.getvalue()
+        if warnings_log:
+            self.addDetail(
+                'merge_changelog warnings', text_content(warnings_log))
+
     def assertMergeChangelog(self, expected_lines, this_lines, other_lines,
                              base_lines=[], conflicted=False):
         status, merged_lines = merge_changelog.merge_changelog(
@@ -139,10 +196,10 @@
             base_lines=[])
 
     def test_unsorted(self):
-        # Passing in an improperly sorted text should result in a properly
-        # sorted one
+        # The order of entries being merged is unchanged, even if they are not
+        # properly sorted.  (This is a merge tool, not a reformatting tool.)
         self.assertMergeChangelog(v_111_2 + v_001_1,
-                                  this_lines = v_001_1 + v_111_2,
+                                  this_lines = v_111_2 + v_001_1,
                                   other_lines = [],
                                   base_lines = [])
 
@@ -158,20 +215,11 @@
 
     def test_3way_conflicted(self):
         self.assertMergeChangelog(
-            expected_lines=['<<<<<<< TREE\n']
-                           + v_111_2b
-                           + ['=======\n']
-                           + v_111_2c
-                           + ['>>>>>>> MERGE-SOURCE\n'],
+            expected_lines=v_111_2bc,
             this_lines=v_111_2b, other_lines=v_111_2c,
-            base_lines=v_111_2,
-            conflicted=True)
+            base_lines=v_111_2)
         self.assertMergeChangelog(
-            expected_lines=['<<<<<<< TREE\n']
-                           + v_111_2b
-                           + ['=======\n']
-                           + v_111_2c
-                           + ['>>>>>>> MERGE-SOURCE\n'],
+            expected_lines=v_111_2bc_empty_base,
             this_lines=v_111_2b, other_lines=v_111_2c,
             base_lines=[],
             conflicted=True)
@@ -186,30 +234,30 @@
  -- Thu, 28 Jan 2010 10:45:44 +0000
 
 """.splitlines(True)
-        # Missing the author and we don't have allow_missing_author set
-        cl = changelog.Changelog()
-        self.assertRaises(changelog.ChangelogParseError,
-                          cl.parse_changelog, ''.join(invalid_changelog), strict=True)
-        # If strict parsing fails, don't try to do special merging
-        self.assertEqual(('not_applicable', None),
-            merge_changelog.merge_changelog(invalid_changelog, v_111_2,
-                                            v_111_2))
-        self.assertEqual(('not_applicable', None),
-            merge_changelog.merge_changelog(v_111_2, invalid_changelog,
-                                            v_111_2))
-        # We are non-strict about parsing BASE, because its contents are not
-        # included in the output.
-        # This triggers a warning, but we don't want to clutter the test run
-        cur_filters = warnings.filters[:]
-        warnings.simplefilter('ignore', UserWarning)
-        try:
-            self.assertMergeChangelog(v_112_1 + v_111_2,
-                                      this_lines=v_111_2,
-                                      other_lines=v_112_1,
-                                      base_lines=invalid_changelog,
-                                      )
-        finally:
-            warnings.filters = cur_filters[:]
+        # invalid_changelog is missing the author, but dpkg-mergechangelogs
+        # copes gracefully with invalid input.
+        status, lines = merge_changelog.merge_changelog(
+            invalid_changelog, v_111_2, v_111_2)
+        self.assertEqual('success', status)
+        # XXX: ideally we'd expect ''.join(lines) ==
+        # ''.join(invalid_changelog), but dpkg-mergechangelogs appears to lose
+        # the final line in these examples.
+        # <https://bugs.launchpad.net/ubuntu/+source/dpkg/+bug/815704>
+        #  - Andrew Bennetts, 25 July 2011.
+        #self.assertEqual(''.join(invalid_changelog), ''.join(lines))
+        status, lines = merge_changelog.merge_changelog(
+            invalid_changelog, v_111_2, v_111_2)
+        self.assertEqual('success', status)
+        #self.assertEqual(''.join(invalid_changelog), ''.join(lines))
+        self.assertMergeChangelog(v_112_1 + 
+                                  ['<<<<<<<\n'] +
+                                  v_111_2 +
+                                  ['=======\n>>>>>>>\n'],
+                                  this_lines=v_111_2,
+                                  other_lines=v_112_1,
+                                  base_lines=invalid_changelog,
+                                  conflicted=True
+                                  )
 
 
 class TestChangelogHook(tests.TestCaseWithMemoryTransport):