sync: keep --superproject-revision across manifest reloads

When --superproject-revision moves `.repo/manifests`, the manifest is
reloaded, discarding the pinned superproject. _UpdateProjectsRevisionId
then read gitlinks from the superproject's default branch instead,
pinning failed, and projects synced unpinned.

Apply the revision in _ConfigureSuperproject so it is re-applied after
every reload.

Verified with https://paste.googleplex.com/5889974172516352

Bug: 571245807
Change-Id: I4d782d229b128c0f15081d11b722d8901709458c
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/641743
Reviewed-by: Gavin Mak <gavinmak@google.com>
Tested-by: Brian Gan <brgan@google.com>
Commit-Queue: Brian Gan <brgan@google.com>

subcmds/sync.py

@@ -827,13 +827,42 @@ later is required to fix a server side protocol bug.
             or opt.current_branch_only
         )
 
+    def _GetSuperprojectRevision(
+        self, opt: optparse.Values, manifest: XmlManifest
+    ) -> Optional[str]:
+        """Returns the --superproject-revision to apply to manifest.
+
+        The option only applies to the outer manifest.
+
+        Args:
+            opt: Program options returned from optparse.  See _Options().
+            manifest: The manifest to check.
+
+        Returns:
+            The requested superproject revision, or None if manifest's
+            superproject should not be pinned.
+        """
+        if manifest == self.outer_manifest:
+            return opt.superproject_revision
+        return None
+
     def _ConfigureSuperproject(
-        self,
-        opt: optparse.Values,
-        manifest,
-        revision: Optional[str] = None,
+        self, opt: optparse.Values, manifest: XmlManifest
     ) -> bool:
-        """Configure superproject with options."""
+        """Configure superproject with options.
+
+        The configuration lives on the manifest's Superproject object, which
+        is discarded whenever the manifest is unloaded (see _ReloadManifest
+        and XmlManifest.Unload). Callers must therefore configure the
+        superproject again after every manifest reload.
+
+        Args:
+            opt: Program options returned from optparse.  See _Options().
+            manifest: The manifest whose superproject should be configured.
+
+        Returns:
+            Whether superproject messages should be printed.
+        """
         if not manifest.superproject:
             return False
         manifest.superproject.SetQuiet(not opt.verbose)
@@ -841,6 +870,7 @@ later is required to fix a server side protocol bug.
             opt.use_superproject, manifest
         )
         manifest.superproject.SetPrintMessages(print_messages)
+        revision = self._GetSuperprojectRevision(opt, manifest)
         if revision:
             manifest.superproject.SetRevisionId(revision)
         return print_messages
@@ -2171,7 +2201,7 @@ later is required to fix a server side protocol bug.
             mp: the manifestProject to query.
             manifest_name: Manifest file to be reloaded.
         """
-        if opt.superproject_revision and mp.manifest == self.outer_manifest:
+        if self._GetSuperprojectRevision(opt, mp.manifest):
             self._SyncToSuperprojectRev(
                 opt, mp.manifest, mp, manifest_name, errors
             )
@@ -2383,18 +2413,19 @@ later is required to fix a server side protocol bug.
     def _SyncToSuperprojectRev(
         self,
         opt: optparse.Values,
-        manifest,
+        manifest: XmlManifest,
         mp: Project,
         manifest_name: Optional[str],
         errors: List[Exception],
     ) -> None:
-        """Sync to a specific superproject commit."""
+        """Sync to a specific superproject commit.
+
+        Only valid for the outer manifest; see _GetSuperprojectRevision.
+        """
         if not manifest.superproject:
             raise SyncError("superproject not defined in manifest")
 
-        self._ConfigureSuperproject(
-            opt, manifest, revision=opt.superproject_revision
-        )
+        self._ConfigureSuperproject(opt, manifest)
 
         sync_result = manifest.superproject.Sync(self.git_event_log)
         if not sync_result.success:

tests/test_subcmds_sync.py

@@ -254,6 +254,66 @@ def test_sync_update_projects_revision_id_populates_logging_data(
     }
 
 
+@pytest.mark.parametrize(
+    "superproject_revision, is_outer_manifest, expected_revision",
+    [
+        ("deadbeef", True, "deadbeef"),
+        ("deadbeef", False, None),
+        (None, True, None),
+    ],
+    ids=["outer_manifest", "not_outer_manifest", "no_superproject_revision"],
+)
+def test_sync_update_projects_revision_id_pins_superproject_revision(
+    tmp_path: Path,
+    superproject_revision: Optional[str],
+    is_outer_manifest: bool,
+    expected_revision: Optional[str],
+) -> None:
+    """Test that --superproject-revision is applied to a reloaded manifest.
+
+    _SyncToSuperprojectRev pins the superproject, but updating the manifest
+    project unloads the manifest, so _UpdateProjectsRevisionId must pin the
+    freshly loaded superproject again before using it.
+    """
+    manifest = _create_manifest_with_groups(tmp_path)
+    cmd = sync.Sync()
+    cmd.manifest = manifest
+    cmd.outer_manifest = manifest if is_outer_manifest else mock.MagicMock()
+
+    # Simulate the state after a manifest reload: a new, unpinned superproject.
+    superproject = mock.MagicMock()
+    superproject.UpdateProjectsRevisionId.return_value = mock.MagicMock(
+        manifest_path=None, fatal=False
+    )
+    manifest._superproject = superproject
+
+    opts, args = cmd.OptionParser.parse_args([])
+    opts.verbose = False
+    opts.this_manifest_only = True
+    opts.local_only = False
+    opts.superproject_revision = superproject_revision
+
+    with mock.patch.object(
+        cmd, "ManifestList", return_value=[manifest]
+    ), mock.patch.object(
+        sync.git_superproject, "UseSuperproject", return_value=True
+    ), mock.patch.object(
+        sync.git_superproject, "PrintMessages", return_value=False
+    ):
+        cmd._UpdateProjectsRevisionId(opts, args, {}, manifest)
+
+    if expected_revision is not None:
+        superproject.SetRevisionId.assert_called_once_with(expected_revision)
+        # The pin must be in place before the superproject is used.
+        method_calls = [name for name, _, _ in superproject.mock_calls]
+        assert method_calls.index("SetRevisionId") < method_calls.index(
+            "UpdateProjectsRevisionId"
+        )
+    else:
+        superproject.SetRevisionId.assert_not_called()
+    superproject.UpdateProjectsRevisionId.assert_called_once()
+
+
 def test_sync_update_projects_revision_id_logs_without_superproject_tag(
     tmp_path: Path,
 ) -> None: