sync: keep smart sync override with -l

Splitting a sync into `repo sync -n` followed by `repo sync -l` should
end up in the same state as a single `repo sync`. With smart sync it
does not always: -n fetches the revisions pinned by the smart sync
manifest and saves it as smart_sync_override.xml, but the following -l
deletes that override and checks out ToT instead, unless the manifest
sets sync-smartsync="true". So `repo sync -s -n` + `repo sync -l` lands
on ToT, while `repo sync -s` lands on the pinned revisions.

Without an explicit -s or -t, -l does not contact the manifest server,
so it cannot choose new pins itself; the only consistent choice is to
keep the override the previous -n left behind. Do that regardless of
the manifest default.

Syncs that choose a manifest themselves still replace or remove the
override as before: any sync without -l, and -l combined with -s, -t,
-m, --superproject-revision or --no-smart-sync. Smart sync stays
non-sticky for regular syncs, and `repo sync -l --no-smart-sync` still
discards the override to check out ToT.

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

docs/smart-sync.md

@@ -138,6 +138,20 @@ copy into `.repo/manifests/smart_sync_override.xml` so users can examine it.
 The next time `repo sync` is run, this file is automatically replaced or removed
 based on the current set of options.
 
+With `-l`/`--local-only` and no explicit `-s`/`--smart-sync` or
+`-t`/`--smart-tag`, repo does not contact the manifest server. Instead, it keeps
+using the existing `smart_sync_override.xml`, if there is one, whether it came
+from an explicit `-s`/`--smart-sync` or from the manifest's `sync-smartsync`
+default. This way, `repo sync -n` followed by `repo sync -l` ends up on the same
+revisions as the equivalent single `repo sync` (e.g. `repo sync -s -n` +
+`repo sync -l` matches `repo sync -s`). Pass `--no-smart-sync` to discard it and
+use the default manifest instead.
+
+`--no-manifest-update` only skips updating the manifest checkout. It does not
+affect Smart Sync: the default Smart Sync from `sync-smartsync` (see below)
+still runs, and an existing `smart_sync_override.xml` is replaced or removed as
+in a regular sync.
+
 ### sync-smartsync (Manifest Attribute)
 
 The manifest can enable Smart Sync by default for all `repo sync` invocations
@@ -158,16 +172,6 @@ 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`, 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
 
 Explicitly enables Smart Sync. Repo will call

subcmds/sync.py

@@ -2499,17 +2499,18 @@ 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 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.
+        # Without an explicit smart sync option, -l does not contact the
+        # manifest server, so it keeps the smart sync manifest the client last
+        # synced to, whether that came from -s or from the manifest's
+        # sync-smartsync default. This keeps `sync -n` + `sync -l` equivalent
+        # to a single sync. Computed before _ResolveSmartSyncOption, which
+        # makes an unset opt.smart_sync indistinguishable from --no-smart-sync.
         reuse_smart_sync_override = (
             opt.local_only
             and opt.smart_sync is None
             and not opt.smart_tag
             and not opt.manifest_name
             and not opt.superproject_revision
-            and getattr(manifest.default, "sync_smartsync", False) is True
             and os.path.isfile(smart_sync_manifest_path)
         )
 

tests/test_subcmds_sync.py

@@ -1774,25 +1774,34 @@ class SyncCommand(unittest.TestCase):
         return override_path, setup, reload, update_mps
 
     def test_smart_sync_override_reused_offline(self) -> None:
-        """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 = True
-                self.opt.mp_update = mp_update
-                path, setup, reload, update_mps = (
-                    self._ExecuteWithSmartSyncOverride()
-                )
-                setup.assert_not_called()
-                reload.assert_called_once_with(
-                    "smart_sync_override.xml", self.manifest
-                )
-                self.assertTrue(os.path.isfile(path))
-                if mp_update:
-                    self.assertEqual(
-                        update_mps.call_args[0][2], "smart_sync_override.xml"
+        """Ensure -l reuses an existing smart sync manifest.
+
+        This applies whether the override came from the manifest's
+        sync-smartsync default or from an explicit -s, so that `sync -n`
+        followed by `sync -l` matches a single sync.
+        """
+        for sync_smartsync in (True, False):
+            for mp_update in (True, False):
+                with self.subTest(
+                    sync_smartsync=sync_smartsync, mp_update=mp_update
+                ):
+                    self.manifest.default.sync_smartsync = sync_smartsync
+                    self.opt.smart_sync = None
+                    self.opt.local_only = True
+                    self.opt.mp_update = mp_update
+                    path, setup, reload, update_mps = (
+                        self._ExecuteWithSmartSyncOverride()
                     )
+                    setup.assert_not_called()
+                    reload.assert_called_once_with(
+                        "smart_sync_override.xml", self.manifest
+                    )
+                    self.assertTrue(os.path.isfile(path))
+                    if mp_update:
+                        self.assertEqual(
+                            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."""
@@ -1821,15 +1830,11 @@ class SyncCommand(unittest.TestCase):
                 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 = (
-            ("--no-smart-sync", False, True),
-            ("manifest without sync-smartsync", None, False),
-        )
-        for name, smart_sync, sync_smartsync in cases:
-            with self.subTest(name):
+        """Ensure -l --no-smart-sync still drops the override."""
+        for sync_smartsync in (True, False):
+            with self.subTest(sync_smartsync=sync_smartsync):
                 self.manifest.default.sync_smartsync = sync_smartsync
-                self.opt.smart_sync = smart_sync
+                self.opt.smart_sync = False
                 self.opt.local_only = True
                 path, setup, reload, _ = self._ExecuteWithSmartSyncOverride()
                 setup.assert_not_called()