sync: check out submodules at the parent's new revision Interleaved sync processes projects in hierarchical levels, so a submodule is only handled after the project holding its gitlink has been fetched and checked out. Its revision, however, is still the one read before that parent was fetched, so the submodule is synced to the revision of the previous sync and only catches up on the next one. Read the gitlinks again right before a level is dispatched. The parent is done by then, so its submodules are fetched and checked out at the revision the parent now points at. Sibling submodules are spread over several levels, so the gitlinks read for one level are carried over to the next ones and every project is only read once per pass. Bug: 550074864 Change-Id: I30395b8a16a9154f60972e1f408a066af64ba77c Signed-off-by: kimhappy <hwanhee.kim@laplacian.cc> Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/621662 Reviewed-by: Gavin Mak <gavinmak@google.com> Reviewed-by: Brian Gan <brgan@google.com>
diff --git a/subcmds/sync.py b/subcmds/sync.py index 6a88530..af12399 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py
@@ -28,7 +28,7 @@ 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 @@ 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 @@ # 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 bc0a827..0e51dfe 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py
@@ -19,6 +19,7 @@ import shutil import tempfile import time +from typing import List, Optional import unittest from unittest import mock @@ -491,12 +492,24 @@ 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 @@ 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 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 @@ 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"])