From 5e975633a8c7ebd9dadbc0cb6a6e6dd652a7a27d Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Thu, 29 Sep 2011 18:07:06 +0000 Subject: [PATCH] Fix handling of file renames. The patches needs to by applied 'in-order' to not delete source files. Added more tests. R=dpranke@chromium.org BUG=94330 TEST= Review URL: http://codereview.chromium.org/8038056 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@103309 0039d316-1c4b-4281-b951-d872f2087c98 --- apply_issue.py | 5 ++- checkout.py | 42 ++++++++++++++++++--- patch.py | 62 +++++++++++++++++++++++++----- tests/checkout_test.py | 70 +++++++++++++++++++++++++++++++--- tests/patch_test.py | 85 +++++++++++++++++++++++++++++++++--------- tests/rietveld_test.py | 6 +-- 6 files changed, 227 insertions(+), 43 deletions(-) diff --git a/apply_issue.py b/apply_issue.py index 69faf8dc2..5bf72197b 100755 --- a/apply_issue.py +++ b/apply_issue.py @@ -40,7 +40,7 @@ def main(): help='Rietveld server') options, args = parser.parse_args() logging.basicConfig( - format='%(levelname)s %(filename)s(%(lineno)d): %(message)s', + format='%(levelname)5s %(module)11s(%(lineno)4d): %(message)s', level=[logging.WARNING, logging.INFO, logging.DEBUG][ min(2, options.verbose)]) if args: @@ -56,7 +56,8 @@ def main(): logging.info('Using patchset %d' % options.patchset) # Download the patch. patchset = obj.get_patch(options.issue, options.patchset) - + for patch in patchset.patches: + logging.info(patch) scm_type = scm.determine_scm(options.root_dir) if scm_type == 'svn': scm_obj = checkout.SvnCheckout(options.root_dir, None, None, None, None) diff --git a/checkout.py b/checkout.py index 672ad9aeb..8a3f7125d 100644 --- a/checkout.py +++ b/checkout.py @@ -13,6 +13,7 @@ import fnmatch import logging import os import re +import shutil import subprocess import sys import tempfile @@ -137,10 +138,21 @@ class RawCheckout(CheckoutBase): with open(filepath, 'wb') as f: f.write(p.get()) else: + if p.source_filename: + if not p.is_new: + raise PatchApplicationFailed( + p.filename, + 'File has a source filename specified but is not new') + # Copy the file first. + if os.path.isfile(filepath): + raise PatchApplicationFailed( + p.filename, 'File exist but was about to be overwriten') + shutil.copy2( + os.path.join(self.project_path, p.source_filename), filepath) if p.diff_hunks: stdout = subprocess2.check_output( - ['patch', '-p%s' % p.patchlevel], - stdin=p.get(), + ['patch', '-u', '--binary', '-p%s' % p.patchlevel], + stdin=p.get(False), stderr=subprocess2.STDOUT, cwd=self.project_path) elif p.is_new and not os.path.exists(filepath): @@ -293,10 +305,21 @@ class SvnCheckout(CheckoutBase, SvnMixIn): with open(filepath, 'wb') as f: f.write(p.get()) else: + if p.source_filename: + if not p.is_new: + raise PatchApplicationFailed( + p.filename, + 'File has a source filename specified but is not new') + # Copy the file first. + if os.path.isfile(filepath): + raise PatchApplicationFailed( + p.filename, 'File exist but was about to be overwriten') + shutil.copy2( + os.path.join(self.project_path, p.source_filename), filepath) if p.diff_hunks: cmd = ['patch', '-p%s' % p.patchlevel, '--forward', '--force'] stdout += subprocess2.check_output( - cmd, stdin=p.get(), cwd=self.project_path) + cmd, stdin=p.get(False), cwd=self.project_path) elif p.is_new and not os.path.exists(filepath): # There is only a header. Just create the file if it doesn't # exist. @@ -434,11 +457,18 @@ class GitCheckoutBase(CheckoutBase): self._check_call_git( ['checkout', '-b', self.working_branch, '%s/%s' % (self.remote, self.remote_branch), '--quiet']) - for p in patches: + for index, p in enumerate(patches): try: stdout = '' if p.is_delete: - stdout += self._check_output_git(['rm', p.filename]) + if (not os.path.exists(p.filename) and + any(p1.source_filename == p.filename for p1 in patches[0:index])): + # The file could already be deleted if a prior patch with file + # rename was already processed. To be sure, look at all the previous + # patches to see if they were a file rename. + pass + else: + stdout += self._check_output_git(['rm', p.filename]) else: dirname = os.path.dirname(p.filename) full_dir = os.path.join(self.project_path, dirname) @@ -452,7 +482,7 @@ class GitCheckoutBase(CheckoutBase): # No need to do anything special with p.is_new or if not # p.diff_hunks. git apply manages all that already. stdout += self._check_output_git( - ['apply', '--index', '-p%s' % p.patchlevel], stdin=p.get()) + ['apply', '--index', '-p%s' % p.patchlevel], stdin=p.get(True)) for prop in p.svn_properties: # Ignore some known auto-props flags through .subversion/config, # bails out on the other ones. diff --git a/patch.py b/patch.py index 8b0407fab..4c96de356 100644 --- a/patch.py +++ b/patch.py @@ -32,6 +32,7 @@ class FilePatchBase(object): is_new = False def __init__(self, filename): + assert self.__class__ is not FilePatchBase self.filename = self._process_filename(filename) # Set when the file is copied or moved. self.source_filename = None @@ -50,9 +51,6 @@ class FilePatchBase(object): filename, 'Filename can\'t start with \'%s\'.' % i) return filename - def get(self): # pragma: no coverage - raise NotImplementedError('Nothing to grab') - def set_relpath(self, relpath): if not relpath: return @@ -69,6 +67,27 @@ class FilePatchBase(object): """Shortcut function to raise UnsupportedPatchFormat.""" raise UnsupportedPatchFormat(self.filename, msg) + def __str__(self): + # Use a status-like board. + out = '' + if self.is_binary: + out += 'B' + else: + out += ' ' + if self.is_delete: + out += 'D' + else: + out += ' ' + if self.is_new: + out += 'N' + else: + out += ' ' + if self.source_filename: + out += 'R' + else: + out += ' ' + return out + ' %s->%s' % (self.source_filename, self.filename) + class FilePatchDelete(FilePatchBase): """Deletes a file.""" @@ -78,9 +97,6 @@ class FilePatchDelete(FilePatchBase): super(FilePatchDelete, self).__init__(filename) self.is_binary = is_binary - def get(self): - raise NotImplementedError('Nothing to grab') - class FilePatchBinary(FilePatchBase): """Content of a new binary file.""" @@ -111,9 +127,18 @@ class FilePatchDiff(FilePatchBase): self._verify_git_header() else: self._verify_svn_header() + if self.source_filename and not self.is_new: + self._fail('If source_filename is set, is_new must be also be set') - def get(self): - return self.diff_header + self.diff_hunks + def get(self, for_git): + if for_git or not self.source_filename: + return self.diff_header + self.diff_hunks + else: + # patch is stupid. It patches the source_filename instead so get rid of + # any source_filename reference if needed. + return ( + self.diff_header.replace(self.source_filename, self.filename) + + self.diff_hunks) def set_relpath(self, relpath): old_filename = self.filename @@ -264,6 +289,7 @@ class FilePatchDiff(FilePatchBase): (match.group(1), line)) return + # Ignore "deleted file mode 100644" since it's not needed. match = re.match(r'^new(| file) mode (\d{6})$', line) if match: mode = match.group(2) @@ -356,10 +382,23 @@ class PatchSet(object): """A list of FilePatch* objects.""" def __init__(self, patches): - self.patches = patches - for p in self.patches: + for p in patches: assert isinstance(p, FilePatchBase) + def key(p): + """Sort by ordering of application. + + File move are first. + Deletes are last. + """ + if p.source_filename: + return (p.is_delete, p.source_filename, p.filename) + else: + # tuple are always greater than string, abuse that fact. + return (p.is_delete, (p.filename,), p.filename) + + self.patches = sorted(patches, key=key) + def set_relpath(self, relpath): """Used to offset the patch into a subdirectory.""" for patch in self.patches: @@ -369,6 +408,9 @@ class PatchSet(object): for patch in self.patches: yield patch + def __getitem__(self, key): + return self.patches[key] + @property def filenames(self): return [p.filename for p in self.patches] diff --git a/tests/checkout_test.py b/tests/checkout_test.py index 21cd67150..1428bd730 100755 --- a/tests/checkout_test.py +++ b/tests/checkout_test.py @@ -40,6 +40,11 @@ class FakeRepos(fake_repos.FakeReposBase): '--non-interactive', '--no-auth-cache', '--username', self.USERS[0][0], '--password', self.USERS[0][1]]) assert os.path.isdir(os.path.join(self.svn_checkout, '.svn')) + self._commit_svn(self._tree_1()) + self._commit_svn(self._tree_2()) + + @staticmethod + def _tree_1(): fs = {} fs['trunk/origin'] = 'svn@1' fs['trunk/codereview.settings'] = ( @@ -63,11 +68,26 @@ class FakeRepos(fake_repos.FakeReposBase): 'ooo\n' 'pp\n' 'q\n') - self._commit_svn(fs) + return fs + + @classmethod + def _tree_2(cls): + fs = cls._tree_1() fs['trunk/origin'] = 'svn@2\n' fs['trunk/extra'] = 'dummy\n' fs['trunk/bin_file'] = '\x00' - self._commit_svn(fs) + fs['trunk/chromeos/views/DOMui_menu_widget.h'] = ( + '// Copyright (c) 2010\n' + '// Use of this source code\n' + '// found in the LICENSE file.\n' + '\n' + '#ifndef DOM\n' + '#define DOM\n' + '#pragma once\n' + '\n' + '#include \n' + '#endif\n') + return fs def populateGit(self): raise NotImplementedError() @@ -100,11 +120,10 @@ class BaseTest(fake_repos.FakeReposTestBase): def get_patches(self): return patch.PatchSet([ + patch.FilePatchDiff('new_dir/subdir/new_file', GIT.NEW_SUBDIR, []), + patch.FilePatchDiff('chrome/file.cc', GIT.PATCH, []), # TODO(maruel): Test with is_new == False. patch.FilePatchBinary('bin_file', '\x00', [], is_new=True), - patch.FilePatchDiff( - 'chrome/file.cc', GIT.PATCH, []), - patch.FilePatchDiff('new_dir/subdir/new_file', GIT.NEW_SUBDIR, []), patch.FilePatchDelete('extra', False), ]) @@ -201,6 +220,35 @@ class BaseTest(fake_repos.FakeReposTestBase): self.assertEquals(len(expected), len(results)) self.assertEquals(expected, results) + def _check_move(self, co): + """Makes sure file moves are handled correctly.""" + co.prepare(None) + patchset = patch.PatchSet([ + patch.FilePatchDelete('chromeos/views/DOMui_menu_widget.h', False), + patch.FilePatchDiff( + 'chromeos/views/webui_menu_widget.h', GIT.RENAME_PARTIAL, []), + ]) + co.apply_patch(patchset) + # Make sure chromeos/views/DOMui_menu_widget.h is deleted and + # chromeos/views/webui_menu_widget.h is correctly created. + root = os.path.join(self.root_dir, self.name) + tree = self.get_trunk(False) + del tree['chromeos/views/DOMui_menu_widget.h'] + tree['chromeos/views/webui_menu_widget.h'] = ( + '// Copyright (c) 2011\n' + '// Use of this source code\n' + '// found in the LICENSE file.\n' + '\n' + '#ifndef WEB\n' + '#define WEB\n' + '#pragma once\n' + '\n' + '#include \n' + '#endif\n') + #print patchset[0].get() + #print fake_repos.read_tree(root) + self.assertTree(tree, root) + class SvnBaseTest(BaseTest): def setUp(self): @@ -349,6 +397,9 @@ class SvnCheckout(SvnBaseTest): def testPrepare(self): self._test_prepare(self._get_co(None)) + def testMove(self): + self._check_move(self._get_co(None)) + class GitSvnCheckout(SvnBaseTest): name = 'foo.git' @@ -419,6 +470,9 @@ class GitSvnCheckout(SvnBaseTest): co.prepare = prepare self._test_prepare(co) + def testMove(self): + self._check_move(self._get_co(None)) + class RawCheckout(SvnBaseTest): def setUp(self): @@ -482,6 +536,9 @@ class RawCheckout(SvnBaseTest): self._test_prepare(co) self.assertEquals([], revs) + def testMove(self): + self._check_move(self._get_co(None)) + class ReadOnlyCheckout(SvnBaseTest): # Use SvnCheckout as the backed since it support read-only checkouts too. @@ -513,6 +570,9 @@ class ReadOnlyCheckout(SvnBaseTest): def testPrepare(self): self._test_prepare(self._get_co(None)) + def testMove(self): + self._check_move(self._get_co(None)) + if __name__ == '__main__': if '-v' in sys.argv: diff --git a/tests/patch_test.py b/tests/patch_test.py index a27fa8660..2710a3864 100755 --- a/tests/patch_test.py +++ b/tests/patch_test.py @@ -40,7 +40,10 @@ class PatchTest(unittest.TestCase): if hasattr(p, 'patchlevel'): self.assertEquals(p.patchlevel, patchlevel) if diff: - self.assertEquals(p.get(), diff) + if is_binary: + self.assertEquals(p.get(), diff) + else: + self.assertEquals(p.get(True), diff) if hasattr(p, 'svn_properties'): self.assertEquals(p.svn_properties, svn_properties) @@ -97,19 +100,29 @@ class PatchTest(unittest.TestCase): hunks = ''.join(lines[4:]) self.assertEquals(header, p.diff_header) self.assertEquals(hunks, p.diff_hunks) - self.assertEquals(RAW.PATCH, p.get()) + self.assertEquals(RAW.PATCH, p.get(True)) + self.assertEquals(RAW.PATCH, p.get(False)) def testValidSvnNew(self): p = patch.FilePatchDiff('chrome/file.cc', RAW.MINIMAL_NEW, []) self.assertEquals(RAW.MINIMAL_NEW, p.diff_header) self.assertEquals('', p.diff_hunks) - self.assertEquals(RAW.MINIMAL_NEW, p.get()) + self.assertEquals(RAW.MINIMAL_NEW, p.get(True)) + self.assertEquals(RAW.MINIMAL_NEW, p.get(False)) def testValidSvnDelete(self): p = patch.FilePatchDiff('chrome/file.cc', RAW.MINIMAL_DELETE, []) self.assertEquals(RAW.MINIMAL_DELETE, p.diff_header) self.assertEquals('', p.diff_hunks) - self.assertEquals(RAW.MINIMAL_DELETE, p.get()) + self.assertEquals(RAW.MINIMAL_DELETE, p.get(True)) + self.assertEquals(RAW.MINIMAL_DELETE, p.get(False)) + + def testValidSvnRename(self): + p = patch.FilePatchDiff('file_b', RAW.MINIMAL_RENAME, []) + self.assertEquals(RAW.MINIMAL_RENAME, p.diff_header) + self.assertEquals('', p.diff_hunks) + self.assertEquals(RAW.MINIMAL_RENAME, p.get(True)) + self.assertEquals('--- file_b\n+++ file_b\n', p.get(False)) def testRelPath(self): patches = patch.PatchSet([ @@ -176,7 +189,8 @@ class PatchTest(unittest.TestCase): ]) expected = ['chrome/file.cc', 'other/place/foo'] self.assertEquals(expected, patches.filenames) - self.assertEquals(RAW.PATCH, patches.patches[0].get()) + self.assertEquals(RAW.PATCH, patches.patches[0].get(True)) + self.assertEquals(RAW.PATCH, patches.patches[0].get(False)) def testDelete(self): p = patch.FilePatchDiff('tools/clang_check/README.chromium', RAW.DELETE, []) @@ -227,6 +241,25 @@ class PatchTest(unittest.TestCase): p, 'wtf2', GIT.COPY_PARTIAL, source_filename='wtf', is_git_diff=True, patchlevel=1, is_new=True) + def testGitCopyPartialAsSvn(self): + p = patch.FilePatchDiff('wtf2', GIT.COPY_PARTIAL, []) + # TODO(maruel): Improve processing. + diff = ( + 'diff --git a/wtf2 b/wtf22\n' + 'similarity index 98%\n' + 'copy from wtf2\n' + 'copy to wtf22\n' + 'index 79fbaf3..3560689 100755\n' + '--- a/wtf2\n' + '+++ b/wtf22\n' + '@@ -1,4 +1,4 @@\n' + '-#!/usr/bin/env python\n' + '+#!/usr/bin/env python1.3\n' + ' # Copyright (c) 2010 The Chromium Authors. All rights reserved.\n' + ' # blah blah blah as\n' + ' # found in the LICENSE file.\n') + self.assertEquals(diff, p.get(False)) + def testGitNewExe(self): p = patch.FilePatchDiff('natsort_test.py', GIT.NEW_EXE, []) self._check_patch( @@ -239,24 +272,42 @@ class PatchTest(unittest.TestCase): p, 'natsort_test.py', GIT.NEW_MODE, is_new=True, is_git_diff=True, patchlevel=1) + def testPatchsetOrder(self): + # Deletes must be last. + # File renames/move/copy must be first. + patches = [ + patch.FilePatchDiff('chrome/file.cc', RAW.PATCH, []), + patch.FilePatchDiff( + 'tools\\clang_check/README.chromium', GIT.DELETE, []), + patch.FilePatchDiff('tools/run_local_server.sh', GIT.RENAME, []), + patch.FilePatchDiff( + '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), + patch.FilePatchBinary('bar', 'data', [], is_new=False), + ] + expected = [ + 'pp', + 'chromeos/views/webui_menu_widget.h', + 'tools/run_local_server.sh', + 'bar', + 'chrome/file.cc', + 'foo', + 'other/place/foo', + 'tools/clang_check/README.chromium', + ] + patchset = patch.PatchSet(patches) + self.assertEquals(expected, patchset.filenames) + class PatchTestFail(unittest.TestCase): # All patches that should throw. def testFilePatchDelete(self): - p = patch.FilePatchDelete('foo', False) - try: - p.get() - self.fail() - except NotImplementedError: - pass + self.assertFalse(hasattr(patch.FilePatchDelete('foo', False), 'get')) def testFilePatchDeleteBin(self): - p = patch.FilePatchDelete('foo', True) - try: - p.get() - self.fail() - except NotImplementedError: - pass + self.assertFalse(hasattr(patch.FilePatchDelete('foo', True), 'get')) def testFilePatchDiffBad(self): try: diff --git a/tests/rietveld_test.py b/tests/rietveld_test.py index 3316df55e..798e84dd9 100755 --- a/tests/rietveld_test.py +++ b/tests/rietveld_test.py @@ -82,7 +82,7 @@ class RietveldTest(unittest.TestCase): if hasattr(p, 'patchlevel'): self.assertEquals(p.patchlevel, patchlevel) if diff: - self.assertEquals(p.get(), diff) + self.assertEquals(p.get(True), diff) if hasattr(p, 'svn_properties'): self.assertEquals(p.svn_properties, svn_properties) @@ -103,9 +103,9 @@ class RietveldTest(unittest.TestCase): ] patches = self.rietveld.get_patch(123, 456) self.assertEquals(2, len(patches.patches)) - self._check_patch(patches.patches[0], 'foo', RAW.NEW, is_new=True) # TODO(maruel): svn sucks. - self._check_patch(patches.patches[1], 'file_a', RAW.NEW_NOT_NULL) + self._check_patch(patches.patches[0], 'file_a', RAW.NEW_NOT_NULL) + self._check_patch(patches.patches[1], 'foo', RAW.NEW, is_new=True) def test_get_patch_add(self): self.requests = [