diff --git a/subcmds/sync.py b/subcmds/sync.py index 6a8853088..af123995a 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py @@ -28,7 +28,7 @@ import subprocess import sys import tempfile import time -from typing import List, NamedTuple, Optional, Set, Tuple, Union +from typing import Dict, List, NamedTuple, Optional, Set, Tuple, Union import urllib.error import urllib.parse import urllib.request @@ -156,6 +156,46 @@ def _SafeCheckoutOrder(checkouts: List[Project]) -> List[List[Project]]: return res +def _RefreshDerivedRevisions( + projects: List[Project], + submodule_revisions: Optional[Dict[Project, Dict[str, str]]] = None, +) -> None: + """Re-resolve the gitlinks of the discovered submodules in |projects|. + + The revision of a discovered submodule is read from its parent when the + manifest is loaded, so it is stale as soon as the parent gets fetched. It + has to be resolved again once the parent is up-to-date, and before the + submodule itself is fetched and checked out. + + Args: + projects: The projects whose discovered submodules to resolve. + submodule_revisions: Gitlinks already read, keyed by the project + 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. + """ + if submodule_revisions is None: + submodule_revisions = {} + + subprojects_by_parent = collections.defaultdict(list) + for project in projects: + if project.Derived and project.parent: + subprojects_by_parent[project.parent].append(project) + + for parent, subprojects in subprojects_by_parent.items(): + revisions = submodule_revisions.get(parent) + if revisions is None: + revisions = parent.GetSubmoduleRevisions() + if revisions is None: + # Leave the submodules of a parent we cannot read alone. + continue + submodule_revisions[parent] = revisions + for subproject in subprojects: + rev = revisions.get(subproject.gitlink_path) + if rev: + subproject.SetRevision(rev, revisionId=rev) + + def _chunksize(projects: int, jobs: int) -> int: """Calculate chunk size for the given number of projects and jobs.""" return min(max(1, projects // jobs), WORKER_BATCH_SIZE) @@ -2977,12 +3017,17 @@ later is required to fix a server side protocol bug. # projects in one level can be processed in # parallel, but we must wait for a level to complete # before starting the next. + submodule_revisions = {} for level_projects in _SafeCheckoutOrder( projects_to_sync ): if not level_projects: continue + _RefreshDerivedRevisions( + level_projects, submodule_revisions + ) + objdir_project_map = collections.defaultdict( list ) diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index bc0a82759..0e51dfeff 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py @@ -19,6 +19,7 @@ from pathlib import Path import shutil import tempfile import time +from typing import List, Optional import unittest from unittest import mock @@ -491,12 +492,24 @@ class LocalSyncState(unittest.TestCase): class FakeProject: - def __init__(self, relpath, name=None, objdir=None): + def __init__( + self, + relpath: str, + name: Optional[str] = None, + objdir: Optional[str] = None, + parent: Optional["FakeProject"] = None, + is_derived: bool = False, + revisionId: Optional[str] = None, + gitlink_path: Optional[str] = None, + ) -> None: self.relpath = relpath self.name = name or relpath self.objdir = objdir or relpath self.worktree = relpath - self.parent = None + self.parent = parent + self.is_derived = is_derived + self.revisionId = revisionId + self.gitlink_path = gitlink_path self.use_git_worktrees = False self.UseAlternates = False @@ -505,6 +518,16 @@ class FakeProject: self.config = mock.MagicMock() self.EnableRepositoryExtension = mock.MagicMock() + @property + def Derived(self) -> bool: + return self.is_derived + + def SetRevision( + self, revisionExpr: str, revisionId: Optional[str] = None + ) -> None: + self.revisionExpr = revisionExpr + self.revisionId = revisionId or revisionExpr + def RelPath(self, local=None): return self.relpath @@ -614,6 +637,78 @@ class SafeCheckoutOrder(unittest.TestCase): ) +class RefreshDerivedRevisions(unittest.TestCase): + def _parent_with_submodules(self, **gitlinks: str) -> FakeProject: + p_a = FakeProject("a") + p_a.GetSubmoduleRevisions = mock.Mock(return_value=gitlinks) + return p_a + + def _submodule(self, parent: FakeProject, path: str) -> FakeProject: + return FakeProject( + f"a/{path}", parent=parent, is_derived=True, gitlink_path=path + ) + + def test_reads_each_parent_once(self) -> None: + p_a = self._parent_with_submodules(b="beef1234", c="cafe1234") + p_a_b = self._submodule(p_a, "b") + p_a_c = self._submodule(p_a, "c") + + sync._RefreshDerivedRevisions([p_a, p_a_b, p_a_c]) + + p_a.GetSubmoduleRevisions.assert_called_once_with() + self.assertEqual(p_a_b.revisionId, "beef1234") + self.assertEqual(p_a_c.revisionId, "cafe1234") + + def test_reuses_gitlinks_read_for_an_earlier_level(self) -> None: + p_a = self._parent_with_submodules(b="beef1234", c="cafe1234") + p_a_b = self._submodule(p_a, "b") + p_a_c = self._submodule(p_a, "c") + submodule_revisions = {} + + sync._RefreshDerivedRevisions([p_a_b], submodule_revisions) + sync._RefreshDerivedRevisions([p_a_c], submodule_revisions) + + p_a.GetSubmoduleRevisions.assert_called_once_with() + self.assertEqual(p_a_c.revisionId, "cafe1234") + + def test_tells_apart_projects_with_the_same_path(self) -> None: + # Paths are relative to their own (sub)manifest, so two projects can + # share one. + first = self._parent_with_submodules(b="beef1234") + second = self._parent_with_submodules(b="cafe1234") + first_sub = self._submodule(first, "b") + second_sub = self._submodule(second, "b") + submodule_revisions = {} + + sync._RefreshDerivedRevisions([first_sub], submodule_revisions) + sync._RefreshDerivedRevisions([second_sub], submodule_revisions) + + self.assertEqual(first_sub.revisionId, "beef1234") + self.assertEqual(second_sub.revisionId, "cafe1234") + + def test_ignores_projects_from_the_manifest(self) -> None: + p_a = self._parent_with_submodules() + + sync._RefreshDerivedRevisions([p_a]) + + p_a.GetSubmoduleRevisions.assert_not_called() + + def test_keeps_submodules_of_an_unreadable_parent(self) -> None: + p_a = FakeProject("a") + p_a.GetSubmoduleRevisions = mock.Mock(return_value=None) + p_a_b = FakeProject( + "a/b", + parent=p_a, + is_derived=True, + revisionId="stale", + gitlink_path="b", + ) + + sync._RefreshDerivedRevisions([p_a, p_a_b]) + + self.assertEqual(p_a_b.revisionId, "stale") + + class Chunksize(unittest.TestCase): """Tests for _chunksize.""" @@ -1158,6 +1253,62 @@ class InterleavedSyncTest(unittest.TestCase): execute_mock.assert_called_once() + def test_interleaved_refreshes_submodule_revision(self) -> None: + """Test submodules are synced at the revision of the fetched parent.""" + 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", + ) + all_projects = [self.projA, submodule] + mock.patch.object( + self.cmd, "GetProjects", return_value=all_projects + ).start() + + self.projA.GetSubmoduleRevisions = mock.Mock( + return_value={"sub": "fetched"} + ) + + 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, project.revisionId)) + 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, + all_projects, + {}, + ) + + self.assertIn(("projA/sub", "fetched"), synced) + 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"])