diff --git a/subcmds/sync.py b/subcmds/sync.py index e71330070..bedc1f1b9 100644 --- a/subcmds/sync.py +++ b/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 diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index e854a1940..b921069d0 100644 --- a/tests/test_subcmds_sync.py +++ b/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"])