Code review comment for lp:~michael.nelson/launchpad/436182-newlines-in-sources-fix-without-workaround

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. skipping the first value prior to the first \n for example.)
>
> Is there another branch to follow this? If not, and this is important,
> please add a bug number to this so that it doesn't get forgotten,
> otherwise please remove it.

No - it was a note to myself. Removed.

>
> > +
> > + 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 not strip leading
>
> s/not/does not/

Thanks!
>
> > + spaces.
> > +
> > + See http://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Description
> > + """
> > + from lp.archiveuploader.tagfiles import parse_tagfile
> > +
> > + 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 contains more than two spaces,"
> > + " it should not\n"
> > + " be stripped at all and be displayed verbatim. Like this one:\n"
>
> Isn't it plainer to say that leading spaces are not stripped in any
> circumstances? Am I missing something?

Erm, yes. I should have just written that (as stated in the debian
policy) lines that if a line starts with 2 or more spaces it will be
displayed verbatim (which is not relevant to our parsing here) - just as
an example of a description. Done.

Thanks Gavin!