launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17257
Re: [Merge] lp:~cjohnston/launchpad/short-ic-emails into lp:launchpad
Review: Needs Fixing code
This breaks three tests in TestInlineCommentsSection, and I have a few other comments.
Diff comments:
> === modified file 'lib/lp/code/mail/codereviewcomment.py'
> --- lib/lp/code/mail/codereviewcomment.py 2014-05-21 08:52:53 +0000
> +++ lib/lp/code/mail/codereviewcomment.py 2014-07-26 02:00:47 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Email notifications for code review comments."""
> @@ -26,6 +26,7 @@
> append_footer,
> format_address,
> )
> +from lp.services.patches import parse_patches
> from lp.services.webapp import canonical_url
>
>
> @@ -171,16 +172,51 @@
>
> def build_inline_comments_section(comments, diff_text):
> """Return a formatted text section with contextualized comments."""
> - result_lines = []
> - diff_lines = diff_text.splitlines()
> - for num, line in enumerate(diff_lines, 1):
> - result_lines.append(u'> {0}'.format(line.decode('utf-8', 'replace')))
> - comment = comments.get(str(num))
> - if comment is not None:
> - result_lines.append('')
> - result_lines.extend(comment.splitlines())
> - result_lines.append('')
> -
> - result_text = u'\n'.join(result_lines)
> + result_diff = []
> + lines = diff_text.splitlines(True)
> + patches = parse_patches(lines, allow_dirty=True)
> + line_count = 0
> + for patch in patches:
> + patch_code = []
> + patch_has_ic = False
> + headers = patch.get_header()
> + has_header_ic = False
> + header_code = []
The nomenclature is a bit confusing.
diff_code is raw diff lines.
But then we have result_diff, patch_code, header_code and hunk_code. Despite the different suffixes, they all store the same mixture of formatted diff and comment lines. Despite the match with the suffix of diff_code, they store a different type of data. None are raw diff or code.
> + for line in headers.splitlines():
> + line_count += 1
> + comment = comments.get(str(line_count))
> + header_code.append(u'> {0}'.format(
> + line.decode('utf-8', 'replace')))
> + if comment is not None:
> + header_code.append('')
> + header_code.extend(comment.splitlines())
> + header_code.append('')
> + has_header_ic = True
> +
> + for p in patch.hunks:
> + hunk_code = []
> + has_ic = False
Can you do away with has_ic and patch_has_ic by checking for emptiness of hunk_code and patch_code?
> + diff_code = str(p).splitlines()
> + for num, line in enumerate(diff_code, 1):
> + line_count += 1
> + hunk_code.append(u'> {0}'.format(
> + line.decode('utf-8', 'replace')))
> + comment = comments.get(str(line_count))
> + if comment is not None:
> + hunk_code.append('')
> + hunk_code.extend(comment.splitlines())
> + hunk_code.append('')
> + has_ic = True
> + if has_ic:
> + patch_code += hunk_code
> + patch_has_ic = True
> + else:
> + if (len(patch_code) > 0 and
> + patch_code[-1] != u'') or len(patch_code) == 0:
"if not patch_code or patch_code[-1] != u'':"
> + patch_code.append('')
> + if has_header_ic or patch_has_ic:
> + result_diff += header_code + patch_code
> +
> + result_text = u'\n'.join(result_diff)
>
> return '\n\nDiff comments:\n\n%s\n\n' % result_text
>
> === modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
> --- lib/lp/code/mail/tests/test_codereviewcomment.py 2014-05-21 09:33:04 +0000
> +++ lib/lp/code/mail/tests/test_codereviewcomment.py 2014-07-26 02:00:47 +0000
> @@ -1,8 +1,9 @@
> -# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
> +# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Test CodeReviewComment emailing functionality."""
>
> +import os
>
> import testtools
> import transaction
> @@ -31,6 +32,12 @@
> from lp.testing.layers import LaunchpadFunctionalLayer
>
>
> +def datafile(filename):
> + data_path = os.path.join(os.path.dirname(__file__),
> + "../../../services/tests/data", filename)
> + return file(data_path, "rb")
> +
> +
> class TestCodeReviewComment(TestCaseWithFactory):
> """Test that comments are generated as expected."""
>
> @@ -215,13 +222,14 @@
> mailer.message.text_contents.splitlines())
>
> def makeCommentWithInlineComments(self, subject=None, content=None,
> - inline_comments=None):
> + inline_comments=None, diff_text=None):
> """Create a `CodeReviewComment` with inline (diff) comments."""
> bmp = self.factory.makeBranchMergeProposal()
> bmp.source_branch.subscribe(bmp.registrant,
> BranchSubscriptionNotificationLevel.NOEMAIL, None,
> CodeReviewNotificationLevel.FULL, bmp.registrant)
> - previewdiff = self.factory.makePreviewDiff(merge_proposal=bmp)
> + previewdiff = self.factory.makePreviewDiff(merge_proposal=bmp,
> + diff_text=diff_text)
> transaction.commit()
> if subject is None:
> subject = 'A comment'
> @@ -235,14 +243,184 @@
> inline_comments=inline_comments)
> return comment
>
> - def test_generateEmailWithInlineComments(self):
> - """Review comments emails consider the inline comments.
> -
> - See `build_inline_comments_section` tests for formatting details.
> - """
> - # Enabled corresponding feature flag.
> - self.useContext(feature_flags())
> - set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + # For the test_inline_comments_email_* tests, please see
> + # `build_inline_comments_section` tests for formatting details.
> +
> + def test_inline_comments_email_text(self):
> + # Testing that a single file with three hunks is parsed properly and
> + # only the header/hunk combinations with comments are provided.
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff = datafile("diff-1").read()
> + expected = datafile("expected-1").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'9': u'Cool',
> + '27': u'Are you sure?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_binary(self):
> + # Testing that a single file with three hunks and a binary file is
> + # parsed properly and only the binary header with comments are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff = u'\n'.join([diff_1, diff_2])
> + expected = datafile("expected-2").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'33': u'Why are you adding this?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_binary(self):
> + # Testing that a binary file is parsed properly and only the binary
> + # header with comments are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff = datafile("diff-2").read()
> + expected = datafile("expected-2").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'2': u'Why are you adding this?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_binary_text(self):
> + # Testing that a single file with three hunks, a binary file and a
> + # text file is parsed properly and only the binary header with comments
> + # are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff_3 = datafile("diff-3").read()
> + diff = u'\n'.join([diff_1, diff_2, diff_3])
> + expected = datafile("expected-3").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'43': u'What is this?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_binary_text_includes_two_files(self):
> + # Testing that a single file with three hunks, a binary file and a
> + # text file is parsed properly and a single hunk from the first file
> + # plus a single hunk from the second file with comments are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff_3 = datafile("diff-3").read()
> + diff = u'\n'.join([diff_1, diff_2, diff_3])
> + expected = datafile("expected-5").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'18': u'Why?',
> + '43': u'Good catch!'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_binary_text_includes_three_files(self):
> + # Testing that a single file with three hunks, a binary file and a
> + # text file is parsed properly and two hunks from the first file,
> + # the second file and a single hunk from the second file with comments
> + # are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff_3 = datafile("diff-3").read()
> + diff = u'\n'.join([diff_1, diff_2, diff_3])
> + expected = datafile("expected-6").read().decode().splitlines()
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'27': u'+1',
Why'd you drop the line 9 comment from here in r17094?
> + '33': u'Do we need this?',
> + '43': u'Can we change this?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_binary_text_no_nl_files(self):
> + # Testing that a single file with three hunks, a binary file and a
> + # text file which do not have a new line between each other is parsed
> + # properly and two hunks from the first file, the second file and a
> + # single hunk from the second file with comments are provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff_3 = datafile("diff-3").read()
> + diff = u''.join([diff_1, diff_2, diff_3])
> + expected = datafile("expected-7").read().decode().splitlines()
There is no expected-7.
> +
> + comment = self.makeCommentWithInlineComments(
> + inline_comments={'9': u'Good stuff',
> + '27': u'+1',
> + '32': u'Do we need this?',
> + '41': u'Can we change this?'},
> + diff_text=diff)
> + mailer = CodeReviewCommentMailer.forCreation(comment)
> + commenter = comment.branch_merge_proposal.registrant
> + ctrl = mailer.generateEmail(
> + commenter.preferredemail.email, commenter)
> +
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
> +
> + def test_inline_comments_email_text_comment_in_header(self):
> + # Testing that a single file with comments in the header is parsed
> + # properly and only the headers is provided
> +
> + # Enabled corresponding feature flag.
> + self.useContext(feature_flags())
> + set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
> + expected = datafile("expected-4").read().decode('utf-8').splitlines()
>
> comment = self.makeCommentWithInlineComments(
> inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
> @@ -251,20 +429,7 @@
> ctrl = mailer.generateEmail(
> commenter.preferredemail.email, commenter)
>
> - expected_lines = [
> - '',
> - 'Diff comments:',
> - '',
> - "> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'",
> - '> --- yvo/yc/pbqr/vagresnprf/qvss.cl '
> - '2009-10-01 13:25:12 +0000',
> - '',
> - u'Is this from Planet Earth\xa9 ?',
> - '',
> - '> +++ yvo/yc/pbqr/vagresnprf/qvss.cl '
> - '2010-02-02 15:48:56 +0000'
> - ]
> - self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
> + self.assertEqual(expected, ctrl.body.splitlines()[:-2])
>
> def test_generateEmailWithInlineComments_feature_disabled(self):
> """Inline comments are not considered if the flag is not enabled."""
>
> === added file 'lib/lp/services/patches.py'
> --- lib/lp/services/patches.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/patches.py 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,235 @@
> +# Copyright 2014 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +# This file is derived from bzrlib.patches in order to extend some
> +# functionality for use by the Launchpad inline comment feature.
> +
> +# In order to reduce the size of a diff being emailed with inline comments
> +# to just the relevant hunks of code, it was needed to break the diff down
> +# and then put the required pieces back together. To do this, the notion of
> +# 'comments' was added to Patch and BinaryPatch in order to return any lines
> +# beginning with '===' which is later added to the file headers and the line
> +# counted.
> +
> +import re
> +
> +from bzrlib.errors import (
> + BzrError,
> + MalformedHunkHeader,
> + MalformedPatchHeader,
> + PatchSyntax,
> + )
> +from bzrlib.patches import (
> + binary_files_re,
> + ContextLine,
> + hunk_from_header,
> + InsertLine,
> + iter_lines_handle_nl,
> + parse_line,
> + RemoveLine,
> + )
> +
> +
> +regex = re.compile(binary_files_re)
I'd use a more specific name.
> +
> +
> +class BinaryFiles(BzrError):
> +
> + _fmt = 'Binary files section encountered.'
> +
> + def __init__(self, comments, line):
> + self.comments = comments
> + self.line = line
> +
> +
> +class BinaryPatch(object):
> + def __init__(self, comments, line):
Why did this lose oldname and newname?
> + self.comments = comments
> + self.line = line
> + self.hunks = []
> +
> + def __str__(self):
> + return self.get_header()
> +
> + def get_header(self):
> + comments = ''.join(self.comments + [self.line])
> + return "%s" % comments
"%s" % comments is a no-op.
> +
> +
> +class Patch(object):
> + def __init__(self, comments, oldname, newname):
> + self.oldname = oldname
> + self.newname = newname
> + self.hunks = []
> + self.comments = comments
> +
> + def __str__(self):
> + ret = self.get_header()
> + ret += "".join([str(h) for h in self.hunks])
> + return ret
> +
> + def get_header(self):
> + comments = ''.join(self.comments)
> + return "%s--- %s\n+++ %s\n" % (comments, self.oldname, self.newname)
> +
> +
> +def get_patch_names(iter_lines):
> + comments = []
> + line = iter_lines.next()
> + while ((line == "\n") or line.startswith('=== ')
> + or line.startswith('*** ') or line.startswith('#')):
I think this would make more sense as a for loop with a break.
> + comments.append(line)
> + line = iter_lines.next()
This will crash with StopIteration if there are no non-comment lines left. Previously the function would always raise MalformedPatchHeader if it couldn't find a patch.
> + try:
> + match = re.match(binary_files_re, line)
Can you reuse the precompiled regex that you defined for iter_file_patch?
> + if match is not None:
> + match_line = line
We don't write to match_line or line after this, so why create match_line at all?
> + try:
> + keeper = line
> + next_line = iter_lines.next()
Why do we try to consume the next line of a binary patch, but not of a text patch? This at least deserves a comment if it can't be moved somewhere more consistent.
> + if next_line == '\n':
What if it isn't?
> + match_line += next_line
Is match_line now in fact multiple lines in one?
> + line = match_line
Why are we setting line here? We raise BinaryFiles in three lines, and that doesn't use line.
> + else:
> + line = keeper
keeper was just set to line 6 lines earlier! Should keeper be a thing at all?
> + except:
Bare excepts aren't a thing.
> + pass
> + raise BinaryFiles(comments, match_line)
match_line is multiple lines, so BinaryPatch.line will be multiple lines. That seems suboptimal.
> + if not line.startswith("--- "):
> + raise MalformedPatchHeader("No orig name", line)
> + else:
> + orig_name = line[4:].rstrip("\n")
> + except StopIteration:
> + raise MalformedPatchHeader("No orig line", "")
> + try:
> + line = iter_lines.next()
> + if not line.startswith("+++ "):
> + raise PatchSyntax("No mod name")
> + else:
> + mod_name = line[4:].rstrip("\n")
> + except StopIteration:
> + raise MalformedPatchHeader("No mod line", "")
> + return (comments, orig_name, mod_name)
> +
> +
> +def iter_hunks(iter_lines, allow_dirty=False):
> + '''
> + :arg iter_lines: iterable of lines to parse for hunks
> + :kwarg allow_dirty: If True, when we encounter something that is not
> + a hunk header when we're looking for one, assume the rest of the lines
> + are not part of the patch (comments or other junk). Default False
> + '''
> + hunk = None
> + line_no = 0
> + for line in iter_lines:
> + line_no += 1 # 1 based index
Why is line_no here?
> + if line == "\n":
> + if hunk is not None:
> + hunk.lines.append(line)
> + yield hunk
> + hunk = None
> + continue
> + if hunk is not None:
> + yield hunk
> + try:
> + hunk = hunk_from_header(line)
> + except MalformedHunkHeader:
> + if allow_dirty:
> + # If the line isn't a hunk header, then we've reached the end
> + # of this patch and there's "junk" at the end. Ignore the
> + # rest of this patch.
> + return
> + raise
> + orig_size = 0
> + mod_size = 0
> + while orig_size < hunk.orig_range or mod_size < hunk.mod_range:
> + hunk_line = parse_line(iter_lines.next())
> + hunk.lines.append(hunk_line)
> + if isinstance(hunk_line, (RemoveLine, ContextLine)):
> + orig_size += 1
> + if isinstance(hunk_line, (InsertLine, ContextLine)):
> + mod_size += 1
> + if hunk is not None:
> + yield hunk
> +
> +
> +def iter_file_patch(iter_lines, allow_dirty=False):
> + '''
> + :arg iter_lines: iterable of lines to parse for patches
> + :kwarg allow_dirty: If True, allow comments and other non-patch text
> + before the first patch. Note that the algorithm here can only find
> + such text before any patches have been found. Comments after the
> + first patch are stripped away in iter_hunks() if it is also passed
> + allow_dirty=True. Default False.
> + '''
> + ### FIXME: Docstring is not quite true. We allow certain comments no
> + # matter what, If they startwith '===', '***', or '#' Someone should
> + # reexamine this logic and decide if we should include those in
> + # allow_dirty or restrict those to only being before the patch is found
> + # (as allow_dirty does).
> + saved_lines = []
> + keeper = []
> + binary = False
> + for line in iter_lines:
> + # if it's a === or binary it's a new patch, but we might have
> + # both together.
I don't understand what this means.
> + if line.startswith('\n') and binary:
> + saved_lines.append(line)
Won't this cause the line to be appended twice? The line is empty, so we'll hit the else: block below and append again.
> + binary = False
> + elif binary:
> + binary = False
Couldn't this be simplified by having an outer "if binary:" conditional?
> + if line.startswith('=== '):
> + # assume a new patch until we know differently
> + if len(saved_lines) > 0:
> + yield saved_lines
> + saved_lines = []
> + keeper.append(line)
What's the purpose of keeper? Can't you just append to saved_lines directly?
> + elif regex.match(line):
> + # check for preceeding '=== ' line
> + # a binary patch always yields the previous patch
Except that it only yields when saved_lines is empty.
> + if len(saved_lines) > 0:
> + saved_lines.append(line)
> + binary = True
> + else:
> + yield [line]
> + else:
> + saved_lines.append(line)
> + if len(keeper) > 0:
> + saved_lines += keeper
> + keeper = []
> + if len(saved_lines) > 0:
> + yield saved_lines
> +
> +
> +def parse_patch(iter_lines, allow_dirty=False):
> + '''
> + :arg iter_lines: iterable of lines to parse
> + :kwarg allow_dirty: If True, allow the patch to have trailing junk.
> + Default False
> + '''
> + iter_lines = iter_lines_handle_nl(iter_lines)
> + try:
> + (comments, orig_name, mod_name) = get_patch_names(iter_lines)
> + except BinaryFiles, e:
> + return BinaryPatch(e.comments, e.line)
> + else:
> + patch = Patch(comments, orig_name, mod_name)
> + patch.comments = comments
Didn't the constructor do this?
> + for hunk in iter_hunks(iter_lines, allow_dirty):
> + patch.hunks.append(hunk)
> + return patch
> +
> +
> +def parse_patches(iter_lines, allow_dirty=False):
> + '''
> + :arg iter_lines: iterable of lines to parse for patches
> + :kwarg allow_dirty: If True, allow text that's not part of the patch at
> + selected places. This includes comments before and after a patch
> + for instance. Default False.
> + '''
> + patches = []
> + for f in iter_file_patch(iter_lines, allow_dirty):
> + patch = parse_patch(f.__iter__(), allow_dirty)
> + patches.append(patch)
> +
> + return patches
>
> === added directory 'lib/lp/services/tests/data'
> === added file 'lib/lp/services/tests/data/diff-1'
> --- lib/lp/services/tests/data/diff-1 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/diff-1 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,30 @@
> +=== zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +--- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> ++++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +@@ -70,7 +70,7 @@
> + "jf.bc=trgVayvarPbzzragf&cerivrjqvss_vq=1",
> + zbpxvb.erdhrfgf[0].pbasvt.qngn);
> + zbpxvb.ynfg_erdhrfg = zbpxvb.erdhrfgf[0];
> +- ine abj = (arj Qngr).inyhrBs()
> ++ ine abj = (arj Qngr).inyhrBs();
> + choyvfurq_pbzzragf = [
> + {'yvar_ahzore': '2',
> + 'crefba': crefba_bow,
> +@@ -80,7 +80,7 @@
> + 'crefba': crefba_bow,
> + 'grkg': 'Guvf vf terng.',
> + 'qngr': (arj Qngr(abj - 12600000)),
> +- },
> ++ }
> + ];
> + zbpxvb.fhpprff({
> + erfcbafrGrkg: L.WFBA.fgevatvsl(choyvfurq_pbzzragf),
> +@@ -88,7 +88,7 @@
> +
> + // Choyvfurq pbzzrag vf qvfcynlrq.
> + ine svefg_pbzzragf = L.bar('#qvss-yvar-2').arkg().bar('qvi');
> +- ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq')
> ++ ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq');
> + L.Nffreg.nerRdhny(
> + 'Sbb One (anzr16) jebgr ba 2012-08-12:',
> + svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
>
> === added file 'lib/lp/services/tests/data/diff-2'
> --- lib/lp/services/tests/data/diff-2 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/diff-2 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,2 @@
> +=== nqqrq svyr 'jrohv/obql_ot.wct'
> +Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
>
> === added file 'lib/lp/services/tests/data/diff-3'
> --- lib/lp/services/tests/data/diff-3 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/diff-3 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,12 @@
> +=== zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +--- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 12:32:44 +0000
> ++++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 23:47:16 +0000
> +@@ -3,7 +3,7 @@
> + ine grfgf = L.anzrfcnpr('qngr.grfg');
> + grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> +
> +- ine abj = (arj Qngr).inyhrBs()
> ++ ine abj = (arj Qngr).inyhrBs();
> + grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> + anzr: 'grfg_nccebkvzngrqngr',
> +
>
> === added file 'lib/lp/services/tests/data/expected-1'
> --- lib/lp/services/tests/data/expected-1 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-1 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,35 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +> --- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +> @@ -70,7 +70,7 @@
> +> "jf.bc=trgVayvarPbzzragf&cerivrjqvss_vq=1",
> +> zbpxvb.erdhrfgf[0].pbasvt.qngn);
> +> zbpxvb.ynfg_erdhrfg = zbpxvb.erdhrfgf[0];
> +> - ine abj = (arj Qngr).inyhrBs()
> +> + ine abj = (arj Qngr).inyhrBs();
> +
> +Cool
> +
> +> choyvfurq_pbzzragf = [
> +> {'yvar_ahzore': '2',
> +> 'crefba': crefba_bow,
> +
> +> @@ -88,7 +88,7 @@
> +>
> +> // Choyvfurq pbzzrag vf qvfcynlrq.
> +> ine svefg_pbzzragf = L.bar('#qvss-yvar-2').arkg().bar('qvi');
> +> - ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq')
> +> + ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq');
> +
> +Are you sure?
> +
> +> L.Nffreg.nerRdhny(
> +> 'Sbb One (anzr16) jebgr ba 2012-08-12:',
> +> svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/expected-2'
> --- lib/lp/services/tests/data/expected-2 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-2 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,12 @@
> +
> +
> +Diff comments:
> +
> +> === nqqrq svyr 'jrohv/obql_ot.wct'
> +> Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
> +
> +Why are you adding this?
> +
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/expected-3'
> --- lib/lp/services/tests/data/expected-3 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-3 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,22 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +> --- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 23:47:16 +0000
> +> @@ -3,7 +3,7 @@
> +> ine grfgf = L.anzrfcnpr('qngr.grfg');
> +> grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> +>
> +> - ine abj = (arj Qngr).inyhrBs()
> +> + ine abj = (arj Qngr).inyhrBs();
> +
> +What is this?
> +
> +> grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> +> anzr: 'grfg_nccebkvzngrqngr',
> +>
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/expected-4'
> --- lib/lp/services/tests/data/expected-4 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-4 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,14 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'
> +> --- yvo/yc/pbqr/vagresnprf/qvss.cl 2009-10-01 13:25:12 +0000
> +
> +Is this from Planet Earth© ?
> +
> +> +++ yvo/yc/pbqr/vagresnprf/qvss.cl 2010-02-02 15:48:56 +0000
> +
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/expected-5'
> --- lib/lp/services/tests/data/expected-5 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-5 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,39 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +> --- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +
> +> @@ -80,7 +80,7 @@
> +> 'crefba': crefba_bow,
> +> 'grkg': 'Guvf vf terng.',
> +> 'qngr': (arj Qngr(abj - 12600000)),
> +> - },
> +> + }
> +
> +Why?
> +
> +> ];
> +> zbpxvb.fhpprff({
> +> erfcbafrGrkg: L.WFBA.fgevatvsl(choyvfurq_pbzzragf),
> +
> +> === zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +> --- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 23:47:16 +0000
> +> @@ -3,7 +3,7 @@
> +> ine grfgf = L.anzrfcnpr('qngr.grfg');
> +> grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> +>
> +> - ine abj = (arj Qngr).inyhrBs()
> +> + ine abj = (arj Qngr).inyhrBs();
> +
> +Good catch!
> +
> +> grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> +> anzr: 'grfg_nccebkvzngrqngr',
> +>
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/expected-6'
> --- lib/lp/services/tests/data/expected-6 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/expected-6 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,45 @@
> +
> +
> +Diff comments:
> +
> +> === zbqvsvrq svyr 'yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf'
> +> --- yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/pbqr/wninfpevcg/grfgf/grfg_oenapuzretrcebcbfny.vayvarpbzzragf.wf 2014-07-08 23:47:16 +0000
> +
> +> @@ -88,7 +88,7 @@
> +>
> +> // Choyvfurq pbzzrag vf qvfcynlrq.
> +> ine svefg_pbzzragf = L.bar('#qvss-yvar-2').arkg().bar('qvi');
> +> - ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq')
> +> + ine svefg = svefg_pbzzragf.bar('qvi:svefg-puvyq');
> +
> ++1
> +
> +> L.Nffreg.nerRdhny(
> +> 'Sbb One (anzr16) jebgr ba 2012-08-12:',
> +> svefg.bar('.obneqPbzzragQrgnvyf').trg('grkg'));
> +>
> +> === nqqrq svyr 'jrohv/obql_ot.wct'
> +> Binary files jrohv/obql_ot.wct1970-01-01 00:00:00 +0000 and jrohv/obql_ot.wct19702014-01-31 16:57:19 +0000 differ
> +
> +Do we need this?
> +
> +>
> +> === zbqvsvrq svyr 'yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf'
> +> --- yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 12:32:44 +0000
> +> +++ yvo/yc/ncc/wninfpevcg/grfgf/grfg_qngr.wf 2014-07-08 23:47:16 +0000
> +> @@ -3,7 +3,7 @@
> +> ine grfgf = L.anzrfcnpr('qngr.grfg');
> +> grfgf.fhvgr = arj L.Grfg.Fhvgr("qngr grfgf");
> +>
> +> - ine abj = (arj Qngr).inyhrBs()
> +> + ine abj = (arj Qngr).inyhrBs();
> +
> +Can we change this?
> +
> +> grfgf.fhvgr.nqq(arj L.Grfg.Pnfr({
> +> anzr: 'grfg_nccebkvzngrqngr',
> +>
> +
> +
> +--
>
> === added file 'lib/lp/services/tests/data/insert_top.patch'
> --- lib/lp/services/tests/data/insert_top.patch 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/insert_top.patch 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,7 @@
> +--- orig/pylon/patches.py
> ++++ mod/pylon/patches.py
> +@@ -1,3 +1,4 @@
> ++#test
> + import util
> + import sys
> + class PatchSyntax(Exception):
>
> === added file 'lib/lp/services/tests/data/tricky-diff.patch'
> --- lib/lp/services/tests/data/tricky-diff.patch 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/data/tricky-diff.patch 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,26 @@
> +=== orig-7
> +--- orig-7
> ++++ mod-7
> +@@ -1,10 +1,10 @@
> + -- a
> +--- b
> ++++ c
> + xx d
> + xx e
> + ++ f
> +-++ g
> ++-- h
> + xx i
> + xx j
> + -- k
> +--- l
> ++++ m
> +=== orig-8
> +--- orig-8
> ++++ mod-8
> +@@ -1 +1 @@
> +--- A
> ++++ B
> +@@ -1 +1 @@
> +--- C
> ++++ D
>
> === added file 'lib/lp/services/tests/test_patches.py'
> --- lib/lp/services/tests/test_patches.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/test_patches.py 2014-07-26 02:00:47 +0000
> @@ -0,0 +1,129 @@
> +# Copyright 2014 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import os
> +import re
> +
> +from bzrlib.errors import MalformedPatchHeader
> +from bzrlib.patches import difference_index
> +
> +from lp.services.patches import (
> + BinaryPatch,
> + get_patch_names,
> + parse_patch,
> + parse_patches,
> + Patch,
> + )
> +from lp.testing import TestCase
> +
> +
> +def datafile(filename):
> + data_path = os.path.join(os.path.dirname(__file__),
> + "data", filename)
> + return file(data_path, "rb")
> +
> +
> +class TestBzrLibCode(TestCase):
> + """Test code rewritten from bzrlib.patches for diff comment emails"""
> +
> + def assertContainsRe(self, haystack, needle_re, flags=0):
> + """Assert that a contains something matching a regular expression."""
> + if not re.search(needle_re, haystack, flags):
> + if '\n' in haystack or len(haystack) > 60:
> + # a long string, format it in a more readable way
> + raise AssertionError(
> + 'pattern "%s" not found in\n"""\\\n%s"""\n'
> + % (needle_re, haystack))
> + else:
> + raise AssertionError('pattern "%s" not found in "%s"'
> + % (needle_re, haystack))
> +
> + def data_lines(self, filename):
> + data = datafile(filename)
> + try:
> + return data.readlines()
> + finally:
> + data.close()
> +
> + def testValidPatchHeaderWithComments(self):
> + """Parse a valid patch header"""
> + expected_comments = ["=== modified file 'commands.py'"]
> + lines = ("=== modified file 'commands.py'\n--- orig/commands.py\n"
> + "+++ mod/dommands.py\n").split('\n')
> + (comments, orig, mod) = get_patch_names(lines.__iter__())
> + self.assertEqual(comments, expected_comments)
> + self.assertEqual(orig, "orig/commands.py")
> + self.assertEqual(mod, "mod/dommands.py")
> +
> + def testValidPatchHeader(self):
> + """Parse a valid patch header"""
> + expected_comments = []
> + lines = ("--- orig/commands.py\n+++ mod/dommands.py\n").split('\n')
> + (comments, orig, mod) = get_patch_names(lines.__iter__())
> + self.assertEqual(comments, expected_comments)
> + self.assertEqual(orig, "orig/commands.py")
> + self.assertEqual(mod, "mod/dommands.py")
> +
> + def testInvalidPatchHeader(self):
> + """Parse an invalid patch header"""
> + lines = "-- orig/commands.py\n+++ mod/dommands.py".split('\n')
> + self.assertRaises(MalformedPatchHeader, get_patch_names,
> + lines.__iter__())
> +
> + def compare_parsed(self, patchtext):
> + lines = patchtext.splitlines(True)
> + patch = parse_patch(lines.__iter__())
> + pstr = str(patch)
> + i = difference_index(patchtext, pstr)
> + if i is not None:
> + print "%i: \"%s\" != \"%s\"" % (i, patchtext[i], pstr[i])
> + self.assertEqual(patchtext, str(patch))
> +
> + def testAll(self):
> + """Test parsing a whole patch"""
> + patchtext = datafile("diff-1").read()
> + self.compare_parsed(patchtext)
> +
> + def test_parse_binary(self):
> + """Test parsing a whole patch"""
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff = '\n'.join([diff_2, diff_1]).splitlines(True)
> + patches = parse_patches(diff)
> + self.assertIs(BinaryPatch, patches[0].__class__)
> + self.assertIs(Patch, patches[1].__class__)
> + self.assertContainsRe(
> + str(patches[0]),
> + 'Binary files jrohv/obql_ot.wct1970-01-01.* and '
> + 'jrohv/obql_ot.wct19702014-01-31.* differ\n')
> +
> + def test_parse_binary_after_normal(self):
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff = '\n'.join([diff_1, diff_2]).splitlines(True)
> + patches = parse_patches(diff)
> + self.assertIs(BinaryPatch, patches[1].__class__)
> + self.assertIs(Patch, patches[0].__class__)
> + self.assertContainsRe(
> + str(patches[1]),
> + 'Binary files jrohv/obql_ot.wct1970-01-01.* and '
> + 'jrohv/obql_ot.wct19702014-01-31.* differ\n')
> +
> + def test_roundtrip_binary(self):
> + diff_1 = datafile("diff-1").read()
> + diff_2 = datafile("diff-2").read()
> + diff = '\n'.join([diff_2, diff_1])
> + patches = parse_patches(diff.splitlines(True))
> + self.assertEqual(diff, ''.join(str(p) for p in patches))
> +
> + def testParsePatchesWithComments(self):
> + """Make sure file names can be extracted from tricky unified diffs
> + with comment lines"""
> + patchtext = ''.join(self.data_lines("tricky-diff.patch"))
> + filenames = [('orig-7', 'mod-7'),
> + ('orig-8', 'mod-8')]
> + patches = parse_patches(patchtext.splitlines(True))
> + patch_files = []
> + for patch in patches:
> + patch_files.append((patch.oldname, patch.newname))
> + self.assertEqual(patch_files, filenames)
>
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py 2014-07-18 07:43:06 +0000
> +++ lib/lp/testing/factory.py 2014-07-26 02:00:47 +0000
> @@ -1540,8 +1540,11 @@
> Diff.fromFile(StringIO(diff_text), len(diff_text)))
>
> def makePreviewDiff(self, conflicts=u'', merge_proposal=None,
> - date_created=None):
> - diff = self.makeDiff()
> + date_created=None, diff_text=None):
> + if diff_text is None:
> + diff = self.makeDiff()
> + else:
> + diff = self.makeDiff(diff_text)
> if merge_proposal is None:
> merge_proposal = self.makeBranchMergeProposal()
> preview_diff = PreviewDiff()
>
--
https://code.launchpad.net/~cjohnston/launchpad/short-ic-emails/+merge/225095
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
