From 3a6e25af7537181aae251951b8aabe7748c3aee5 Mon Sep 17 00:00:00 2001 From: kimhappy Date: Thu, 27 Aug 2026 03:04:10 +0900 Subject: [PATCH] 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 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/621662 Reviewed-by: Gavin Mak Reviewed-by: Brian Gan --- subcmds/sync.py | 47 ++++++++++- tests/test_subcmds_sync.py | 155 ++++++++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 3 deletions(-) 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"])