sync: fetch submodules after their parent repository Phased sync fetches every project at once, so a submodule is fetched at the gitlink that was read before its parent was fetched. With -c that stale revision is already present, the fetch is skipped as an immutable revision, and the checkout then fails on the revision the reloaded manifest resolved: error: Cannot checkout a/b fatal: bad object 5556dad99da5df7e53303c5732aacc8782493e03 Fetch in batches, holding a submodule back until the project holding its gitlink is fetched, and read the gitlinks again between batches. A manifest without submodules keeps its single batch. Bug: 550074864 Change-Id: I04c2eb1ca81ab25b4df2c03c5dc048d2e4ecdcb8 Signed-off-by: kimhappy <hwanhee.kim@laplacian.cc> Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/621663 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 af12399..e713300 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py
@@ -156,6 +156,26 @@ return res +def _ParentFirstBatches(projects: List[Project]) -> List[List[Project]]: + """Group |projects| so that a parent is fetched before its submodules. + + A discovered submodule can only be fetched at the right revision once the + project holding its gitlink has been fetched, so it is held back to a later + batch than its parent. Projects that are not discovered submodules all end + up in the first batch, which keeps manifests without submodules on a single + batch. + """ + batches = collections.defaultdict(list) + for project in projects: + depth = 0 + ancestor = project + while ancestor.Derived and ancestor.parent: + depth += 1 + ancestor = ancestor.parent + batches[depth].append(project) + return [batches[depth] for depth in sorted(batches)] + + def _RefreshDerivedRevisions( projects: List[Project], submodule_revisions: Optional[Dict[Project, Dict[str, str]]] = None, @@ -1107,6 +1127,38 @@ return _FetchResult(ret, fetched) + def _FetchParentFirst( + self, + projects: List[Project], + opt: optparse.Values, + err_event: _threading.Event, + ssh_proxy: ssh.ProxyManager, + errors: List[Exception], + ) -> _FetchResult: + """Fetch |projects|, holding submodules back until their parent is done. + + Args: + projects: Projects to fetch. + opt: Program options returned from optparse. See _Options(). + err_event: Whether an error was hit while processing. + ssh_proxy: SSH manager for clients & masters. + errors: A list to accumulate errors. + + Returns: + _FetchResult for all the batches combined. + """ + success = True + fetched = set() + for batch in _ParentFirstBatches(projects): + _RefreshDerivedRevisions(batch) + batch.sort(key=self._fetch_times.Get, reverse=True) + result = self._Fetch(batch, opt, err_event, ssh_proxy, errors) + success = success and result.success + fetched.update(result.projects) + if not success and opt.fail_fast: + break + return _FetchResult(success, fetched) + def _FetchMain( self, opt, args, all_projects, err_event, ssh_proxy, manifest, errors ): @@ -1123,12 +1175,10 @@ Returns: List of all projects that should be checked out. """ - to_fetch = [] - to_fetch.extend(all_projects) - to_fetch.sort(key=self._fetch_times.Get, reverse=True) - try: - result = self._Fetch(to_fetch, opt, err_event, ssh_proxy, errors) + result = self._FetchParentFirst( + all_projects, opt, err_event, ssh_proxy, errors + ) success = result.success fetched = result.projects if not success: @@ -1171,7 +1221,9 @@ if previously_missing_set == missing_set: break previously_missing_set = missing_set - result = self._Fetch(missing, opt, err_event, ssh_proxy, errors) + result = self._FetchParentFirst( + missing, opt, err_event, ssh_proxy, errors + ) success = result.success new_fetched = result.projects if not success:
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index 0e51dfe..e854a19 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py
@@ -19,7 +19,7 @@ import shutil import tempfile import time -from typing import List, Optional +from typing import Dict, List, Optional import unittest from unittest import mock @@ -637,6 +637,21 @@ ) +class ParentFirstBatches(unittest.TestCase): + def test_no_submodules(self) -> None: + p_a = FakeProject("a") + p_a_b = FakeProject("a/b") + out = sync._ParentFirstBatches([p_a, p_a_b]) + self.assertEqual(out, [[p_a, p_a_b]]) + + def test_submodules_follow_their_parent(self) -> None: + p_a = FakeProject("a") + p_a_b = FakeProject("a/b", parent=p_a, is_derived=True) + p_a_b_c = FakeProject("a/b/c", parent=p_a_b, is_derived=True) + out = sync._ParentFirstBatches([p_a_b_c, p_a, p_a_b]) + self.assertEqual(out, [[p_a], [p_a_b], [p_a_b_c]]) + + class RefreshDerivedRevisions(unittest.TestCase): def _parent_with_submodules(self, **gitlinks: str) -> FakeProject: p_a = FakeProject("a") @@ -709,6 +724,46 @@ self.assertEqual(p_a_b.revisionId, "stale") +class FetchParentFirst(unittest.TestCase): + def test_submodules_are_fetched_after_their_parent(self) -> None: + cmd = sync.Sync() + cmd._fetch_times = mock.Mock() + cmd._fetch_times.Get = mock.Mock(return_value=0) + + calls = [] + p_a = FakeProject("a") + + def fake_read() -> Dict[str, str]: + calls.append(("read gitlinks of", p_a.relpath)) + return {"b": "beef1234"} + + p_a.GetSubmoduleRevisions = mock.Mock(side_effect=fake_read) + p_a_b = FakeProject( + "a/b", parent=p_a, is_derived=True, gitlink_path="b" + ) + + def fake_fetch( + projects: List[FakeProject], *_args: object + ) -> sync._FetchResult: + calls.append(("fetch", [p.relpath for p in projects])) + return sync._FetchResult(True, {p.objdir for p in projects}) + + opt = mock.Mock(fail_fast=False) + with mock.patch.object(cmd, "_Fetch", side_effect=fake_fetch): + result = cmd._FetchParentFirst([p_a_b, p_a], opt, None, None, []) + + self.assertTrue(result.success) + self.assertEqual(result.projects, {"a", "a/b"}) + self.assertEqual( + calls, + [ + ("fetch", ["a"]), + ("read gitlinks of", "a"), + ("fetch", ["a/b"]), + ], + ) + + class Chunksize(unittest.TestCase): """Tests for _chunksize."""