sync: don't let --nmu affect smart sync

--no-manifest-update has always meant "use the existing manifest
checkout as-is": it skips updating .repo/manifests and nothing else.
Smart sync pins do not come from that checkout but from the manifest
server, and an explicit `-s --nmu` already works that way.

The manifest-driven default smart sync (sync-smartsync="true") instead
treated --nmu like -l: it disabled the implicit smart sync and kept an
existing smart_sync_override.xml. So on such manifests `repo sync --nmu`
fetched and checked out the previous pins (or ToT if there were none)
instead of doing what `repo sync` does minus the manifest checkout
update.

Make --nmu orthogonal to smart sync again: it no longer disables the
default smart sync, and it no longer keeps the override. Both have to
change together; otherwise a failed implicit smart sync under --nmu
would fall back to ToT while leaving the override on disk. Only -l,
which skips fetching, keeps the override.

Bug: 569927750
Change-Id: I33730363df485d72cd86ae99bf5eb8399b58e81a
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/641264
Reviewed-by: Brian Gan <brgan@google.com>
Commit-Queue: Rahul Yadav <yadavrah@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Tested-by: Rahul Yadav <yadavrah@google.com>

docs/smart-sync.md

@@ -158,11 +158,15 @@ sync, as before. Options that specify an explicit target or manifest source
 (such as `-t`/`--smart-tag`, `-m`/`--manifest-name` or
 `--superproject-revision`) will also disable the default Smart Sync behavior.
 
-With `-l`/`--local-only` or `--no-manifest-update`, the default Smart Sync does
-not contact the manifest server. Instead, repo keeps using the
-`smart_sync_override.xml` from the last Smart Sync, if there is one. Pass
-`--no-smart-sync` to discard it and use the default manifest instead. These
-options do not change the behavior of an explicit `-s`/`--smart-sync`.
+With `-l`/`--local-only`, the default Smart Sync does not contact the manifest
+server. Instead, repo keeps using the `smart_sync_override.xml` from the last
+Smart Sync, if there is one. Pass `--no-smart-sync` to discard it and use the
+default manifest instead. These options do not change the behavior of an
+explicit `-s`/`--smart-sync`.
+
+`--no-manifest-update` only skips updating the manifest checkout. It does not
+affect Smart Sync: the default Smart Sync still runs, and an existing
+`smart_sync_override.xml` is replaced or removed as in a regular sync.
 
 ### --smart-sync / -s
 

subcmds/sync.py

@@ -2432,7 +2432,6 @@ later is required to fix a server side protocol bug.
             and not opt.manifest_name
             and not opt.superproject_revision
             and not opt.local_only
-            and opt.mp_update
             and getattr(manifest.default, "sync_smartsync", False) is True
         )
         if opt.smart_sync is None:
@@ -2459,12 +2458,12 @@ later is required to fix a server side protocol bug.
         if opt.clone_bundle is None:
             opt.clone_bundle = manifest.CloneBundle
 
