sync: skip submodules removed from their parent

The submodules to sync are derived from their parent before it is
fetched, so one that the parent's new revision no longer holds a gitlink
for is still synced. Interleaved sync then fails to check it out,
because `git submodule init` no longer knows that path:

  error: Cannot checkout a/c
  error: pathspec '.../a/c' did not match any file(s) known to git

Report such submodules while the gitlinks are read again, and leave them
out of the fetch and of the checkout, so the usual project list update
removes them from the working tree. Phased sync already left them out of
the checkout, but still fetched them.

Nothing is reported as removed while the gitlinks of a parent cannot be
read, since a missing gitlink cannot be told apart from a fetch that did
not happen.

Bug: 550074864
Change-Id: Ia429b35c12758a68e4df73665e5512a201af658a
Signed-off-by: kimhappy <hwanhee.kim@laplacian.cc>
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/621681
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Brian Gan <brgan@google.com>

subcmds/sync.py

@@ -179,7 +179,7 @@ def _ParentFirstBatches(projects: List[Project]) -> List[List[Project]]:
 def _RefreshDerivedRevisions(
     projects: List[Project],
     submodule_revisions: Optional[Dict[Project, Dict[str, str]]] = None,
-) -> None:
+) -> List[Project]:
     """Re-resolve the gitlinks of the discovered submodules in |projects|.
 
     The revision of a discovered submodule is read from its parent when the
@@ -193,6 +193,9 @@ def _RefreshDerivedRevisions(
             holding them. Passing the same dict for project sets that follow
             the same fetches, e.g. the levels of one checkout order, keeps a
             project from being read more than once.
+
+    Returns:
+        The submodules that their parent no longer holds a gitlink for.
     """
     if submodule_revisions is None:
         submodule_revisions = {}
