Merge lp:~michael.nelson/launchpad/436182-newlines-in-sources-fix-without-workaround into lp:launchpad

[Michael Nelson]

[Michael Nelson (michael.nelson)]

= Summary =
This branch fixes a soyuz parsing error when a debian source control
file is uploaded.

The error is described in bug 436182 (although, somewhat confusedly...
as I was discovering the issues as I went).

To re-cap, soyuz has a function for parsing tag files (such as debian
source control files - dsc, or changes files), which should adhere to

http://www.debian.org/doc/debian-policy/ch-controlfields.html

but it doesn't in a number of cases, in particular one case which
started occurring recently - fields containing values that span multiple
lines.

The original issue that is noted in bug 436182 is that, when a value for
a field spans multiple lines, our parser was leaving a trailing '\n' on
the parsed value - which caused the Source index to be created with a
*blank* line (as shown in the original snippet in the bug description).
This is completely incorrect and causes any reparsing of this file by
standard libraries (such as apt_pkg.ParseTagFile) to fail.

While creating a workaround for this a second issue was discovered: our
parser removes the required space/tab from subsequent lines of a value
that spans multiple lines. According to the policy:

"Many fields' values may span several lines; in this case each
continuation line must start with a space or a tab."

This issue can also be seen in the example snippet in the bug
description. The effect that a Source generated from this incorrectly
parsed data will be reparsed by apt_pgk.ParseTagFile which will ignore
the subsequent lines that are not preceded with space or tab.

== Proposed fix ==

I've already landed a work-around that will handle the corrupt data in
the database (of which there are 27 as of 2009-10-06) when generating
the index files (see
http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/9606
and
http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/9654
) and will get this cherry-picked.

This branch itself ensures that our parse_tagfiles function parses the
three different types of multi-line values available in tag files (a
new-line-delimited value, a value spanning multiple lines and a
description) with exactly the same result as apt_pkg.ParseTagFile().
Part of the reason for this is to make it easier to later remove the
function and replace it with apt_pkg.ParseTagFile() - if that's an
option (I need to talk with Colin or someone who knows the history
regarding why we've created our own parsing function).

== Pre-implementation notes ==

== Implementation details ==

== Tests ==

bin/test -vv -t Testtagfiles -t TestTagFileDebianPolicyCompat

== Demo and Q/A ==

I'll be QA'ing this on DF.

= Launchpad lint =

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

Wow - ok, so I guess this file hasn't been touched since our python
style guide came into effect ;).

I've fixed all the lint issues below.

Linting changed files:
  lib/lp/archiveuploader/tagfiles.py
  lib/lp/archiveuploader/tests/test_tagfiles.py
  lib/lp/archiveuploader/tests/data/test436182_0.1_source.changes

== Pyflakes notices ==

lib/lp/archiveuploader/tests/test_tagfiles.py
    10: 'sys'...

[Gavin Panella (allenap)]

Hi Michael,

I looked at the diff (revisions 9647 and 9648) and there's nothing to
comment on there, so the diff I've reviewed doesn't contain those
revisions.

My only comments below are almost trivial, so r=me. It was a very good
idea to compare to apt_pkg. Also, parse_tagfile_lines is a ugly pig of
a function!

Gavin.

