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
> +            # 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
>
>
> === 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-07 11:10:35 +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 <email address hidden>
> +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 contains more than two spaces, it should not
> + be stripped at all and be displayed verbatim. Like this one:
> +  Don't strip me.
> +    Or me.
> + 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-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.

> +
> +        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/

> +        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?

> +            "  Don't strip me.\n"
> +            "    Or me.\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__)
>