@@ -202,6 +205,7 @@ def _RefreshDerivedRevisions(
         if project.Derived and project.parent:
             subprojects_by_parent[project.parent].append(project)
 
+    removed = []
     for parent, subprojects in subprojects_by_parent.items():
         revisions = submodule_revisions.get(parent)
         if revisions is None:
@@ -214,6 +218,19 @@ def _RefreshDerivedRevisions(
             rev = revisions.get(subproject.gitlink_path)
             if rev:
                 subproject.SetRevision(rev, revisionId=rev)
+            else:
+                removed.append(subproject)
+    return removed
+
+
+def _WithoutProjects(
+    projects: List[Project], unwanted: List[Project]
+) -> List[Project]:
+    """Return |projects| without the projects in |unwanted|."""
+    if not unwanted:
+        return projects
+    dropped = set(unwanted)
+    return [p for p in projects if p not in dropped]
 
 
 def _chunksize(projects: int, jobs: int) -> int:
@@ -1150,7 +1167,9 @@ later is required to fix a server side protocol bug.
         success = True
         fetched = set()
         for batch in _ParentFirstBatches(projects):
-            _RefreshDerivedRevisions(batch)
+            batch = _WithoutProjects(batch, _RefreshDerivedRevisions(batch))
+            if not batch:
+                continue
             batch.sort(key=self._fetch_times.Get, reverse=True)
             result = self._Fetch(batch, opt, err_event, ssh_proxy, errors)
             success = success and result.success
@@ -3076,9 +3095,14 @@ later is required to fix a server side protocol bug.
                                 if not level_projects:
                                     continue
 
-                                _RefreshDerivedRevisions(
-                                    level_projects, submodule_revisions
+                                level_projects = _WithoutProjects(
+                                    level_projects,
+                                    _RefreshDerivedRevisions(
+                                        level_projects, submodule_revisions
+                                    ),
                                 )
+                                if not level_projects:
+                                    continue
 
                                 objdir_project_map = collections.defaultdict(
                                     list

tests/test_subcmds_sync.py

@@ -708,6 +708,14 @@ class RefreshDerivedRevisions(unittest.TestCase):
 
         p_a.GetSubmoduleRevisions.assert_not_called()
 
+    def test_reports_submodules_removed_from_their_parent(self) -> None:
+        p_a = self._parent_with_submodules(b="beef1234")
+        p_a_c = self._submodule(p_a, "c")
+
+        removed = sync._RefreshDerivedRevisions([p_a, p_a_c])
+
+        self.assertEqual(removed, [p_a_c])
+
     def test_keeps_submodules_of_an_unreadable_parent(self) -> None:
         p_a = FakeProject("a")
         p_a.GetSubmoduleRevisions = mock.Mock(return_value=None)
@@ -719,8 +727,9 @@ class RefreshDerivedRevisions(unittest.TestCase):
             gitlink_path="b",
         )
 
-        sync._RefreshDerivedRevisions([p_a, p_a_b])
+        removed = sync._RefreshDerivedRevisions([p_a, p_a_b])
 
+        self.assertEqual(removed, [])
         self.assertEqual(p_a_b.revisionId, "stale")
 
 
@@ -764,6 +773,23 @@ class FetchParentFirst(unittest.TestCase):
         )
 
 
+class WithoutProjects(unittest.TestCase):
+    def test_drops_the_unwanted_projects(self) -> None:
+        p_a = FakeProject("a")
+        p_a_b = FakeProject("a/b")
+        self.assertEqual(sync._WithoutProjects([p_a, p_a_b], [p_a_b]), [p_a])
+        self.assertEqual(sync._WithoutProjects([p_a, p_a_b], []), [p_a, p_a_b])
+
+    def test_keeps_projects_with_the_same_path(self) -> None:
+        # Paths are relative to their own (sub)manifest, so two projects can
+        # share one.
+        first = FakeProject("a/b")
+        second = FakeProject("a/b")
+        self.assertEqual(
+            sync._WithoutProjects([first, second], [second]), [first]
+        )
+
+
 class Chunksize(unittest.TestCase):
     """Tests for _chunksize."""
 
@@ -1364,6 +1390,60 @@ class InterleavedSyncTest(unittest.TestCase):
 
         self.assertIn(("projA/sub", "fetched"), synced)
 
+    def test_interleaved_skips_removed_submodule(self) -> None:
+        """Test submodules dropped by their parent are not checked out."""
+        opt, args = self.cmd.OptionParser.parse_args(["--interleaved", "-j4"])
+        opt.quiet = True
+
+        submodule = FakeProject(
+            "projA/sub",
+            name="projA_sub",
+            objdir="objA_sub",
+            parent=self.projA,
+            is_derived=True,
+            revisionId="stale",
+            gitlink_path="sub",
+        )
+        # The parent no longer holds a gitlink for the submodule.
+        self.projA.GetSubmoduleRevisions = mock.Mock(return_value={})
+        # The reloaded manifest no longer derives the removed submodule.
+        mock.patch.object(
+            self.cmd, "GetProjects", return_value=[self.projA]
+        ).start()
+
+        synced = []
+
+        def execute_side_effect(
+            jobs: int,
+            target: object,
+            work_items: List[List[int]],
+            **kwargs: object,
+        ) -> bool:
+            synced_relpaths_set = kwargs["callback"].args[0]
+            projects_in_pass = self.cmd.get_parallel_context()["projects"]
+            for item in work_items:
+                for project_idx in item:
+                    project = projects_in_pass[project_idx]
+                    synced.append(project.relpath)
+                    synced_relpaths_set.add(project.relpath)
+            return True
+
+        mock.patch.object(
+            self.cmd, "ExecuteInParallel", side_effect=execute_side_effect
+        ).start()
+
+        self.cmd._SyncInterleaved(
+            opt,
+            args,
+            [],
+            self.manifest,
+            self.manifest.manifestProject,
+            [self.projA, submodule],
+            {},
+        )
+
+        self.assertEqual(synced, ["projA"])
+
     def test_interleaved_shared_objdir_serial(self):
         """Test that projects with shared objdir are processed serially."""
         opt, args = self.cmd.OptionParser.parse_args(["--interleaved", "-j4"])