sync: Share self-update logic between sync modes

The logic for checking for repo self-updates lives in _FetchMain, which
is part of the "phased" sync path.

Extract this logic into a new _UpdateRepoProject helper method. Call
this common helper from _ExecuteHelper before either sync mode begins,
so the repo self-update check is always performed.

Bug: 421935613
Change-Id: I9a804f43fbf6239c4146be446040be531f12fc8a
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/484041
Reviewed-by: Scott Lee <ddoman@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>
Tested-by: Gavin Mak <gavinmak@google.com>

2 files changed, 160 insertions, 10 deletions

diff --git a/subcmds/sync.py b/subcmds/sync.py
index e75a8154..b848d137 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -989,26 +989,16 @@ later is required to fix a server side protocol bug.
         Returns:
             List of all projects that should be checked out.
         """
-        rp = manifest.repoProject
-
         to_fetch = []
-        now = time.time()
-        if _ONE_DAY_S <= (now - rp.LastFetch):
-            to_fetch.append(rp)
         to_fetch.extend(all_projects)
         to_fetch.sort(key=self._fetch_times.Get, reverse=True)
 
         result = self._Fetch(to_fetch, opt, err_event, ssh_proxy, errors)
         success = result.success
         fetched = result.projects
-
         if not success:
             err_event.set()
 
-        # Call self update, unless requested not to
-        # TODO(b/42193561): Extract repo update logic to ExecuteHelper.
-        if os.environ.get("REPO_SKIP_SELF_UPDATE", "0") == "0":
-            _PostRepoFetch(rp, opt.repo_verify)
         if opt.network_only:
             # Bail out now; the rest touches the working tree.
             if err_event.is_set():
@@ -1369,6 +1359,61 @@ later is required to fix a server side protocol bug.
             t.join()
         pm.end()
 
+    def _UpdateRepoProject(self, opt, manifest, errors):
+        """Fetch the repo project and check for updates."""
+        if opt.local_only:
+            return
+
+        rp = manifest.repoProject
+        now = time.time()
+        # If we've fetched in the last day, don't bother fetching again.
+        if (now - rp.LastFetch) < _ONE_DAY_S:
+            return
+
+        with multiprocessing.Manager() as manager:
+            with ssh.ProxyManager(manager) as ssh_proxy:
+                ssh_proxy.sock()
+                start = time.time()
+                buf = TeeStringIO(sys.stdout if opt.verbose else None)
+                sync_result = rp.Sync_NetworkHalf(
+                    quiet=opt.quiet,
+                    verbose=opt.verbose,
+                    output_redir=buf,
+                    current_branch_only=self._GetCurrentBranchOnly(
+                        opt, manifest
+                    ),
+                    force_sync=opt.force_sync,
+                    clone_bundle=opt.clone_bundle,
+                    tags=opt.tags,
+                    archive=manifest.IsArchive,
+                    optimized_fetch=opt.optimized_fetch,
+                    retry_fetches=opt.retry_fetches,
+                    prune=opt.prune,
+                    ssh_proxy=ssh_proxy,
+                    clone_filter=manifest.CloneFilter,
+                    partial_clone_exclude=manifest.PartialCloneExclude,
+                    clone_filter_for_depth=manifest.CloneFilterForDepth,
+                )
+                if sync_result.error:
+                    errors.append(sync_result.error)
+
+                finish = time.time()
+                self.event_log.AddSync(
+                    rp,
+                    event_log.TASK_SYNC_NETWORK,
+                    start,
+                    finish,
+                    sync_result.success,
+                )
+                if not sync_result.success:
+                    logger.error("error: Cannot fetch repo tool %s", rp.name)
+                    return
+
+        # After fetching, check if a new version of repo is available and
+        # restart. This is only done if the user hasn't explicitly disabled it.
+        if os.environ.get("REPO_SKIP_SELF_UPDATE", "0") == "0":
+            _PostRepoFetch(rp, opt.repo_verify)
+
     def _ReloadManifest(self, manifest_name, manifest):
         """Reload the manfiest from the file specified by the |manifest_name|.
 
@@ -1871,6 +1916,9 @@ later is required to fix a server side protocol bug.
         # might be in the manifest.
         self._ValidateOptionsWithManifest(opt, mp)
 
