sync: keep local sync state paths relative to repo root

`LocalSyncState` uses `Project.relpath` to identify projects, but that
path is relative to each project's own manifest. Projects in different
manifests can have the same relpath, so their fetch and checkout
timestamps overwrite each other. This can make a later partial sync look
like a full sync.

Pruning has a related problem. It treats the stored paths as relative to
the current manifest's topdir, so when sync starts from the outermost
manifest it can look for submanifest projects in the wrong place and
remove valid state.

Use `Project.RelPath(local=False)` to identify projects in local sync
state and check stored paths from the repo root when pruning.

Add tests for same-relpath projects, partial-sync detection, and pruning
when sync starts from either the outermost manifest or a submanifest.

Change-Id: I24485c315f1277a14913443674245cedd20820fe
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/632421
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Mike Frysinger <vapier@google.com>
Tested-by: Victor Pushkarev <corvinus.v@gmail.com>
Commit-Queue: Victor Pushkarev <corvinus.v@gmail.com>

subcmds/sync.py

@@ -3365,13 +3365,13 @@ class LocalSyncState:
 
     def _Get(self, project, key):
         self._Load()
-        p = project.relpath
+        p = project.RelPath(local=False)
         if p not in self._state:
             return
         return self._state[p].get(key)
 
     def _Set(self, project, key):
-        p = project.relpath
+        p = project.RelPath(local=False)
         if p not in self._state:
             self._state[p] = {}
         self._state[p][key] = self._time
@@ -3399,8 +3399,9 @@ class LocalSyncState:
         if not self._state:
             return
         delete = set()
+        outer_topdir = self._manifest.outer_client.topdir
         for path in self._state:
-            gitdir = os.path.join(self._manifest.topdir, path, ".git")
+            gitdir = os.path.join(outer_topdir, path, ".git")
             if not os.path.exists(gitdir) or os.path.islink(gitdir):
                 delete.add(path)
         if not delete:

tests/test_subcmds_sync.py

@@ -338,8 +338,9 @@ class LocalSyncState(unittest.TestCase):
         self.manifest = mock.MagicMock(
             topdir=self.topdir,
             repodir=self.repodir,
-            repoProject=mock.MagicMock(relpath=".repo/repo"),
+            repoProject=FakeProject(".repo/repo"),
         )
+        self.manifest.outer_client = self.manifest
         self.state = self._new_state()
 
     def tearDown(self):
@@ -352,7 +353,7 @@ class LocalSyncState(unittest.TestCase):
 
     def test_set(self):
         """Times are set."""
-        p = mock.MagicMock(relpath="projA")
+        p = FakeProject("projA")
         self.state.SetFetchTime(p)
         self.state.SetCheckoutTime(p)
         self.assertEqual(self.state.GetFetchTime(p), self._TIME)
@@ -374,8 +375,8 @@ class LocalSyncState(unittest.TestCase):
 
         # Initialize state to read from the new file.
         self.state = self._new_state()
-        projA = mock.MagicMock(relpath="projA")
-        projB = mock.MagicMock(relpath="projB")
+        projA = FakeProject("projA")
+        projB = FakeProject("projB")
         self.assertEqual(self.state.GetFetchTime(projA), None)
         self.assertEqual(self.state.GetFetchTime(projB), 5)
         self.assertEqual(self.state.GetCheckoutTime(projB), 7)
@@ -386,9 +387,43 @@ class LocalSyncState(unittest.TestCase):
         self.assertEqual(self.state.GetFetchTime(projB), self._TIME)
         self.assertEqual(self.state.GetCheckoutTime(projB), 7)
 
+    def test_same_relpath_projects_keep_separate_state(self) -> None:
+        """Projects with the same relpath keep separate sync state."""
+        outer = FakeProject("proj")
+        child = FakeProject("proj", path_prefix="sub")
+
+        self.state.SetFetchTime(outer)
+        self.state.Save()
+
+        self.state = self._new_state(self._TIME + 1)
+        self.state.SetFetchTime(child)
+
+        self.assertEqual(self.state.GetFetchTime(outer), self._TIME)
+        self.assertEqual(
+            self.state.GetFetchTime(child),
+            self._TIME + 1,
+        )
+
+    def test_partial_sync_with_same_relpath_projects(self) -> None:
+        """Projects with the same relpath keep independent partial sync."""
+        outer = FakeProject("proj")
+        child = FakeProject("proj", path_prefix="sub")
+
+        for project in (outer, child):
+            self.state.SetFetchTime(project)
+            self.state.SetCheckoutTime(project)
+        self.state.Save()
+        self.assertFalse(self.state.IsPartiallySynced())
+
+        self.state = self._new_state(self._TIME + 1)
+        self.state.SetFetchTime(outer)
+        self.state.SetCheckoutTime(outer)
+
+        self.assertTrue(self.state.IsPartiallySynced())
+
     def test_save_to_file(self):
         """Data is saved under repodir."""
