From 97366bef6c1984357f225ab89a8d1044e56c7916 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 3 Jun 2011 20:02:46 +0000 Subject: [PATCH] Increase coverage to 91%; Much stricter about header parsing. Add is_new to be used in a later patch by checkout classes. R=dpranke@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/7054048 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@87838 0039d316-1c4b-4281-b951-d872f2087c98 --- patch.py | 170 +++++++++++++++++++++++++++-------------- rietveld.py | 3 +- tests/checkout_test.py | 3 +- tests/patch_test.py | 145 ++++++++++++++++++++++++++++++----- 4 files changed, 246 insertions(+), 75 deletions(-) diff --git a/patch.py b/patch.py index c627f48c8..9c54924df 100644 --- a/patch.py +++ b/patch.py @@ -29,6 +29,7 @@ class FilePatchBase(object): """ is_delete = False is_binary = False + is_new = False def __init__(self, filename): self.filename = None @@ -44,7 +45,7 @@ class FilePatchBase(object): if self.filename.startswith(i): self._fail('Filename can\'t start with \'%s\'.' % i) - def get(self): + def get(self): # pragma: no coverage raise NotImplementedError('Nothing to grab') def set_relpath(self, relpath): @@ -75,10 +76,11 @@ class FilePatchBinary(FilePatchBase): """Content of a new binary file.""" is_binary = True - def __init__(self, filename, data, svn_properties): + def __init__(self, filename, data, svn_properties, is_new): super(FilePatchBinary, self).__init__(filename) self.data = data self.svn_properties = svn_properties or [] + self.is_new = is_new def get(self): return self.data @@ -195,52 +197,86 @@ class FilePatchDiff(FilePatchBase): if not old or not new: self._fail('Unexpected git diff; couldn\'t find git header.') - # Handle these: - # new file mode \d{6} - # rename from <> - # rename to <> - # copy from <> - # copy to <> + last_line = '' + while lines: - if lines[0].startswith('--- '): - break line = lines.pop(0) - match = re.match(r'^(rename|copy) from (.+)$', line) - if match: - if old != match.group(2): - self._fail('Unexpected git diff input name for %s.' % match.group(1)) - if not lines: - self._fail('Missing git diff output name for %s.' % match.group(1)) - match = re.match(r'^(rename|copy) to (.+)$', lines.pop(0)) - if not match: - self._fail('Missing git diff output name for %s.' % match.group(1)) - if new != match.group(2): - self._fail('Unexpected git diff output name for %s.' % match.group(1)) - continue + # TODO(maruel): old should be replace with self.source_file + # TODO(maruel): new == self.filename and remove new + self._verify_git_header_process_line(lines, line, last_line, old, new) + last_line = line - match = re.match(r'^new file mode (\d{6})$', line) - if match: - mode = match.group(1) - # Only look at owner ACL for executable. - if bool(int(mode[4]) & 4): - self.svn_properties.append(('svn:executable', '*')) + # Cheap check to make sure the file name is at least mentioned in the + # 'diff' header. That the only remaining invariant. + if not self.filename in self.diff_header: + self._fail('Diff seems corrupted.') - # Handle ---/+++ - while lines: - match = re.match(r'^--- (.*)$', lines.pop(0)) - if not match: - continue + def _verify_git_header_process_line(self, lines, line, last_line, old, new): + """Processes a single line of the header. + + Returns True if it should continue looping. + """ + # Handle these: + # rename from <> + # copy from <> + match = re.match(r'^(rename|copy) from (.+)$', line) + if match: + if old != match.group(2): + self._fail('Unexpected git diff input name for line %s.' % line) + if not lines or not lines[0].startswith('%s to ' % match.group(1)): + self._fail( + 'Confused %s from/to git diff for line %s.' % + (match.group(1), line)) + return + + # Handle these: + # rename to <> + # copy to <> + match = re.match(r'^(rename|copy) to (.+)$', line) + if match: + if new != match.group(2): + self._fail('Unexpected git diff output name for line %s.' % line) + if not last_line.startswith('%s from ' % match.group(1)): + self._fail( + 'Confused %s from/to git diff for line %s.' % + (match.group(1), line)) + return + + # Handle "new file mode \d{6}" + match = re.match(r'^new file mode (\d{6})$', line) + if match: + mode = match.group(1) + # Only look at owner ACL for executable. + if bool(int(mode[4]) & 4): + self.svn_properties.append(('svn:executable', '*')) + + # Handle "--- " + match = re.match(r'^--- (.*)$', line) + if match: + if last_line[:3] in ('---', '+++'): + self._fail('--- and +++ are reversed') + self.is_new = match.group(1) == '/dev/null' + # TODO(maruel): Use self.source_file. if old != self.mangle(match.group(1)) and match.group(1) != '/dev/null': self._fail('Unexpected git diff: %s != %s.' % (old, match.group(1))) - if not lines: + if not lines or not lines[0].startswith('+++'): self._fail('Missing git diff output name.') - match = re.match(r'^\+\+\+ (.*)$', lines.pop(0)) - if not match: + return + + # Handle "+++ " + match = re.match(r'^\+\+\+ (.*)$', line) + if match: + if not last_line.startswith('---'): self._fail('Unexpected git diff: --- not following +++.') + # TODO(maruel): new == self.filename. if new != self.mangle(match.group(1)) and '/dev/null' != match.group(1): + # TODO(maruel): Can +++ be /dev/null? If so, assert self.is_delete == + # True. self._fail('Unexpected git diff: %s != %s.' % (new, match.group(1))) - assert not lines, '_split_header() is broken' - break + if lines: + self._fail('Crap after +++') + # We're done. + return def _verify_svn_header(self): """Sanity checks the header. @@ -250,30 +286,52 @@ class FilePatchDiff(FilePatchBase): localized. """ lines = self.diff_header.splitlines() + last_line = '' + while lines: - match = re.match(r'^--- ([^\t]+).*$', lines.pop(0)) - if not match: - continue - # For copy and renames, it's possible that the -- line doesn't match +++, - # so don't check match.group(1) to match self.filename or '/dev/null', it - # can be anything else. + line = lines.pop(0) + self._verify_svn_header_process_line(lines, line, last_line) + last_line = line + + # Cheap check to make sure the file name is at least mentioned in the + # 'diff' header. That the only remaining invariant. + if not self.filename in self.diff_header: + self._fail('Diff seems corrupted.') + + def _verify_svn_header_process_line(self, lines, line, last_line): + """Processes a single line of the header. + + Returns True if it should continue looping. + """ + match = re.match(r'^--- ([^\t]+).*$', line) + if match: + if last_line[:3] in ('---', '+++'): + self._fail('--- and +++ are reversed') + self.is_new = match.group(1) == '/dev/null' + # For copy and renames, it's possible that the -- line doesn't match + # +++, so don't check match.group(1) to match self.filename or + # '/dev/null', it can be anything else. # TODO(maruel): Handle rename/copy explicitly. - # if match.group(1) not in (self.filename, '/dev/null'): + # if (self.mangle(match.group(1)) != self.filename and + # match.group(1) != '/dev/null'): # self.source_file = match.group(1) - if not lines: + if not lines or not lines[0].startswith('+++'): self._fail('Nothing after header.') - match = re.match(r'^\+\+\+ ([^\t]+).*$', lines.pop(0)) - if not match: + return + + match = re.match(r'^\+\+\+ ([^\t]+).*$', line) + if match: + if not last_line.startswith('---'): self._fail('Unexpected diff: --- not following +++.') - if match.group(1) not in (self.filename, '/dev/null'): + if (self.mangle(match.group(1)) != self.filename and + match.group(1) != '/dev/null'): + # TODO(maruel): Can +++ be /dev/null? If so, assert self.is_delete == + # True. self._fail('Unexpected diff: %s.' % match.group(1)) - assert not lines, '_split_header() is broken' - break - else: - # Cheap check to make sure the file name is at least mentioned in the - # 'diff' header. That the only remaining invariant. - if not self.filename in self.diff_header: - self._fail('Diff seems corrupted.') + if lines: + self._fail('Crap after +++') + # We're done. + return class PatchSet(object): diff --git a/rietveld.py b/rietveld.py index b3895d639..40be3c0ee 100644 --- a/rietveld.py +++ b/rietveld.py @@ -133,7 +133,8 @@ class Rietveld(object): out.append(patch.FilePatchBinary( filename, self.get_file_content(issue, patchset, state['id']), - props)) + props, + is_new=(status[0] == 'A'))) else: if state['num_chunks']: diff = self.get_file_diff(issue, patchset, state['id']) diff --git a/tests/checkout_test.py b/tests/checkout_test.py index 5707c7be6..59a3f529a 100755 --- a/tests/checkout_test.py +++ b/tests/checkout_test.py @@ -151,7 +151,8 @@ class BaseTest(fake_repos.FakeReposTestBase): return patch.PatchSet([ patch.FilePatchDiff( 'svn_utils_test.txt', GIT_PATCH, []), - patch.FilePatchBinary('bin_file', '\x00', []), + # TODO(maruel): Test with is_new == False. + patch.FilePatchBinary('bin_file', '\x00', [], is_new=True), patch.FilePatchDelete('extra', False), patch.FilePatchDiff('new_dir/subdir/new_file', PATCH_ADD, []), ]) diff --git a/tests/patch_test.py b/tests/patch_test.py index 4ca45e7e5..dd4b72efe 100755 --- a/tests/patch_test.py +++ b/tests/patch_test.py @@ -5,6 +5,7 @@ """Unit tests for patch.py.""" +import logging import os import sys import unittest @@ -125,21 +126,30 @@ GIT_NEW = ( '+bar\n') +NEW = ( + '--- /dev/null\n' + '+++ foo\n' + '@@ -0,0 +1 @@\n' + '+bar\n') + + class PatchTest(unittest.TestCase): def testFilePatchDelete(self): c = patch.FilePatchDelete('foo', False) - self.assertEquals(c.is_delete, True) - self.assertEquals(c.is_binary, False) self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, True) + self.assertEquals(c.is_new, False) try: c.get() self.fail() except NotImplementedError: pass c = patch.FilePatchDelete('foo', True) - self.assertEquals(c.is_delete, True) - self.assertEquals(c.is_binary, True) self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_binary, True) + self.assertEquals(c.is_delete, True) + self.assertEquals(c.is_new, False) try: c.get() self.fail() @@ -147,59 +157,97 @@ class PatchTest(unittest.TestCase): pass def testFilePatchBinary(self): - c = patch.FilePatchBinary('foo', 'data', []) - self.assertEquals(c.is_delete, False) + c = patch.FilePatchBinary('foo', 'data', [], is_new=False) + self.assertEquals(c.filename, 'foo') self.assertEquals(c.is_binary, True) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_new, False) + self.assertEquals(c.get(), 'data') + + def testFilePatchBinaryNew(self): + c = patch.FilePatchBinary('foo', 'data', [], is_new=True) self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_binary, True) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_new, True) self.assertEquals(c.get(), 'data') def testFilePatchDiff(self): c = patch.FilePatchDiff('chrome/file.cc', SVN_PATCH, []) - self.assertEquals(c.is_delete, False) - self.assertEquals(c.is_binary, False) self.assertEquals(c.filename, 'chrome/file.cc') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, False) self.assertEquals(c.is_git_diff, False) + self.assertEquals(c.is_new, False) self.assertEquals(c.patchlevel, 0) self.assertEquals(c.get(), SVN_PATCH) + + def testFilePatchDiffHeaderMode(self): diff = ( 'diff --git a/git_cl/git-cl b/git_cl/git-cl\n' 'old mode 100644\n' 'new mode 100755\n') c = patch.FilePatchDiff('git_cl/git-cl', diff, []) - self.assertEquals(c.is_delete, False) - self.assertEquals(c.is_binary, False) self.assertEquals(c.filename, 'git_cl/git-cl') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, False) self.assertEquals(c.is_git_diff, True) + self.assertEquals(c.is_new, False) self.assertEquals(c.patchlevel, 1) self.assertEquals(c.get(), diff) + + def testFilePatchDiffHeaderModeIndex(self): diff = ( 'Index: Junk\n' 'diff --git a/git_cl/git-cl b/git_cl/git-cl\n' 'old mode 100644\n' 'new mode 100755\n') c = patch.FilePatchDiff('git_cl/git-cl', diff, []) - self.assertEquals(c.is_delete, False) - self.assertEquals(c.is_binary, False) self.assertEquals(c.filename, 'git_cl/git-cl') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, False) self.assertEquals(c.is_git_diff, True) + self.assertEquals(c.is_new, False) self.assertEquals(c.patchlevel, 1) self.assertEquals(c.get(), diff) - def testFilePatchBadDiff(self): + def testFilePatchDiffSvnNew(self): + # The code path is different for git and svn. + c = patch.FilePatchDiff('foo', NEW, []) + self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_git_diff, False) + self.assertEquals(c.is_new, True) + self.assertEquals(c.patchlevel, 0) + self.assertEquals(c.get(), NEW) + + def testFilePatchDiffGitNew(self): + # The code path is different for git and svn. + c = patch.FilePatchDiff('foo', GIT_NEW, []) + self.assertEquals(c.filename, 'foo') + self.assertEquals(c.is_binary, False) + self.assertEquals(c.is_delete, False) + self.assertEquals(c.is_git_diff, True) + self.assertEquals(c.is_new, True) + self.assertEquals(c.patchlevel, 1) + self.assertEquals(c.get(), GIT_NEW) + + def testFilePatchDiffBad(self): try: patch.FilePatchDiff('foo', 'data', []) self.fail() except patch.UnsupportedPatchFormat: pass - def testFilePatchNoDiff(self): + def testFilePatchDiffEmpty(self): try: patch.FilePatchDiff('foo', '', []) self.fail() except patch.UnsupportedPatchFormat: pass - def testFilePatchNoneDiff(self): + def testFilePatchDiffNone(self): try: patch.FilePatchDiff('foo', None, []) self.fail() @@ -210,10 +258,60 @@ class PatchTest(unittest.TestCase): try: patch.FilePatchDiff('foo', SVN_PATCH, []) self.fail() + except patch.UnsupportedPatchFormat, e: + self.assertEquals( + "Can't process patch for file foo.\nUnexpected diff: chrome/file.cc.", + str(e)) + + def testFilePatchDiffBadHeader(self): + try: + diff = ( + '+++ b/foo\n' + '@@ -0,0 +1 @@\n' + '+bar\n') + patch.FilePatchDiff('foo', diff, []) + self.fail() except patch.UnsupportedPatchFormat: pass - def testInvalidFilePatchDiffGit(self): + def testFilePatchDiffBadGitHeader(self): + try: + diff = ( + 'diff --git a/foo b/foo\n' + '+++ b/foo\n' + '@@ -0,0 +1 @@\n' + '+bar\n') + patch.FilePatchDiff('foo', diff, []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + + def testFilePatchDiffBadHeaderReversed(self): + try: + diff = ( + '+++ b/foo\n' + '--- b/foo\n' + '@@ -0,0 +1 @@\n' + '+bar\n') + patch.FilePatchDiff('foo', diff, []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + + def testFilePatchDiffGitBadHeaderReversed(self): + try: + diff = ( + 'diff --git a/foo b/foo\n' + '+++ b/foo\n' + '--- b/foo\n' + '@@ -0,0 +1 @@\n' + '+bar\n') + patch.FilePatchDiff('foo', diff, []) + self.fail() + except patch.UnsupportedPatchFormat: + pass + + def testFilePatchDiffInvalidGit(self): try: patch.FilePatchDiff('svn_utils_test.txt', ( 'diff --git a/tests/svn_utils_test_data/svn_utils_test.txt ' @@ -291,7 +389,7 @@ class PatchTest(unittest.TestCase): patch.FilePatchDiff('pp', GIT_COPY, []), patch.FilePatchDiff('foo', GIT_NEW, []), patch.FilePatchDelete('other/place/foo', True), - patch.FilePatchBinary('bar', 'data', []), + patch.FilePatchBinary('bar', 'data', [], is_new=False), ]) expected = [ 'chrome/file.cc', 'tools/clang_check/README.chromium', @@ -326,6 +424,16 @@ class PatchTest(unittest.TestCase): except patch.UnsupportedPatchFormat: pass + def testRelPathEmpty(self): + patches = patch.PatchSet([ + patch.FilePatchDiff('chrome\\file.cc', SVN_PATCH, []), + patch.FilePatchDelete('other\\place\\foo', True), + ]) + patches.set_relpath('') + self.assertEquals( + ['chrome/file.cc', 'other/place/foo'], + [f.filename for f in patches]) + def testBackSlash(self): mangled_patch = SVN_PATCH.replace('chrome/', 'chrome\\') patches = patch.PatchSet([ @@ -407,4 +515,7 @@ class PatchTest(unittest.TestCase): if __name__ == '__main__': + logging.basicConfig(level= + [logging.WARNING, logging.INFO, logging.DEBUG][ + min(2, sys.argv.count('-v'))]) unittest.main()