diff --git a/patch.py b/patch.py index 333cbb9f6..888e10095 100644 --- a/patch.py +++ b/patch.py @@ -33,6 +33,8 @@ class FilePatchBase(object): def __init__(self, filename): self.filename = self._process_filename(filename) + # Set when the file is copied or moved. + self.source_filename = None @staticmethod def _process_filename(filename): @@ -59,6 +61,9 @@ class FilePatchBase(object): self._fail('Relative path starts with %s' % relpath[0]) self.filename = self._process_filename( posixpath.join(relpath, self.filename)) + if self.source_filename: + self.source_filename = self._process_filename( + posixpath.join(relpath, self.source_filename)) def _fail(self, msg): """Shortcut function to raise UnsupportedPatchFormat.""" @@ -112,9 +117,21 @@ class FilePatchDiff(FilePatchBase): def set_relpath(self, relpath): old_filename = self.filename + old_source_filename = self.source_filename or self.filename super(FilePatchDiff, self).set_relpath(relpath) # Update the header too. - self.diff_header = self.diff_header.replace(old_filename, self.filename) + source_filename = self.source_filename or self.filename + lines = self.diff_header.splitlines(True) + for i, line in enumerate(lines): + if line.startswith('diff --git'): + lines[i] = line.replace( + 'a/' + old_source_filename, source_filename).replace( + 'b/' + old_filename, self.filename) + elif re.match(r'^\w+ from .+$', line) or line.startswith('---'): + lines[i] = line.replace(old_source_filename, source_filename) + elif re.match(r'^\w+ to .+$', line) or line.startswith('+++'): + lines[i] = line.replace(old_filename, self.filename) + self.diff_header = ''.join(lines) def _split_header(self, diff): """Splits a diff in two: the header and the hunks.""" @@ -186,12 +203,11 @@ class FilePatchDiff(FilePatchBase): match = re.match(r'^diff \-\-git (.*?) (.*)$', lines.pop(0)) if not match: continue - old = match.group(1).replace('\\', '/') - new = match.group(2).replace('\\', '/') - if old.startswith('a/') and new.startswith('b/'): + if match.group(1).startswith('a/') and match.group(2).startswith('b/'): self.patchlevel = 1 - old = old[2:] - new = new[2:] + old = self.mangle(match.group(1)) + new = self.mangle(match.group(2)) + # The rename is about the new file so the old file can be anything. if new not in (self.filename, 'dev/null'): self._fail('Unexpected git diff output name %s.' % new) @@ -202,13 +218,15 @@ class FilePatchDiff(FilePatchBase): if not old or not new: self._fail('Unexpected git diff; couldn\'t find git header.') + if old not in (self.filename, 'dev/null'): + # Copy or rename. + self.source_filename = old + last_line = '' while lines: line = lines.pop(0) - # 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) + self._verify_git_header_process_line(lines, line, last_line) last_line = line # Cheap check to make sure the file name is at least mentioned in the @@ -216,7 +234,7 @@ class FilePatchDiff(FilePatchBase): if not self.filename in self.diff_header: self._fail('Diff seems corrupted.') - def _verify_git_header_process_line(self, lines, line, last_line, old, new): + def _verify_git_header_process_line(self, lines, line, last_line): """Processes a single line of the header. Returns True if it should continue looping. @@ -225,6 +243,7 @@ class FilePatchDiff(FilePatchBase): http://www.kernel.org/pub/software/scm/git/docs/git-diff.html """ match = re.match(r'^(rename|copy) from (.+)$', line) + old = self.source_filename or self.filename if match: if old != match.group(2): self._fail('Unexpected git diff input name for line %s.' % line) @@ -236,7 +255,7 @@ class FilePatchDiff(FilePatchBase): match = re.match(r'^(rename|copy) to (.+)$', line) if match: - if new != match.group(2): + if self.filename != 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( @@ -258,7 +277,7 @@ class FilePatchDiff(FilePatchBase): 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': + if self.mangle(match.group(1)) not in (old, 'dev/null'): self._fail('Unexpected git diff: %s != %s.' % (old, match.group(1))) if not lines or not lines[0].startswith('+++'): self._fail('Missing git diff output name.') @@ -271,8 +290,9 @@ class FilePatchDiff(FilePatchBase): # TODO(maruel): new == self.filename. if '/dev/null' == match.group(1): self.is_delete = True - elif new != self.mangle(match.group(1)): - self._fail('Unexpected git diff: %s != %s.' % (new, match.group(1))) + elif self.filename != self.mangle(match.group(1)): + self._fail( + 'Unexpected git diff: %s != %s.' % (self.filename, match.group(1))) if lines: self._fail('Crap after +++') # We're done. @@ -308,13 +328,9 @@ class FilePatchDiff(FilePatchBase): 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 (self.mangle(match.group(1)) != self.filename and - # match.group(1) != '/dev/null'): - # self.source_file = match.group(1) + if (self.mangle(match.group(1)) != self.filename and + match.group(1) != '/dev/null'): + self.source_filename = match.group(1) if not lines or not lines[0].startswith('+++'): self._fail('Nothing after header.') return diff --git a/rietveld.py b/rietveld.py index 2320f3d2b..6f4f4ae7f 100644 --- a/rietveld.py +++ b/rietveld.py @@ -155,10 +155,14 @@ class Rietveld(object): # Support this use case if it ever happen. raise patch.UnsupportedPatchFormat( filename, 'Empty diff is not supported yet.\n') - out.append(patch.FilePatchDiff(filename, diff, svn_props)) + p = patch.FilePatchDiff(filename, diff, svn_props) + out.append(p) if status[0] == 'A': # It won't be set for empty file. - out[-1].is_new = True + p.is_new = True + if status[1] == '+' and not (p.source_filename or p.svn_properties): + raise patch.UnsupportedPatchFormat( + filename, 'Failed to process the svn properties') else: raise patch.UnsupportedPatchFormat( filename, 'Change with status \'%s\' is not supported.' % status) diff --git a/tests/patch_test.py b/tests/patch_test.py index d53e89345..5f6562a00 100755 --- a/tests/patch_test.py +++ b/tests/patch_test.py @@ -74,16 +74,16 @@ GIT_DELETE = ( # http://codereview.chromium.org/download/issue6250123_3013_6010.diff GIT_RENAME_PARTIAL = ( - 'Index: chrome/browser/chromeos/views/webui_menu_widget.h\n' - 'diff --git a/chrome/browser/chromeos/views/domui_menu_widget.h ' - 'b/chrome/browser/chromeos/views/webui_menu_widget.h\n' + 'Index: chromeos/views/webui_menu_widget.h\n' + 'diff --git a/chromeos/views/DOMui_menu_widget.h ' + 'b/chromeos/views/webui_menu_widget.h\n' 'similarity index 79%\n' - 'rename from chrome/browser/chromeos/views/domui_menu_widget.h\n' - 'rename to chrome/browser/chromeos/views/webui_menu_widget.h\n' + 'rename from chromeos/views/DOMui_menu_widget.h\n' + 'rename to chromeos/views/webui_menu_widget.h\n' 'index 095d4c474fd9718f5aebfa41a1ccb2d951356d41..' '157925075434b590e8acaaf605a64f24978ba08b 100644\n' - '--- a/chrome/browser/chromeos/views/domui_menu_widget.h\n' - '+++ b/chrome/browser/chromeos/views/webui_menu_widget.h\n' + '--- a/chromeos/views/DOMui_menu_widget.h\n' + '+++ b/chromeos/views/webui_menu_widget.h\n' '@@ -1,9 +1,9 @@\n' '-// Copyright (c) 2010 The Chromium Authors. All rights reserved.\n' '+// Copyright (c) 2011 The Chromium Authors. All rights reserved.\n' @@ -103,9 +103,9 @@ GIT_RENAME_PARTIAL = ( # http://codereview.chromium.org/download/issue6287022_3001_4010.diff GIT_RENAME = ( 'Index: tools/run_local_server.sh\n' - 'diff --git a/tools/run_local_server.py b/tools/run_local_server.sh\n' + 'diff --git a/tools/run_local_server.PY b/tools/run_local_server.sh\n' 'similarity index 100%\n' - 'rename from tools/run_local_server.py\n' + 'rename from tools/run_local_server.PY\n' 'rename to tools/run_local_server.sh\n') @@ -145,6 +145,7 @@ class PatchTest(unittest.TestCase): p, filename, diff, + source_filename=None, is_binary=False, is_delete=False, is_git_diff=False, @@ -153,6 +154,7 @@ class PatchTest(unittest.TestCase): svn_properties=None): svn_properties = svn_properties or [] self.assertEquals(p.filename, filename) + self.assertEquals(p.source_filename, source_filename) self.assertEquals(p.is_binary, is_binary) self.assertEquals(p.is_delete, is_delete) if hasattr(p, 'is_git_diff'): @@ -378,8 +380,7 @@ class PatchTest(unittest.TestCase): 'tools\\clang_check/README.chromium', GIT_DELETE, []), patch.FilePatchDiff('tools/run_local_server.sh', GIT_RENAME, []), patch.FilePatchDiff( - 'chrome\\browser/chromeos/views/webui_menu_widget.h', - GIT_RENAME_PARTIAL, []), + 'chromeos\\views/webui_menu_widget.h', GIT_RENAME_PARTIAL, []), patch.FilePatchDiff('pp', GIT_COPY, []), patch.FilePatchDiff('foo', GIT_NEW, []), patch.FilePatchDelete('other/place/foo', True), @@ -388,20 +389,24 @@ class PatchTest(unittest.TestCase): expected = [ 'chrome/file.cc', 'tools/clang_check/README.chromium', 'tools/run_local_server.sh', - 'chrome/browser/chromeos/views/webui_menu_widget.h', 'pp', 'foo', + 'chromeos/views/webui_menu_widget.h', 'pp', 'foo', 'other/place/foo', 'bar'] self.assertEquals(expected, patches.filenames) orig_name = patches.patches[0].filename + orig_source_name = patches.patches[0].source_filename or orig_name patches.set_relpath(os.path.join('a', 'bb')) expected = [os.path.join('a', 'bb', x) for x in expected] self.assertEquals(expected, patches.filenames) # Make sure each header is updated accordingly. header = [] new_name = os.path.join('a', 'bb', orig_name) + new_source_name = os.path.join('a', 'bb', orig_source_name) for line in SVN_PATCH.splitlines(True): if line.startswith('@@'): break - if line[:3] in ('---', '+++', 'Ind'): + if line[:3] == '---': + line = line.replace(orig_source_name, new_source_name) + if line[:3] == '+++': line = line.replace(orig_name, new_name) header.append(line) header = ''.join(header) @@ -427,6 +432,7 @@ class PatchTest(unittest.TestCase): self.assertEquals( ['chrome/file.cc', 'other/place/foo'], [f.filename for f in patches]) + self.assertEquals([None, None], [f.source_filename for f in patches]) def testBackSlash(self): mangled_patch = SVN_PATCH.replace('chrome/', 'chrome\\') @@ -452,19 +458,21 @@ class PatchTest(unittest.TestCase): def testGitRename(self): p = patch.FilePatchDiff('tools/run_local_server.sh', GIT_RENAME, []) self._check_patch(p, 'tools/run_local_server.sh', GIT_RENAME, - is_git_diff=True, patchlevel=1) + is_git_diff=True, patchlevel=1, + source_filename='tools/run_local_server.PY') def testGitRenamePartial(self): p = patch.FilePatchDiff( - 'chrome/browser/chromeos/views/webui_menu_widget.h', - GIT_RENAME_PARTIAL, []) + 'chromeos/views/webui_menu_widget.h', GIT_RENAME_PARTIAL, []) self._check_patch( - p, 'chrome/browser/chromeos/views/webui_menu_widget.h', - GIT_RENAME_PARTIAL, is_git_diff=True, patchlevel=1) + p, 'chromeos/views/webui_menu_widget.h', GIT_RENAME_PARTIAL, + source_filename='chromeos/views/DOMui_menu_widget.h', is_git_diff=True, + patchlevel=1) def testGitCopy(self): p = patch.FilePatchDiff('pp', GIT_COPY, []) - self._check_patch(p, 'pp', GIT_COPY, is_git_diff=True, patchlevel=1) + self._check_patch(p, 'pp', GIT_COPY, is_git_diff=True, patchlevel=1, + source_filename='PRESUBMIT.py') def testOnlyHeader(self): diff = '--- file_a\n+++ file_a\n' @@ -495,7 +503,7 @@ class PatchTest(unittest.TestCase): diff = '--- file_a\n+++ file_b\n' p = patch.FilePatchDiff('file_b', diff, []) # Should it be marked as new? - self._check_patch(p, 'file_b', diff) + self._check_patch(p, 'file_b', diff, source_filename='file_a') def testGitCopyPartial(self): diff = ( @@ -514,7 +522,8 @@ class PatchTest(unittest.TestCase): ' # found in the LICENSE file.\n') p = patch.FilePatchDiff('wtf2', diff, []) # Should it be marked as new? - self._check_patch(p, 'wtf2', diff, is_git_diff=True, patchlevel=1) + self._check_patch( + p, 'wtf2', diff, source_filename='wtf', is_git_diff=True, patchlevel=1) def testGitExe(self): diff = ( diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py index 5412cf26c..7551e4b80 100755 --- a/tests/rietveld_test.py +++ b/tests/rietveld_test.py @@ -288,6 +288,38 @@ class RietveldTest(unittest.TestCase): # TODO(maruel): Change with no diff, only svn property change: # http://codereview.chromium.org/6462019/ + def test_get_patch_moved(self): + output = ( + '{\n' + ' "files":\n' + ' {\n' + ' "file_a":\n' + ' {\n' + ' "status": "A+",\n' + ' "is_binary": false,\n' + ' "num_chunks": 1,\n' + ' "id": 789\n' + ' }\n' + ' }\n' + '}\n') + diff = ( + '--- /dev/null\n' + '+++ file_a\n' + '@@ -0,0 +1 @@\n' + '+foo\n') + r = rietveld.Rietveld('url', 'email', 'password') + def _send(args, **kwargs): + if args == '/api/123/456': + return output + elif args == '/download/issue123_456_789.diff': + return diff + else: + self.fail() + r._send = _send + patches = r.get_patch(123, 456) + self.assertEquals(diff, patches.patches[0].get()) + self.assertEquals([], patches.patches[0].svn_properties) + if __name__ == '__main__': logging.basicConfig(level=logging.ERROR)