Improve SVN.Revert() again to work better in reverting svn added directories.

Revert the order back to deleting then svn revert'ing, which is safer than the
reverse. Document a bit better the behavior.

R=dpranke@chromium.org
BUG=none
TEST=commit queue should die less often


Review URL: http://codereview.chromium.org/6681032

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@78224 0039d316-1c4b-4281-b951-d872f2087c98

scm.py

@@ -863,7 +863,9 @@ class SVN(object):
     """
     for file_status in SVN.CaptureStatus(repo_root):
       file_path = os.path.join(repo_root, file_status[1])
-      if ignore_externals and file_status[0][0] == 'X':
+      if (ignore_externals and
+          file_status[0][0] == 'X' and
+          file_status[0][1:].isspace()):
         # Ignore externals.
         logging.info('Ignoring external %s' % file_status[1])
         continue
@@ -871,14 +873,25 @@ class SVN(object):
       if callback:
         callback(file_status)
 
-      if file_status[0].isspace():
-        # Try reverting the file since it's probably a property change.
-        gclient_utils.CheckCall(
-            ['svn', 'revert', file_status[1]], cwd=repo_root)
+      if os.path.exists(file_path):
+        # svn revert is really stupid. It fails on inconsistent line-endings,
+        # on switched directories, etc. So take no chance and delete everything!
+        # In theory, it wouldn't be necessary for property-only change but then
+        # it'd have to look for switched directories, etc so it's not worth
+        # optimizing this use case.
+        if os.path.isfile(file_path) or os.path.islink(file_path):
+          logging.info('os.remove(%s)' % file_path)
+          os.remove(file_path)
+        elif os.path.isdir(file_path):
+          logging.info('gclient_utils.RemoveDirectory(%s)' % file_path)
+          gclient_utils.RemoveDirectory(file_path)
+        else:
+          logging.critical(
+            ('No idea what is %s.\nYou just found a bug in gclient'
+              ', please ping maruel@chromium.org ASAP!') % file_path)
 
-      # svn revert is really stupid. It fails on inconsistent line-endings,
-      # on switched directories, etc. So take no chance and delete everything!
-      if file_status[0][0] in ('D', 'A', '!') or file_status[0][2] != ' ':
+      if (file_status[0][0] in ('D', 'A', '!') or
+          not file_status[0][1:].isspace()):
         # Added, deleted file requires manual intervention and require calling
         # revert, like for properties.
         try:
@@ -887,17 +900,3 @@ class SVN(object):
           if not os.path.exists(file_path):
             continue
           raise
-
-      if not os.path.exists(file_path):
-        continue
-
-      if os.path.isfile(file_path) or os.path.islink(file_path):
-        logging.info('os.remove(%s)' % file_path)
-        os.remove(file_path)
-      elif os.path.isdir(file_path):
-        logging.info('gclient_utils.RemoveDirectory(%s)' % file_path)
-        gclient_utils.RemoveDirectory(file_path)
-      else:
-        logging.critical(
-          ('No idea what is %s.\nYou just found a bug in gclient'
-            ', please ping maruel@chromium.org ASAP!') % file_path)

tests/scm_unittest.py

@@ -267,26 +267,43 @@ class RealSvnTest(fake_repos.FakeReposTestBase):
     # - Delete a file
     # - svn delete a file
     # - svn move a directory and svn rename files in it
+    # - add a directory tree.
+    def join(*args):
+      return scm.os.path.join(self.svn_root, *args)
     self._capture(['move', 'foo', 'foo2'])
     self._capture(
         ['move',
          scm.os.path.join('foo2', 'origin'),
          scm.os.path.join('foo2', 'o')])
-    scm.os.remove(scm.os.path.join(self.svn_root, 'origin'))
-    self._capture(
-        ['propset', 'foo', 'bar',
-         scm.os.path.join(self.svn_root, 'prout', 'origin')])
-    fake_repos.gclient_utils.rmtree(scm.os.path.join(self.svn_root, 'prout'))
-    with open(scm.os.path.join(self.svn_root, 'faa'), 'w') as f:
+    scm.os.remove(join('origin'))
+    self._capture(['propset', 'foo', 'bar', join('prout', 'origin')])
+    fake_repos.gclient_utils.rmtree(join('prout'))
+    with open(join('faa'), 'w') as f:
       f.write('eh')
-    with open(scm.os.path.join(self.svn_root, 'faala'), 'w') as f:
+    with open(join('faala'), 'w') as f:
       f.write('oh')
-    self._capture(['add', scm.os.path.join(self.svn_root, 'faala')])
-    added_and_removed = scm.os.path.join(self.svn_root, 'added_and_removed')
+    self._capture(['add', join('faala')])
+    added_and_removed = join('added_and_removed')
     with open(added_and_removed, 'w') as f:
       f.write('oh')
     self._capture(['add', added_and_removed])
     scm.os.remove(added_and_removed)
+    # Make sure a tree of directories can be removed.
+    scm.os.makedirs(join('new_dir', 'subdir'))
+    with open(join('new_dir', 'subdir', 'newfile'), 'w') as f:
+      f.write('ah!')
+    self._capture(['add', join('new_dir')])
+    self._capture(['add', join('new_dir', 'subdir')])
+    self._capture(['add', join('new_dir', 'subdir', 'newfile')])
+    # A random file in an added directory confuses svn.
+    scm.os.makedirs(join('new_dir2', 'subdir'))
+    with open(join('new_dir2', 'subdir', 'newfile'), 'w') as f:
+      f.write('ah!')
+    self._capture(['add', join('new_dir2')])
+    self._capture(['add', join('new_dir2', 'subdir')])
+    self._capture(['add', join('new_dir2', 'subdir', 'newfile')])
+    with open(join('new_dir2', 'subdir', 'unversionedfile'), 'w') as f:
+      f.write('unadded file!')
 
     scm.SVN.Revert(self.svn_root)
     self._capture(['update', '--revision', 'base'])