sync: Ensure child dependencies are always removed first.

Fixed: chromium:1454643, chromium:1486677
Change-Id: I3f8f5f9a2e2419830e94fa1419292717820830c8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4914238
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

gclient.py

@@ -2052,7 +2052,17 @@ it or fix the checkout.
         ]
 
         removed_cipd_entries = []
-        for entry, prev_url in self._ReadEntries().items():
+        read_entries = self._ReadEntries()
+        # We process entries sorted in reverse to ensure a child dir is
+        # always deleted before its parent dir.
+        # This is especially important for submodules with pinned revisions
+        # overwritten by a vars or custom_deps. In this case, if a parent
+        # submodule is encountered first in the loop, it cannot tell the
+        # difference between modifications from the vars or actual user
+        # modifications that should be kept. http://crbug/1486677#c9 for
+        # more details.
+        for entry in sorted(read_entries, reverse=True):
+            prev_url = read_entries[entry]
             if not prev_url:
                 # entry must have been overridden via .gclient custom_deps
                 continue
@@ -2092,11 +2102,8 @@ it or fix the checkout.
                         'checkout, so not removing.' % entry)
                     continue
 
-                # This is to handle the case of third_party/WebKit migrating
-                # from being a DEPS entry to being part of the main project. If
-                # the subproject is a Git project, we need to remove its .git
-                # folder. Otherwise git operations on that folder will have
-                # different effects depending on the current working directory.
+                versioned_state = None
+                # Check if this is a submodule or versioned directory.
                 if os.path.abspath(scm_root) == os.path.abspath(e_dir):
                     e_par_dir = os.path.join(e_dir, os.pardir)
                     if gclient_scm.scm.GIT.IsInsideWorkTree(e_par_dir):
@@ -2105,8 +2112,14 @@ it or fix the checkout.
                         # rel_e_dir : relative path of entry w.r.t. its parent
                         # repo.
                         rel_e_dir = os.path.relpath(e_dir, par_scm_root)
-                        if gclient_scm.scm.GIT.IsDirectoryVersioned(
-                                par_scm_root, rel_e_dir):
+                        versioned_state = gclient_scm.scm.GIT.IsVersioned(
+                            par_scm_root, rel_e_dir)
+                        # This is to handle the case of third_party/WebKit migrating
+                        # from being a DEPS entry to being part of the main project. If
+                        # the subproject is a Git project, we need to remove its .git
+                        # folder. Otherwise git operations on that folder will have
+                        # different effects depending on the current working directory.
+                        if versioned_state == gclient_scm.scm.VERSIONED_DIR:
                             save_dir = scm.GetGitBackupDirPath()
                             # Remove any eventual stale backup dir for the same
                             # project.
@@ -2182,6 +2195,10 @@ it or fix the checkout.
                     print('\n________ deleting \'%s\' in \'%s\'' %
                           (entry_fixed, self.root_dir))
                     gclient_utils.rmtree(e_dir)
+                    # We restore empty directories of submodule paths.
+                    if versioned_state == gclient_scm.scm.VERSIONED_SUBMODULE:
+                        gclient_scm.scm.GIT.Capture(
+                            ['restore', '--', rel_e_dir], cwd=par_scm_root)
         # record the current list of entries for next time
         self._SaveEntries()
         return removed_cipd_entries
@@ -2308,7 +2325,8 @@ it or fix the checkout.
                         try:
                             gclient_scm.scm.GIT.Capture(['checkout', tail],
                                                         cwd=cwd)
-                        except subprocess2.CalledProcessError:
+                        except (subprocess2.CalledProcessError, OSError):
+                            # repo of the deleted cipd may also have been deleted.
                             pass
 
         if not self._options.nohooks:

scm.py

@@ -17,6 +17,11 @@ import subprocess2
 # TODO: Should fix these warnings.
 # pylint: disable=line-too-long
 
+# constants used to identify the tree state of a directory.
+VERSIONED_NO = 0
+VERSIONED_DIR = 1
+VERSIONED_SUBMODULE = 2
+
 
 def ValidateEmail(email):
     return (re.match(r"^[a-zA-Z0-9._%\-+]+@[a-zA-Z0-9._%-]+.[a-zA-Z]{2,6}$",
@@ -440,9 +445,15 @@ class GIT(object):
             return False
 
     @staticmethod
-    def IsDirectoryVersioned(cwd, relative_dir):
+    def IsVersioned(cwd, relative_dir):
+        # type: (str, str) -> int
         """Checks whether the given |relative_dir| is part of cwd's repo."""
-        return bool(GIT.Capture(['ls-tree', 'HEAD', relative_dir], cwd=cwd))
+        output = GIT.Capture(['ls-tree', 'HEAD', '--', relative_dir], cwd=cwd)
+        if not output:
+            return VERSIONED_NO
+        if output.startswith('160000'):
+            return VERSIONED_SUBMODULE
+        return VERSIONED_DIR
 
     @staticmethod
     def CleanupDir(cwd, relative_dir):

tests/scm_unittest.py

@@ -135,6 +135,22 @@ class GitWrapperTestCase(unittest.TestCase):
             scm.GIT.GetRemoteHeadRef('foo', 'proto://url', 'origin'))
         self.assertEqual(mockCapture.call_count, 2)
 
+    @mock.patch('scm.GIT.Capture')
+    def testIsVersioned(self, mockCapture):
+        mockCapture.return_value = (
+            '160000 blob 423dc77d2182cb2687c53598a1dcef62ea2804ae   dir')
+        actual_state = scm.GIT.IsVersioned('cwd', 'dir')
+        self.assertEqual(actual_state, scm.VERSIONED_SUBMODULE)
+
+        mockCapture.return_value = ''
+        actual_state = scm.GIT.IsVersioned('cwd', 'dir')
+        self.assertEqual(actual_state, scm.VERSIONED_NO)
+
+        mockCapture.return_value = (
+            '040000 tree ef016abffb316e47a02af447bc51342dcef6f3ca    dir')
+        actual_state = scm.GIT.IsVersioned('cwd', 'dir')
+        self.assertEqual(actual_state, scm.VERSIONED_DIR)
+
 
 class RealGitTest(fake_repos.FakeReposTestBase):
     def setUp(self):