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

@@ -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)

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.

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]

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 <string>\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 <string>\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:

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:

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 = [