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>

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
                                 )

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"])