-        # An onboarded client run with -l or --no-manifest-update keeps the
-        # smart sync manifest it last synced to instead of contacting the
-        # manifest server. Computed before _ResolveSmartSyncOption, which
-        # makes an unset opt.smart_sync indistinguishable from --no-smart-sync.
+        # An onboarded client run with -l keeps the smart sync manifest it last
+        # synced to instead of contacting the manifest server. Computed before
+        # _ResolveSmartSyncOption, which makes an unset opt.smart_sync
+        # indistinguishable from --no-smart-sync.
         reuse_smart_sync_override = (
-            (opt.local_only or not opt.mp_update)
+            opt.local_only
             and opt.smart_sync is None
             and not opt.smart_tag
             and not opt.manifest_name

tests/test_subcmds_sync.py

@@ -408,8 +408,9 @@ def test_cli_jobs_sync_j_max(
         (["--manifest-name=other.xml"], True, False, False),
         (["-l"], True, False, False),
         (["--local-only"], True, False, False),
-        (["--nmu"], True, False, False),
-        (["--no-manifest-update"], True, False, False),
+        (["--nmu"], True, True, True),
+        (["--no-manifest-update"], True, True, True),
+        (["--nmu"], False, False, False),
         (["--superproject-revision=abc"], True, False, False),
         (["-s", "-l"], True, True, False),
         (["-s", "--nmu"], True, True, False),
@@ -449,7 +450,8 @@ def test_cli_smart_sync(
             sync.SmartSyncError,
         ),
         (["-l", "-u", "user", "-p", "pass"], True, sync.SmartSyncError),
-        (["--nmu", "-u", "user", "-p", "pass"], True, sync.SmartSyncError),
+        (["--nmu", "-u", "user", "-p", "pass"], True, None),
+        (["--nmu", "-u", "user", "-p", "pass"], False, sync.SmartSyncError),
         (["-s", "-l", "-u", "user", "-p", "pass"], True, None),
         (["-u", "user"], False, SystemExit),
         (["-p", "pass"], False, SystemExit),
@@ -1667,7 +1669,9 @@ class SyncCommand(unittest.TestCase):
                 self.cmd.Execute(self.opt, [])
 
     def _ExecuteWithSmartSyncOverride(
-        self, reload_error: Optional[Exception] = None
+        self,
+        reload_error: Optional[Exception] = None,
+        setup_error: Optional[Exception] = None,
     ) -> Tuple[str, mock.MagicMock, mock.MagicMock, mock.MagicMock]:
         """Run Execute with an existing smart sync override on disk.
 
@@ -1694,7 +1698,9 @@ class SyncCommand(unittest.TestCase):
             ):
                 stack.enter_context(mock.patch.object(self.cmd, name))
             setup = stack.enter_context(
-                mock.patch.object(self.cmd, "_SmartSyncSetup")
+                mock.patch.object(
+                    self.cmd, "_SmartSyncSetup", side_effect=setup_error
+                )
             )
             reload = stack.enter_context(
                 mock.patch.object(
@@ -1708,12 +1714,12 @@ class SyncCommand(unittest.TestCase):
         return override_path, setup, reload, update_mps
 
     def test_smart_sync_override_reused_offline(self) -> None:
-        """Ensure -l/--nmu reuse an onboarded client's smart sync manifest."""
-        for local_only, mp_update in ((True, True), (False, False)):
-            with self.subTest(local_only=local_only, mp_update=mp_update):
+        """Ensure -l reuses an onboarded client's smart sync manifest."""
+        for mp_update in (True, False):
+            with self.subTest(mp_update=mp_update):
                 self.manifest.default.sync_smartsync = True
                 self.opt.smart_sync = None
-                self.opt.local_only = local_only
+                self.opt.local_only = True
                 self.opt.mp_update = mp_update
                 path, setup, reload, update_mps = (
                     self._ExecuteWithSmartSyncOverride()
@@ -1728,6 +1734,32 @@ class SyncCommand(unittest.TestCase):
                         update_mps.call_args[0][2], "smart_sync_override.xml"
                     )
 
+    def test_no_manifest_update_keeps_default_smart_sync(self) -> None:
+        """Ensure --nmu still runs the manifest-driven default smart sync."""
+        self.manifest.default.sync_smartsync = True
+        self.opt.smart_sync = None
+        self.opt.mp_update = False
+        _, setup, _, update_mps = self._ExecuteWithSmartSyncOverride()
+        setup.assert_called_once()
+        update_mps.assert_not_called()
+
+    def test_no_manifest_update_does_not_reuse_override(self) -> None:
+        """Ensure --nmu handles an existing override like a regular sync."""
+        cases = (
+            ("manifest without sync-smartsync", False, None),
+            ("failed default smart sync", True, sync.SmartSyncError("down")),
+        )
+        for name, sync_smartsync, setup_error in cases:
+            with self.subTest(name):
+                self.manifest.default.sync_smartsync = sync_smartsync
+                self.opt.smart_sync = None
+                self.opt.mp_update = False
+                path, _, reload, _ = self._ExecuteWithSmartSyncOverride(
+                    setup_error=setup_error
+                )
+                reload.assert_not_called()
+                self.assertFalse(os.path.isfile(path))
+
     def test_smart_sync_override_removed_offline(self) -> None:
         """Ensure -l still drops the override when reuse does not apply."""
         cases = (