sync: tell apart same-path submanifest projects

Interleaved sync remembers which projects are done and which ones are
still pending by Project.relpath, and lists failing projects by it as
well. That path is relative to the project's own (sub)manifest though,
so once several manifests are synced together two projects can share
it, and the sets no longer tell them apart.

The damage is done when a project turns up in a later pass, e.g. a
submodule that is only derived once its parent has been synced: its
path is already recorded as finished, so it is left out of every pass
that follows and never synced, while sync still reports success. With
an outer manifest holding

  <project name="a" path="a" sync-s="true"/>

where a carries a submodule at b, and a submanifest at sub/ holding

  <project name="sub-ab" path="a/b"/>

a fresh `repo sync` checks out sub/a/b but never a/b. Failing projects
are reported under the same ambiguous path, and the stall detection
merges them too.

Use RelPath(local=opt.this_manifest_only) instead, which is unique
within the set of projects being synced and is what GetProjects() and
the other subcommands already use.

Spotted during the review of I30395b8a16a9154f60972e1f408a066af64ba77c.

Change-Id: I5fa694b968774667be6060d9722cb57acbd2b579
Signed-off-by: kimhappy <hwanhee.kim@laplacian.cc>
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/622641
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Brian Gan <brgan@google.com>

2 files changed, 120 insertions, 6 deletions

diff --git a/subcmds/sync.py b/subcmds/sync.py
index bedc1f1b9..d88ec883e 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -301,7 +301,8 @@ class _SyncResult(NamedTuple):
 
     Attributes:
       project_index (int): The index of the project in the shared list.
-      relpath (str): The project's relative path from the repo client top.
+      relpath (str): The project's path relative to the tree being synced.
+          Unlike Project.relpath, it is unique across submanifests.
       remote_fetched (bool): True if the remote was actually queried.
       fetch_success (bool): True if the fetch operation was successful.
       fetch_errors (List[Exception]): The Exceptions from a failed fetch.