-        p = mock.MagicMock(relpath="projA")
+        p = FakeProject("projA")
         self.state.SetFetchTime(p)
         self.state.Save()
         self.assertEqual(
@@ -415,7 +450,7 @@ class LocalSyncState(unittest.TestCase):
 
         # Initialize state to read from the new file.
         self.state = self._new_state()
-        projB = mock.MagicMock(relpath="projB")
+        projB = FakeProject("projB")
         self.assertEqual(self.state.IsPartiallySynced(), False)
 
         self.state.SetFetchTime(projB)
@@ -424,7 +459,7 @@ class LocalSyncState(unittest.TestCase):
 
     def test_ignore_repo_project(self):
         """Sync data for repo project is ignored when checking partial sync."""
-        p = mock.MagicMock(relpath="projA")
+        p = FakeProject("projA")
         self.state.SetFetchTime(p)
         self.state.SetCheckoutTime(p)
         self.state.SetFetchTime(self.manifest.repoProject)
@@ -441,7 +476,7 @@ class LocalSyncState(unittest.TestCase):
 
     def test_nonexistent_project(self):
         """Unsaved projects don't have data."""
-        p = mock.MagicMock(relpath="projC")
+        p = FakeProject("projC")
         self.assertEqual(self.state.GetFetchTime(p), None)
         self.assertEqual(self.state.GetCheckoutTime(p), None)
 
@@ -466,8 +501,8 @@ class LocalSyncState(unittest.TestCase):
                 return False
             return True
 
-        projA = mock.MagicMock(relpath="projA")
-        projB = mock.MagicMock(relpath="projB")
+        projA = FakeProject("projA")
+        projB = FakeProject("projB")
         self.state = self._new_state()
         self.assertEqual(self.state.GetFetchTime(projA), 5)
         self.assertEqual(self.state.GetFetchTime(projB), 7)
@@ -479,6 +514,38 @@ class LocalSyncState(unittest.TestCase):
         self.assertIsNone(self.state.GetFetchTime(projA))
         self.assertEqual(self.state.GetFetchTime(projB), 7)
 
+    def test_prune_keeps_submanifest_project(self) -> None:
+        """Existing submanifest projects are not pruned."""
+        project = FakeProject("proj", path_prefix="sub")
+        os.makedirs(
+            os.path.join(self.topdir, "sub", "proj", ".git"),
+        )
+
+        self.state.SetFetchTime(project)
+        self.state.PruneRemovedProjects()
+
+        self.assertEqual(self.state.GetFetchTime(project), self._TIME)
+
+    def test_prune_from_submanifest_keeps_project(self) -> None:
+        """Existing projects are not pruned during a submanifest sync."""
+        child_topdir = os.path.join(self.topdir, "sub")
+        project = FakeProject("proj", path_prefix="sub")
+        os.makedirs(os.path.join(child_topdir, "proj", ".git"))
+
+        child_manifest = mock.MagicMock(
+            topdir=child_topdir,
+            repodir=self.repodir,
+            outer_client=self.manifest,
+        )
+
+        with mock.patch("time.time", return_value=self._TIME):
+            state = sync.LocalSyncState(child_manifest)
+
+        state.SetFetchTime(project)
+        state.PruneRemovedProjects()
+
+        self.assertEqual(state.GetFetchTime(project), self._TIME)
+
     def test_prune_removed_and_symlinked_projects(self):
         """Removed projects that still exists on disk as symlink are pruned."""
         with open(self.state._path, "w") as f:
@@ -503,8 +570,8 @@ class LocalSyncState(unittest.TestCase):
                 return True
             return False
 
-        projA = mock.MagicMock(relpath="projA")
-        projB = mock.MagicMock(relpath="projB")
+        projA = FakeProject("projA")
+        projB = FakeProject("projB")
         self.state = self._new_state()
         self.assertEqual(self.state.GetFetchTime(projA), 5)
         self.assertEqual(self.state.GetFetchTime(projB), 7)