Merge lp:~jelmer/launchpad/635591-sync-source-unicode into lp:launchpad

[Jelmer Vernooij]

Fix handling of non-UTF8 changelog entries in sync-source.

This branch fixes the ability to sync sources that contain a changelog entry with non-ASCII UTF8 characters.

I've moved generate_changes() so that it would be possible to write unit tests for it.

tests: ./bin/test lp.soyuz.scripts.test_sync_source

[Gavin Panella (allenap)]

Nice branch :)

A few comments, but they're all minor. +1

[1]

There's some trailing whitespace in ftpmaster.py. There are some other
lint warnings (especially from pep8) but I don't know if any of them
are related to your changes. If you have time, consider fixing them,
but don't worry too much.

[2]

+ # Strip trailing newline
+ return changes

I think that comment can go.

[3]

+ self.assertFalse("Description" in changes)
+ self.assertFalse("Closes" in changes)
+ self.assertFalse("Launchpad-bugs-fixed" in changes)
...
+ self.assertFalse("Launchpad-bugs-fixed" in changes)

If you were to switch the base class from unittest.TestCase to
lp.testing.TestCase (or testtools.TestCase, its parent) then you could
use assertNotIn() in these places.

[4]

+ self.assertTrue(
+ contents.find("Updated French translation by J\xc3\xa9lmer."))

I think this would be clearer as:

        self.assertTrue(
            "Updated French translation by J\xc3\xa9lmer." in contents)

Or, better:

        self.assertIn(
            "Updated French translation by J\xc3\xa9lmer.", contents)

[5]

     filehandle = open(output_filename, 'w')
- # XXX cprov 2007-07-03: The Soyuz .changes parser requires the extra '\n'
- filehandle.write(changes+'\n')
- filehandle.close()
+ try:
+ changes.dump(filehandle, encoding="utf-8")
+ finally:
+ filehandle.close()

Consider using "with" here.

review: Approve (code)

=== modified file 'lib/lp/soyuz/scripts/ftpmaster.py'
--- lib/lp/soyuz/scripts/ftpmaster.py	2010-09-16 11:14:42 +0000
+++ lib/lp/soyuz/scripts/ftpmaster.py	2010-09-16 18:46:12 +0000
@@ -1464,7 +1464,6 @@
         changes["Closes"] = " ".join(closes)
     if lp_closes:
         changes["Launchpad-bugs-fixed"] = " ".join(lp_closes)
-    changes["Changes"] = "\n%s" % changelog
     files = []
     for filename in dsc_files:
         if filename in files_from_librarian:
@@ -1477,4 +1476,5 @@
                      })
 
     changes["Files"] = files
+    changes["Changes"] = "\n%s" % changelog
     return changes

=== modified file 'lib/lp/soyuz/scripts/tests/test_sync_source.py'
--- lib/lp/soyuz/scripts/tests/test_sync_source.py	2010-09-16 11:35:26 +0000
+++ lib/lp/soyuz/scripts/tests/test_sync_source.py	2010-09-16 18:46:12 +0000
@@ -5,7 +5,9 @@
 
 __metaclass__ = type
 
+from cStringIO import StringIO
 from debian.deb822 import (
+    Changes,
     Deb822Dict,
     Dsc,
     )
@@ -507,6 +509,31 @@
         self.assertIn(
             "Updated French translation by J\xc3\xa9lmer.", contents)
 
+    def test_changelog_whitelines(self):
+        # The changelog entry can contain empty lines, and this should not
+        # mess up the parsing of the changes file.
+        changelog = "* Foo\n\n\n* Bar\n.\nEntries"
+        changes = self.generateChanges(changelog=changelog)
+        contents = changes.dump(encoding="utf-8").encode("utf-8")
+        # Read contents back
+        read_changes = Changes(StringIO(
+            changes.dump(encoding="utf-8").encode("utf-8")))
+        self.assertEquals("\n%s" % changelog, changes['Changes'])
+        self.assertContentEqual([
+            'Architecture',
+            'Binary',
+            'Changed-By',
+            'Changes',
+            'Date',
+            'Distribution',
+            'Files',
+            'Format',
+            'Maintainer',
+            'Origin',
+            'Source',
+            'Urgency',
+            'Version',
+            ], read_changes.keys())
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)