@@ -2869,7 +2870,7 @@ later is required to fix a server side protocol bug.
 
         return _SyncResult(
             project_index=project_index,
-            relpath=project.relpath,
+            relpath=project.RelPath(local=opt.this_manifest_only),
             fetch_success=fetch_success,
             remote_fetched=remote_fetched,
             checkout_success=checkout_success,
@@ -3017,6 +3018,10 @@ later is required to fix a server side protocol bug.
         self._interleaved_err_checkout = False
         self._interleaved_err_checkout_results = []
 
+        # Project.relpath is relative to its own (sub)manifest, so it does not
+        # tell apart projects of different manifests being synced together.
+        _RelPath = lambda p: p.RelPath(local=opt.this_manifest_only)
+
         err_event = multiprocessing.Event()
         finished_relpaths = set()
         project_list = list(all_projects)
@@ -3053,13 +3058,13 @@ later is required to fix a server side protocol bug.
                             projects_to_sync = [
                                 p
                                 for p in project_list
-                                if p.relpath not in finished_relpaths
+                                if _RelPath(p) not in finished_relpaths
                             ]
                             if not projects_to_sync:
                                 break
 
                             pending_relpaths = {
-                                p.relpath for p in projects_to_sync
+                                _RelPath(p) for p in projects_to_sync
                             }
                             if previously_pending_relpaths == pending_relpaths:
                                 stalled_projects_str = "\n".join(
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index b921069d0..e929f22b9 100644
--- a/tests/test_subcmds_sync.py
+++ b/tests/test_subcmds_sync.py
@@ -14,6 +14,7 @@
 """Unittests for the subcmds/sync.py module."""
 
 import json
+import optparse
 import os
 from pathlib import Path
 import shutil
@@ -501,8 +502,10 @@ class FakeProject:
         is_derived: bool = False,
         revisionId: Optional[str] = None,
         gitlink_path: Optional[str] = None,
+        path_prefix: str = "",
     ) -> None:
         self.relpath = relpath
+        self.path_prefix = path_prefix
         self.name = name or relpath
         self.objdir = objdir or relpath
         self.worktree = relpath
@@ -528,8 +531,10 @@ class FakeProject:
         self.revisionExpr = revisionExpr
         self.revisionId = revisionId or revisionExpr
 
-    def RelPath(self, local=None):
-        return self.relpath
+    def RelPath(self, local: bool = True) -> str:
+        if local:
+            return self.relpath
+        return os.path.join(self.path_prefix, self.relpath)
 
     def __str__(self):
         return f"project: {self.relpath}"
@@ -1444,6 +1449,93 @@ class InterleavedSyncTest(unittest.TestCase):
 
         self.assertEqual(synced, ["projA"])
 
+    def _make_syncable(self, project: FakeProject) -> FakeProject:
+        project.Sync_NetworkHalf = mock.Mock(
+            return_value=SyncNetworkHalfResult(error=None, remote_fetched=True)
+        )
+        project.Sync_LocalHalf = mock.Mock()
+        return project
+
+    def _run_interleaved(
+        self,
+        opt: optparse.Values,
+        initial_projects: List[FakeProject],
+        reloaded_projects: List[FakeProject],
+    ) -> None:
+        """Run _SyncInterleaved with the real workers and callback.
+
+        |initial_projects| make up the first pass, |reloaded_projects| every
+        later one, the way reloading the manifest between passes does.
+        """
+        mock.patch.object(
+            self.cmd, "GetProjects", return_value=reloaded_projects
+        ).start()
+        mock.patch.object(self.cmd, "event_log").start()
+
+        def execute_side_effect(
+            jobs: int,
+            target: object,
+            work_items: List[List[int]],
+            **kwargs: object,
+        ) -> bool:
+            results = [target(item) for item in work_items]
+            return kwargs["callback"](None, kwargs["output"], results)
+
+        mock.patch.object(
+            self.cmd, "ExecuteInParallel", side_effect=execute_side_effect
+        ).start()
+
+        with mock.patch("subcmds.sync.SyncBuffer") as mock_sync_buffer:
+            mock_sync_buffer.return_value.Finish.return_value = True
+            mock_sync_buffer.return_value.errors = []
+            self.cmd._SyncInterleaved(
+                opt,
+                [],
+                [],
+                self.manifest,
+                self.manifest.manifestProject,
+                initial_projects,
+                {},
+            )
+
+    def test_interleaved_syncs_same_path_projects_of_every_manifest(
+        self,
+    ) -> None:
+        """Test a project is not skipped because another shares its path."""
+        opt = self._get_opts(["--interleaved", "-j4"])
+        outer = self._make_syncable(
+            FakeProject("foo", name="outer", objdir="a")
+        )
+        sub = self._make_syncable(
+            FakeProject("foo", name="sub", objdir="b", path_prefix="sub")
+        )
+
+        # |sub| is only discovered once the manifest is reloaded after the
+        # first pass has synced |outer|.
+        self._run_interleaved(opt, [outer], [outer, sub])
+
+        outer.Sync_LocalHalf.assert_called_once()
+        sub.Sync_LocalHalf.assert_called_once()
+
+    def test_interleaved_reports_failures_by_a_unique_path(self) -> None:
+        """Test failing projects are listed by a path that is theirs alone."""
+        opt = self._get_opts(["--interleaved", "-j4"])
+        outer = self._make_syncable(
+            FakeProject("foo", name="outer", objdir="a")
+        )
+        sub = self._make_syncable(
+            FakeProject("foo", name="sub", objdir="b", path_prefix="sub")
+        )
+        sub.Sync_LocalHalf.side_effect = GitError("checkout failed")
+        self.cmd.git_event_log = mock.MagicMock()
+
+        with self.assertRaises(sync.SyncError):
+            self._run_interleaved(opt, [outer, sub], [outer, sub])
+
+        self.assertEqual(
+            self.cmd._interleaved_err_checkout_results, ["sub/foo"]
+        )
+
     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"])
@@ -1538,6 +1630,23 @@ class InterleavedSyncTest(unittest.TestCase):
             project.Sync_NetworkHalf.assert_called_once()
             project.Sync_LocalHalf.assert_called_once()
 
+    def test_worker_reports_a_path_unique_across_manifests(self) -> None:
+        """Test _SyncResult.relpath tells apart same-path projects."""
+        project = FakeProject("foo", objdir="objA", path_prefix="sub")
+        self._make_syncable(project)
+        self.mock_context["projects"] = [project]
+
+        for this_manifest_only, expected in ((False, "sub/foo"), (True, "foo")):
+            with self.subTest(this_manifest_only=this_manifest_only):
+                opt = self._get_opts()
+                opt.this_manifest_only = this_manifest_only
+                with mock.patch("subcmds.sync.SyncBuffer") as mock_sync_buffer:
+                    mock_sync_buffer.return_value.Finish.return_value = True
+                    mock_sync_buffer.return_value.errors = []
+                    result_obj = self.cmd._SyncProjectList(opt, [0])
+
+                self.assertEqual(result_obj.results[0].relpath, expected)
+
     def test_worker_fetch_fails(self):
         """Test _SyncProjectList with a failed fetch."""
         opt = self._get_opts()