> === modified file 'lib/lp/archiveuploader/tagfiles.py'
> --- lib/lp/archiveuploader/tagfiles.py 2009-06-24 23:33:29 +0000
> +++ lib/lp/archiveuploader/tagfiles.py 2009-10-06 12:09:42 +0000
> @@ -64,8 +64,8 @@
> """This exception is raised if parse_changes encounters nastiness"""
> pass
>
> -re_single_line_field = re.compile(r"^(\S*)\s*:\s*(.*)");
> -re_multi_line_field = re.compile(r"^\s(.*)");
> +re_single_line_field = re.compile(r"^(\S*)\s*:\s*(.*)")
> +re_multi_line_field = re.compile(r"^(\s.*)")
>
>
> def parse_tagfile_lines(lines, dsc_whitespace_rules=0, allow_unsigned=False,
> @@ -103,7 +103,8 @@
>
> num_of_lines = len(indexed_lines.keys())
> index = 0
> - first = -1
> + first_value_for_newline_delimited_field = False
> + more_values_can_follow = False
> while index < num_of_lines:
> index += 1
> line = indexed_lines[index]
> @@ -145,21 +146,47 @@
> if slf:
> field = slf.groups()[0].lower()
> changes[field] = slf.groups()[1]
> - first = 1
> +
> + # If there is no value on this line, we assume this is
> + # the first line of a multiline field, such as the 'files'
> + # field.
> + if changes[field] == '':
> + first_value_for_newline_delimited_field = True
> +
> + # Either way, more values for this field could follow
> + # on the next line.
> + more_values_can_follow = True
> continue
> if line.rstrip() == " .":
> - changes[field] += '\n'
> + changes[field] += '\n' + line
> continue
> mlf = re_multi_line_field.match(line)
> if mlf:
> - if first == -1:
> + if more_values_can_follow is False:
> raise TagFileParseError("%s: could not parse .changes file "
> "line %d: '%s'\n [Multi-line field continuing on from nothing?]"
> % (filename, index,line))
> - if first == 1 and changes[field] != "":
> - changes[field] += '\n'
> - first = 0
> - changes[field] += mlf.groups()[0] + '\n'
> +
> + # XXX Michael Nelson 20091001 bug=440014
> + # Is there any reason why we're not simply using
> + # apt_pkg.ParseTagFile instead of this looong function.
> + # If we can get rid of this code that is trying to mimic
> + # what ParseTagFile does out of the box, it would be a good
> + # thing.
> +
> + # The first value for a newline delimited field, such as
> + # the 'files' field, has its leading spaces stripped. Other
> + # fields (such as a 'binary' field spanning multiple lines)
> + # should *not* be l-stripped of their leading spaces otherwise
> + ...

review: Approve

[Michael Nelson (michael.nelson)]

On Wed, 2009-10-07 at 14:51 +0000, Gavin Panella wrote:
> Review: Approve
> Hi Michael,
>
> I looked at the diff (revisions 9647 and 9648) and there's nothing to
> comment on there, so the diff I've reviewed doesn't contain those
> revisions.
>
> My only comments below are almost trivial, so r=me. It was a very good
> idea to compare to apt_pkg. Also, parse_tagfile_lines is a ugly pig of
> a function!

Thanks Gavin - yeah, I'm keen to get rid of parse_tagfile_lines in the
future if possible.

>
> Gavin.
>

[snip]

> > === modified file 'lib/lp/archiveuploader/tests/test_tagfiles.py'
> > --- lib/lp/archiveuploader/tests/test_tagfiles.py 2009-06-24 23:33:29 +0000
> > +++ lib/lp/archiveuploader/tests/test_tagfiles.py 2009-10-07 11:10:35 +0000
> > @@ -5,6 +5,7 @@
> >
> > # arch-tag: 52e0c871-49a3-4186-beb8-9817d02d5465
> >
> > +import apt_pkg
> > import unittest
> > import sys
> > import shutil
> > @@ -106,19 +107,110 @@
> > tf = parse_tagfile(datadir("changes-with-exploit-bottom"))
> > self.assertRaises(KeyError, tf.__getitem__, "you")
> >
> > +
> > +class TestTagFileDebianPolicyCompat(unittest.TestCase):
> > +
> > + def setUp(self):
> > + """Parse the test file using apt_pkg for comparison."""
> > +
> > + tagfile_path = datadir("test436182_0.1_source.changes")
> > + tagfile = open(tagfile_path)
> > + self.apt_pkg_parsed_version = apt_pkg.ParseTagFile(tagfile)
> > + self.apt_pkg_parsed_version.Step()
> > +
> > + # Is it just because this is a very old file that test-related
> > + # things are imported locally?
> > + from lp.archiveuploader.tagfiles import parse_tagfile
> > + self.parse_tagfile_version = parse_tagfile(
> > + tagfile_path, allow_unsigned = True)
> > +
> > + def test_parse_tagfile_with_multiline_values(self):
> > + """parse_tagfile should not leave trailing '\n' on multiline values.
> > +
> > + This is a regression test for bug 436182.
> > + Previously we,
> > + 1. Stripped leading space/tab from subsequent lines of multiline
> > + values, and
> > + 2. appended a trailing '\n' to the end of the value.
> > + """
> > +
> > + expected_text = (
> > + 'test75874, anotherbinary,\n'
> > + ' andanother, andonemore,\n'
> > + '\tlastone')
> > +
> > + self.assertEqual(
> > + expected_text,
> > + self.apt_pkg_parsed_version.Section['Binary'])
> > +
> > + self.assertEqual(
> > + expected_text,
> > + self.parse_tagfile_version['binary'])
> > +
> > + def test_parse_tagfile_with_newline_delimited_field(self):
> > + """parse_tagfile should not leave leading or tailing '\n' when
> > + parsing newline delimited fields.
> > +
> > + Newline-delimited fields should be parsed to match
> > + apt_pkg.ParseTageFile.
> > +
> > + Note: in the past, our parse_tagfile function left the leading
> > + '\n' in the parsed value, whereas it should not have.
> > +
> > + XXX check where this value is used and ensure it won't be affected
> > + (ie. skipp...

[Michael Nelson (michael.nelson)]

Just adding a incremental that fixes test breakages - ensuring that we
always format the changes before displaying it in emails (so it appears
exactly as it used to in emails - without the '\n .' blank line token or
initial spaces.

Julian has already reviewed the incremental and I've put it through ec2
test so it's ready for testing on dogfood.

--
Michael

[Michael Nelson (michael.nelson)]

Hi Colin,

I'm adding you as a reviewer as apparently you've been around long enough to have a better understanding of the code in question here (our custom parse_tagfile_lines function), or at least know who else might :).

There is already a work-around that's been CP'd onto Germanium that ensures that the Source index creation of PPAs copes with the incorrectly parsed data.

This branch fixes the actual parsing error so that we won't get any more incorrectly-parsed dsc_binaries etc., ensuring that our custom parser does exactly what apt_pkg.ParseTagFile() would do in the given scenarios that failed.

The problem was not restricted to Binary fields in dsc files, but all values that can span multiple lines in both dsc and changes files. From my manual searching, as well as from the current test-suite coverage, this affected only:
  (1) Indexes generation for PPAs (per the bug) as we do not use apt-ftparchive,
  (2) formatting of text in Queue notification emails.
but there could be places that both the test coverage and I missed.

I've created bug 440014 to completely get rid of our custom parse_tagfile_lines function (replacing it with apt_pkg.ParseTagFile()).

I've also tested this branch on dogfood, details here: http://pastebin.ubuntu.com/292254/

So if you approve of this branch landing, when would you prefer to be landed? We could
 1. get it cherry-pick it now and so have some time to be condident that there are no unforseen problems, or
 2. land it in devel without a CP, but then it will be rolled out in the beginning of Nov - probably not ideal, or
 3. wait until LP 3.1.10 has been rolled out, and Karmic is released, and commit it to devel then (perhaps CPing it). Safer - but then more corrupt data in the database.

The dsc files with Binary values spanning multiple lines have approximately doubled each week, - mainly from soures that produce many binaries (linux, mono, oo.org) being added to people's ppas:
24-09-2009 - 12
06-10-2009 - 24
13-10-2009 - 48

Thanks for your time!

[Colin Watson (cjwatson)]

> I'm adding you as a reviewer as apparently you've been around long enough to
> have a better understanding of the code in question here (our custom
> parse_tagfile_lines function), or at least know who else might :).

Thanks for asking me to review. The algorithmic changes look OK to me. I'm not sure I know any better than you why we have our own version of this function, though. It's been around since the very first version of Soyuz was committed (see r542, although it was called parse_changes back then). Even then, apt_pkg.ParseTagFile was used in the very same file, so it clearly wasn't that Daniel didn't know about it; although the TagFile class has apparently never been used outside tests.

Reasons I can think of why it might have needed to be a separate function:

  * no other way to enforce dsc_whitespace_rules?
  * problems with case-insensitivity at the time? (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=384182)
  * apt_pkg.ParseTagFile seems to really want a real file, not e.g. StringIO

> So if you approve of this branch landing, when would you prefer to be landed?
> We could
> 1. get it cherry-pick it now and so have some time to be condident that there
> are no unforseen problems, or
> 2. land it in devel without a CP, but then it will be rolled out in the
> beginning of Nov - probably not ideal, or
> 3. wait until LP 3.1.10 has been rolled out, and Karmic is released, and
> commit it to devel then (perhaps CPing it). Safer - but then more corrupt data
> in the database.

Given that it's much more likely to affect PPAs than the main archive, and as you say the amount of corrupt data is increasing, I'd be happy for this to be cherry-picked ASAP.

review: Approve

=== modified file 'lib/canonical/launchpad/emailtemplates/upload-accepted.txt'
--- lib/canonical/launchpad/emailtemplates/upload-accepted.txt	2008-09-18 11:08:16 +0000
+++ lib/canonical/launchpad/emailtemplates/upload-accepted.txt	2009-10-15 13:21:16 +0000
@@ -1,4 +1,5 @@
 %(CHANGESFILE)s
+
 %(DATE)s%(CHANGEDBY)s%(MAINTAINER)s%(SIGNER)s%(ORIGIN)s
 %(SPR_URL)s
 

=== modified file 'lib/canonical/launchpad/emailtemplates/upload-announcement.txt'
--- lib/canonical/launchpad/emailtemplates/upload-announcement.txt	2008-09-18 11:08:16 +0000
+++ lib/canonical/launchpad/emailtemplates/upload-announcement.txt	2009-10-15 13:21:16 +0000
@@ -1,3 +1,4 @@
 %(CHANGESFILE)s
+
 %(DATE)s%(CHANGEDBY)s%(MAINTAINER)s%(SIGNER)s%(ORIGIN)s
 %(SPR_URL)s

=== modified file 'lib/canonical/launchpad/emailtemplates/upload-new.txt'
--- lib/canonical/launchpad/emailtemplates/upload-new.txt	2007-07-03 11:15:10 +0000
+++ lib/canonical/launchpad/emailtemplates/upload-new.txt	2009-10-15 13:21:16 +0000
@@ -2,6 +2,7 @@
 
 %(CHANGESFILE)s
 
+
 Your package contains new components which requires manual editing of
 the override file.  It is ok otherwise, so please be patient.  New
 packages are usually added to the overrides about once a week.

=== modified file 'lib/canonical/launchpad/emailtemplates/upload-rejection.txt'
--- lib/canonical/launchpad/emailtemplates/upload-rejection.txt	2008-09-26 07:34:37 +0000
+++ lib/canonical/launchpad/emailtemplates/upload-rejection.txt	2009-10-15 13:21:16 +0000
@@ -2,6 +2,7 @@
 %(SUMMARY)s
 
 %(CHANGESFILE)s
+
 %(DATE)s%(CHANGEDBY)s%(MAINTAINER)s%(SIGNER)s%(ORIGIN)s
 
 ===

=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py	2009-06-24 23:33:29 +0000
+++ lib/lp/archiveuploader/changesfile.py	2009-10-15 13:21:16 +0000
@@ -307,10 +307,24 @@
         """Return changesfile claimed version."""
         return self._dict['version']
 
+    @classmethod
+    def formatChangesComment(cls, comment):
+        """A class utility method for formatting changes for display."""
+
+        # Return the display version of the comment using the
+        # debian policy rules. First replacing the blank line
+        # indicator '\n .' and then stripping one space from each
+        # successive line.
+        comment = comment.replace('\n .', '\n')
+        comment = comment.replace('\n ', '\n')
+        return comment
+
     @property
     def changes_comment(self):
         """Return changesfile 'change' comment."""
-        return self._dict['changes']
+        comment = self._dict['changes']
+
+        return self.formatChangesComment(comment)
 
     @property
     def date(self):

=== modified file 'lib/lp/archiveuploader/tagfiles.py'
--- lib/lp/archiveuploader/tagfiles.py	2009-06-24 23:33:29 +0000
+++ lib/lp/archiveuploader/tagfiles.py	2009-10-15 13:21:16 +0000
@@ -53,19 +53,19 @@
         """Expose a dicty has_key"""
         return key in self.stanza.keys()
 
-    """Enables (foo in bar) functionality"""
+    # Enables (foo in bar) functionality.
     __contains__ = has_key
 
     def items(self):
         """Allows for k,v in foo.items()"""
-        return [ (k,self.stanza[k]) for k in self.stanza.keys() ]
+        return [ (k, self.stanza[k]) for k in self.stanza.keys() ]
 
 class TagFileParseError(Exception):
     """This exception is raised if parse_changes encounters nastiness"""
     pass
 
-re_single_line_field = re.compile(r"^(\S*)\s*:\s*(.*)");
-re_multi_line_field = re.compile(r"^\s(.*)");
+re_single_line_field = re.compile(r"^(\S*)\s*:\s*(.*)")
+re_multi_line_field = re.compile(r"^(\s.*)")
 
 
 def parse_tagfile_lines(lines, dsc_whitespace_rules=0, allow_unsigned=False,
@@ -103,7 +103,8 @@
 
     num_of_lines = len(indexed_lines.keys())
     index = 0
-    first = -1
+    first_value_for_newline_delimited_field = False
+    more_values_can_follow = False
     while index < num_of_lines:
         index += 1
         line = indexed_lines[index]
@@ -116,11 +117,15 @@
             if dsc_whitespace_rules:
                 index += 1
                 if index > num_of_lines:
-                    raise TagFileParseError("%s: invalid .dsc file at line %d" % (filename, index))
+                    raise TagFileParseError(
+                        "%s: invalid .dsc file at line %d" % (
+                            filename, index))
                 line = indexed_lines[index]
                 if not line.startswith("-----BEGIN PGP SIGNATURE"):
-                    raise TagFileParseError("%s: invalid .dsc file at line %d -- "
-                        "expected PGP signature; got '%s'" % (filename, index,line))
+                    raise TagFileParseError(
+                        "%s: invalid .dsc file at line %d -- "
+                        "expected PGP signature; got '%s'" % (
+                            filename, index,line))
                 inside_signature = 0
                 break
             else:
@@ -128,8 +133,9 @@
         if line.startswith("-----BEGIN PGP SIGNATURE"):
             break
 
-        # If we're at the start of a signed section, then consume the signature
-        # information, and remember that we're inside the signed data.
+        # If we're at the start of a signed section, then consume the
+        # signature information, and remember that we're inside the signed
+        # data.
         if line.startswith("-----BEGIN PGP SIGNED MESSAGE"):
             inside_signature = 1
             if dsc_whitespace_rules:
@@ -145,31 +151,60 @@
         if slf:
             field = slf.groups()[0].lower()
             changes[field] = slf.groups()[1]
-            first = 1
+
+            # If there is no value on this line, we assume this is
+            # the first line of a multiline field, such as the 'files'
+            # field.
+            if changes[field] == '':
+                first_value_for_newline_delimited_field = True
+
+            # Either way, more values for this field could follow
+            # on the next line.
+            more_values_can_follow = True
             continue
         if line.rstrip() == " .":
-            changes[field] += '\n'
+            changes[field] += '\n' + line
             continue
         mlf = re_multi_line_field.match(line)
         if mlf:
-            if first == -1:
-                raise TagFileParseError("%s: could not parse .changes file "
-                "line %d: '%s'\n [Multi-line field continuing on from nothing?]"
-                % (filename, index,line))
-            if first == 1 and changes[field] != "":
-                changes[field] += '\n'
-            first = 0
-            changes[field] += mlf.groups()[0] + '\n'
+            if more_values_can_follow is False:
+                raise TagFileParseError(
+                    "%s: could not parse .changes file line %d: '%s'\n"
+                    " [Multi-line field continuing on from nothing?]" % (
+                        filename, index,line))
+
+            # XXX Michael Nelson 20091001 bug=440014
+            # Is there any reason why we're not simply using
+            # apt_pkg.ParseTagFile instead of this looong function.
+            # If we can get rid of this code that is trying to mimic
+            # what ParseTagFile does out of the box, it would be a good
+            # thing.
+
+            # The first value for a newline delimited field, such as
+            # the 'files' field, has its leading spaces stripped. Other
+            # fields (such as a 'binary' field spanning multiple lines)
+            # should *not* be l-stripped of their leading spaces otherwise
+            # they will be re-parsed incorrectly by apt_get.ParseTagFiles()
+            # (for example, from a Source index).
+            value = mlf.groups()[0]
+            if first_value_for_newline_delimited_field:
+                changes[field] = value.lstrip()
+            else:
+                changes[field] += '\n' + value
+
+            first_value_for_newline_delimited_field = False
             continue
         error += line
 
     if dsc_whitespace_rules and inside_signature:
-        raise TagFileParseError("%s: invalid .dsc format at line %d" % (filename, index))
+        raise TagFileParseError(
+            "%s: invalid .dsc format at line %d" % (filename, index))
 
     changes["filecontents"] = "".join(lines)
 
     if error:
-        raise TagFileParseError("%s: unable to parse .changes file: %s" % (filename, error))
+        raise TagFileParseError(
+            "%s: unable to parse .changes file: %s" % (filename, error))
 
     return changes
 

=== added file 'lib/lp/archiveuploader/tests/data/test436182_0.1_source.changes'
--- lib/lp/archiveuploader/tests/data/test436182_0.1_source.changes	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/test436182_0.1_source.changes	2009-10-15 13:21:16 +0000
@@ -0,0 +1,23 @@
+Format: 1.0
+Date: Fri, 15 Dec 2006 12:02:42 +0000
+Source: test75874
+Binary: test75874 anotherbinary
+ andanother andonemore
+	lastone
+Architecture: all
+Version: 0.1
+Maintainer: Colin Watson <[e-mail]>
+Description: Here's the single-line synopsis.
+ Then there is the extended description which can
+ span multiple lines, and even include blank-lines like this:
+ .
+ There you go. If a line starts with two or more spaces,
+ it will be displayed verbatim. Like this one:
+  Example verbatim line.
+    Another verbatim line.
+ OK, back to normal.
+Files: 
+ f26bb9b29b1108e53139da3584a4dc92 1511 test75874_0.1.tar.gz
+ 29c955ff520cea32ab3e0316306d0ac1 393742 pmount_0.9.7.orig.tar.gz
+ 91a8f46d372c406fadcb57c6ff7016f3 5302 pmount_0.9.7-2ubuntu2.diff.gz
+

=== modified file 'lib/lp/archiveuploader/tests/test_tagfiles.py'
--- lib/lp/archiveuploader/tests/test_tagfiles.py	2009-06-24 23:33:29 +0000
+++ lib/lp/archiveuploader/tests/test_tagfiles.py	2009-10-15 13:21:16 +0000
@@ -5,23 +5,18 @@
 
 # arch-tag: 52e0c871-49a3-4186-beb8-9817d02d5465
 
+import apt_pkg
 import unittest
-import sys
-import shutil
 from lp.archiveuploader.tests import datadir
 
+from lp.archiveuploader.tagfiles import (
+    parse_tagfile, TagFile, TagFileParseError)
+
 class Testtagfiles(unittest.TestCase):
 
-    def testImport(self):
-        """lp.archiveuploader.tagfiles should be importable"""
-        from lp.archiveuploader.tagfiles import TagFile
-        from lp.archiveuploader.tagfiles import TagFileParseError
-        from lp.archiveuploader.tagfiles import parse_tagfile
-
     def testTagFileOnSingular(self):
         """lp.archiveuploader.tagfiles.TagFile should parse a singular stanza
         """
-        from lp.archiveuploader.tagfiles import TagFile
         f = TagFile(file(datadir("singular-stanza"), "r"))
         seenone = False
         for stanza in f:
@@ -32,8 +27,7 @@
             self.assertEquals("FooBar" in stanza, False)
 
     def testTagFileOnSeveral(self):
-        """lp.archiveuploader.tagfiles.TagFile should parse multiple stanzas"""
-        from lp.archiveuploader.tagfiles import TagFile
+        """TagFile should parse multiple stanzas."""
         f = TagFile(file(datadir("multiple-stanzas"), "r"))
         seen = 0
         for stanza in f:
@@ -47,15 +41,12 @@
         """lp.archiveuploader.tagfiles.parse_tagfile should work on a good
            changes file
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
         p = parse_tagfile(datadir("good-signed-changes"))
 
     def testCheckParseBadChangesRaises(self):
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on failure
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
-        from lp.archiveuploader.tagfiles import TagFileParseError
         self.assertRaises(TagFileParseError,
                           parse_tagfile, datadir("badformat-changes"), 1)
 
@@ -63,8 +54,6 @@
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on empty
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
-        from lp.archiveuploader.tagfiles import TagFileParseError
         self.assertRaises(TagFileParseError,
                           parse_tagfile, datadir("empty-file"), 1)
 
@@ -72,16 +61,12 @@
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on malformed signatures
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
-        from lp.archiveuploader.tagfiles import TagFileParseError
         self.assertRaises(TagFileParseError,
                           parse_tagfile, datadir("malformed-sig-changes"), 1)
 
     def testCheckParseMalformedMultilineRaises(self):
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on malformed continuation lines"""
-        from lp.archiveuploader.tagfiles import parse_tagfile
-        from lp.archiveuploader.tagfiles import TagFileParseError
         self.assertRaises(TagFileParseError,
                           parse_tagfile, datadir("bad-multiline-changes"), 1)
 
@@ -89,8 +74,6 @@
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on unterminated signatures
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
-        from lp.archiveuploader.tagfiles import TagFileParseError
         self.assertRaises(TagFileParseError,
                           parse_tagfile,
                           datadir("unterminated-sig-changes"),
@@ -100,25 +83,106 @@
         """lp.archiveuploader.tagfiles.parse_tagfile should not be vulnerable
            to tags outside of the signed portion
         """
-        from lp.archiveuploader.tagfiles import parse_tagfile
         tf = parse_tagfile(datadir("changes-with-exploit-top"))
         self.assertRaises(KeyError, tf.__getitem__, "you")
         tf = parse_tagfile(datadir("changes-with-exploit-bottom"))
         self.assertRaises(KeyError, tf.__getitem__, "you")
 
+
+class TestTagFileDebianPolicyCompat(unittest.TestCase):
+
+    def setUp(self):
+        """Parse the test file using apt_pkg for comparison."""
+
+        tagfile_path = datadir("test436182_0.1_source.changes")
+        tagfile = open(tagfile_path)
+        self.apt_pkg_parsed_version = apt_pkg.ParseTagFile(tagfile)
+        self.apt_pkg_parsed_version.Step()
+
+        self.parse_tagfile_version = parse_tagfile(
+            tagfile_path, allow_unsigned = True)
+
+    def test_parse_tagfile_with_multiline_values(self):
+        """parse_tagfile should not leave trailing '\n' on multiline values.
+
+        This is a regression test for bug 436182.
+        Previously we,
+          1. Stripped leading space/tab from subsequent lines of multiline
+             values, and
+          2. appended a trailing '\n' to the end of the value.
+        """
+
+        expected_text = (
+            'test75874 anotherbinary\n'
+            ' andanother andonemore\n'
+            '\tlastone')
+
+        self.assertEqual(
+            expected_text,
+            self.apt_pkg_parsed_version.Section['Binary'])
+
+        self.assertEqual(
+            expected_text,
+            self.parse_tagfile_version['binary'])
+
+    def test_parse_tagfile_with_newline_delimited_field(self):
+        """parse_tagfile should not leave leading or tailing '\n' when
+        parsing newline delimited fields.
+
+        Newline-delimited fields should be parsed to match
+        apt_pkg.ParseTageFile.
+
+        Note: in the past, our parse_tagfile function left the leading
+        '\n' in the parsed value, whereas it should not have.
+
+        For an example,
+        see http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Files
+        """
+
+        expected_text = (
+            'f26bb9b29b1108e53139da3584a4dc92 1511 test75874_0.1.tar.gz\n '
+            '29c955ff520cea32ab3e0316306d0ac1 393742 '
+                'pmount_0.9.7.orig.tar.gz\n'
+            ' 91a8f46d372c406fadcb57c6ff7016f3 5302 '
+                'pmount_0.9.7-2ubuntu2.diff.gz')
+
+        self.assertEqual(
+            expected_text,
+            self.apt_pkg_parsed_version.Section['Files'])
+
+        self.assertEqual(
+            expected_text,
+            self.parse_tagfile_version['files'])
+
+    def test_parse_description_field(self):
+        """Apt-pkg preserves the blank-line indicator and does not strip
+        leading spaces.
+
+        See http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Description
+        """
+        expected_text = (
+            "Here's the single-line synopsis.\n"
+            " Then there is the extended description which can\n"
+            " span multiple lines, and even include blank-lines like this:\n"
+            " .\n"
+            " There you go. If a line starts with two or more spaces,\n"
+            " it will be displayed verbatim. Like this one:\n"
+            "  Example verbatim line.\n"
+            "    Another verbatim line.\n"
+            " OK, back to normal.")
+
+        self.assertEqual(
+            expected_text,
+            self.apt_pkg_parsed_version.Section['Description'])
+
+        # In the past our parse_tagfile function replaced blank-line
+        # indicators in the description (' .\n') with new lines ('\n'),
+        # but it is now compatible with ParseTagFiles (and ready to be
+        # replaced by ParseTagFiles).
+        self.assertEqual(
+            expected_text,
+            self.parse_tagfile_version['description'])
+
 def test_suite():
-    suite = unittest.TestSuite()
-    loader = unittest.TestLoader()
-    suite.addTest(loader.loadTestsFromTestCase(Testtagfiles))
-    return suite
-
-def main(argv):
-    suite = test_suite()
-    runner = unittest.TextTestRunner(verbosity = 2)
-    if not runner.run(suite).wasSuccessful():
-        return 1
-    return 0
-
-if __name__ == '__main__':
-    sys.exit(main(sys.argv))
+    return unittest.TestLoader().loadTestsFromName(__name__)
 

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2009-10-09 11:13:30 +0000
+++ lib/lp/soyuz/model/queue.py	2009-10-15 13:21:16 +0000
@@ -33,6 +33,7 @@
 from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.customupload import CustomUploadError
 from lp.archivepublisher.utils import get_ppa_reference
+from lp.archiveuploader.changesfile import ChangesFile
 from lp.archiveuploader.tagfiles import parse_tagfile_lines
 from lp.archiveuploader.utils import safe_fix_maintainer
 from lp.buildmaster.pas import BuildDaemonPackagesArchSpecific
@@ -747,14 +748,16 @@
             """PPA rejected message."""
             template = get_email_template('ppa-upload-rejection.txt')
             SUMMARY = sanitize_string(summary_text)
-            CHANGESFILE = sanitize_string("".join(changes_lines))
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment("".join(changes_lines)))
             USERS_ADDRESS = config.launchpad.users_address
 
         class RejectedMessage:
             """Rejected message."""
             template = get_email_template('upload-rejection.txt')
             SUMMARY = sanitize_string(summary_text)
-            CHANGESFILE = sanitize_string(changes['changes'])
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment(changes['changes']))
             CHANGEDBY = ''
             ORIGIN = ''
             SIGNER = ''
@@ -831,7 +834,8 @@
 
             STATUS = "New"
             SUMMARY = summarystring
-            CHANGESFILE = sanitize_string(changes['changes'])
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment(changes['changes']))
             DISTRO = self.distroseries.distribution.title
             if announce_list:
                 ANNOUNCE = 'Announcing to %s' % announce_list
@@ -845,7 +849,8 @@
             STATUS = "Waiting for approval"
             SUMMARY = summarystring + (
                     "\nThis upload awaits approval by a distro manager\n")
-            CHANGESFILE = sanitize_string(changes['changes'])
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment(changes['changes']))
             DISTRO = self.distroseries.distribution.title
             if announce_list:
                 ANNOUNCE = 'Announcing to %s' % announce_list
@@ -863,7 +868,8 @@
 
             STATUS = "Accepted"
             SUMMARY = summarystring
-            CHANGESFILE = sanitize_string(changes['changes'])
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment(changes['changes']))
             DISTRO = self.distroseries.distribution.title
             if announce_list:
                 ANNOUNCE = 'Announcing to %s' % announce_list
@@ -881,14 +887,16 @@
 
             STATUS = "Accepted"
             SUMMARY = summarystring
-            CHANGESFILE = guess_encoding("".join(changes_lines))
+            CHANGESFILE = guess_encoding(
+                ChangesFile.formatChangesComment("".join(changes_lines)))
 
         class AnnouncementMessage:
             template = get_email_template('upload-announcement.txt')
 
             STATUS = "Accepted"
             SUMMARY = summarystring
-            CHANGESFILE = sanitize_string(changes['changes'])
+            CHANGESFILE = sanitize_string(
+                ChangesFile.formatChangesComment(changes['changes']))
             CHANGEDBY = ''
             ORIGIN = ''
             SIGNER = ''