+        # Update the repo project and check for new versions of repo.
+        self._UpdateRepoProject(opt, manifest, errors)
+
         superproject_logging_data = {}
         self._UpdateProjectsRevisionId(
             opt, args, superproject_logging_data, manifest
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index e7213ed9..9cd19f10 100644
--- a/tests/test_subcmds_sync.py
+++ b/tests/test_subcmds_sync.py
@@ -527,6 +527,108 @@ class SyncCommand(unittest.TestCase):
             self.assertIn(self.sync_network_half_error, e.aggregate_errors)
 
 
+class SyncUpdateRepoProject(unittest.TestCase):
+    """Tests for Sync._UpdateRepoProject."""
+
+    def setUp(self):
+        """Common setup."""
+        self.repodir = tempfile.mkdtemp(".repo")
+        self.manifest = manifest = mock.MagicMock(repodir=self.repodir)
+        # Create a repoProject with a mock Sync_NetworkHalf.
+        repoProject = mock.MagicMock(name="repo")
+        repoProject.Sync_NetworkHalf = mock.Mock(
+            return_value=SyncNetworkHalfResult(True, None)
+        )
+        manifest.repoProject = repoProject
+        manifest.IsArchive = False
+        manifest.CloneFilter = None
+        manifest.PartialCloneExclude = None
+        manifest.CloneFilterForDepth = None
+
+        git_event_log = mock.MagicMock(ErrorEvent=mock.Mock(return_value=None))
+        self.cmd = sync.Sync(manifest=manifest, git_event_log=git_event_log)
+
+        opt, _ = self.cmd.OptionParser.parse_args([])
+        opt.local_only = False
+        opt.repo_verify = False
+        opt.verbose = False
+        opt.quiet = True
+        opt.force_sync = False
+        opt.clone_bundle = False
+        opt.tags = False
+        opt.optimized_fetch = False
+        opt.retry_fetches = 0
+        opt.prune = False
+        self.opt = opt
+        self.errors = []
+
+        mock.patch.object(sync.Sync, "_GetCurrentBranchOnly").start()
+
+    def tearDown(self):
+        shutil.rmtree(self.repodir)
+        mock.patch.stopall()
+
+    def test_fetches_when_stale(self):
+        """Test it fetches when the repo project is stale."""
+        self.manifest.repoProject.LastFetch = time.time() - (
+            sync._ONE_DAY_S + 1
+        )
+
+        with mock.patch.object(sync, "_PostRepoFetch") as mock_post_fetch:
+            self.cmd._UpdateRepoProject(self.opt, self.manifest, self.errors)
+            self.manifest.repoProject.Sync_NetworkHalf.assert_called_once()
+            mock_post_fetch.assert_called_once()
+            self.assertEqual(self.errors, [])
+
+    def test_skips_when_fresh(self):
+        """Test it skips fetch when repo project is fresh."""
+        self.manifest.repoProject.LastFetch = time.time()
+
+        with mock.patch.object(sync, "_PostRepoFetch") as mock_post_fetch:
+            self.cmd._UpdateRepoProject(self.opt, self.manifest, self.errors)
+            self.manifest.repoProject.Sync_NetworkHalf.assert_not_called()
+            mock_post_fetch.assert_not_called()
+
+    def test_skips_local_only(self):
+        """Test it does nothing with --local-only."""
+        self.opt.local_only = True
+        self.manifest.repoProject.LastFetch = time.time() - (
+            sync._ONE_DAY_S + 1
+        )
+
+        with mock.patch.object(sync, "_PostRepoFetch") as mock_post_fetch:
+            self.cmd._UpdateRepoProject(self.opt, self.manifest, self.errors)
+            self.manifest.repoProject.Sync_NetworkHalf.assert_not_called()
+            mock_post_fetch.assert_not_called()
+
+    def test_post_repo_fetch_skipped_on_env_var(self):
+        """Test _PostRepoFetch is skipped when REPO_SKIP_SELF_UPDATE is set."""
+        self.manifest.repoProject.LastFetch = time.time()
+
+        with mock.patch.dict(os.environ, {"REPO_SKIP_SELF_UPDATE": "1"}):
+            with mock.patch.object(sync, "_PostRepoFetch") as mock_post_fetch:
+                self.cmd._UpdateRepoProject(
+                    self.opt, self.manifest, self.errors
+                )
+                mock_post_fetch.assert_not_called()
+
+    def test_fetch_failure_is_handled(self):
+        """Test that a fetch failure is recorded and doesn't crash."""
+        self.manifest.repoProject.LastFetch = time.time() - (
+            sync._ONE_DAY_S + 1
+        )
+        fetch_error = GitError("Fetch failed")
+        self.manifest.repoProject.Sync_NetworkHalf.return_value = (
+            SyncNetworkHalfResult(False, fetch_error)
+        )
+
+        with mock.patch.object(sync, "_PostRepoFetch") as mock_post_fetch:
+            self.cmd._UpdateRepoProject(self.opt, self.manifest, self.errors)
+            self.manifest.repoProject.Sync_NetworkHalf.assert_called_once()
+            mock_post_fetch.assert_not_called()
+            self.assertEqual(self.errors, [fetch_error])
+
+
 class InterleavedSyncTest(unittest.TestCase):
     """Tests for interleaved sync."""