sync: Refactor to use _RunOneGC and fix config leakagev2.63stable

Extract _RunOneGC to handle GC on a single project. This refactoring
makes it easier to invoke GC from parallel worker tasks.

Also, avoid modifying the passed-in config dictionary in _RunOneGC by
creating a local copy, preventing unintended side effects on other
commands sharing the same config.

Bug: 498290329
Change-Id: I7b77ed6629b14b5ee3322870b9c6c8ce2bfd6ea2
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/574923
Reviewed-by: Becky Siegel <beckysiegel@google.com>
Tested-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Gavin Mak <gavinmak@google.com>

2 files changed, 42 insertions, 26 deletions

diff --git a/subcmds/sync.py b/subcmds/sync.py
index 85ead6c31..bfbe1937d 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -1244,14 +1244,15 @@ later is required to fix a server side protocol bug.
 
         return False
 
-    def _SetPreciousObjectsState(self, project: Project, opt):
+    @classmethod
+    def _SetPreciousObjectsState(cls, project: Project, opt):
         """Correct the preciousObjects state for the project.
 
         Args:
             project: the project to examine, and possibly correct.
             opt: options given to sync.
         """
-        expected = self._GetPreciousObjectsState(project, opt)
+        expected = cls._GetPreciousObjectsState(project, opt)
         actual = (
             project.config.GetBoolean("extensions.preciousObjects") or False
         )
@@ -1285,7 +1286,22 @@ later is required to fix a server side protocol bug.
                 project.config.SetString("extensions.preciousObjects", None)
                 project.config.SetString("gc.pruneExpire", None)
 
-    def _GCProjects(self, projects, opt, err_event):
+    @staticmethod
+    def _RunOneGC(project: Project, config: Optional[dict] = None) -> None:
+        """Run auto GC on a single project."""
+        local_config = {}
+        if config:
+            local_config.update(config)
+        local_config["gc.autoDetach"] = "false"
+        project.bare_git.gc("--auto", config=local_config)
+
+    @classmethod
+    def _GCProjects(
+        cls,
+        projects: List[Project],
+        opt: optparse.Values,
+        err_event: _threading.Event,
+    ) -> None:
         """Perform garbage collection.
 
         If We are skipping garbage collection (opt.auto_gc not set), we still
@@ -1295,7 +1311,7 @@ later is required to fix a server side protocol bug.
         if not opt.auto_gc:
             # Just repair preciousObjects state, and return.
             for project in projects:
-                self._SetPreciousObjectsState(project, opt)
+                cls._SetPreciousObjectsState(project, opt)
             return
 
         pm = Progress(
@@ -1305,9 +1321,8 @@ later is required to fix a server side protocol bug.
 
         tidy_dirs = {}
         for project in projects:
-            self._SetPreciousObjectsState(project, opt)
+            cls._SetPreciousObjectsState(project, opt)
 
-            project.config.SetString("gc.autoDetach", "false")
             # Only call git gc once per objdir, but call pack-refs for the
             # remainder.
             if project.objdir not in tidy_dirs:
@@ -1328,7 +1343,7 @@ later is required to fix a server side protocol bug.
                 pm.update(msg=bare_git._project.name)
 
                 if run_gc:
-                    bare_git.gc("--auto")
+                    cls._RunOneGC(bare_git._project)
                 else:
                     bare_git.pack_refs()
             pm.end()
@@ -1345,7 +1360,7 @@ later is required to fix a server side protocol bug.
             try:
                 try:
                     if run_gc:
-                        bare_git.gc("--auto", config=config)
+                        cls._RunOneGC(bare_git._project, config=config)
                     else:
                         bare_git.pack_refs(config=config)
                 except GitError:
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index a6be0807f..b3726d2d1 100644
--- a/tests/test_subcmds_sync.py
+++ b/tests/test_subcmds_sync.py
@@ -552,35 +552,36 @@ class GCProjectsTest(unittest.TestCase):
     def test_GCProjects_skip_gc(self, mock_progress):
         """Test that it skips GC if opt.auto_gc is False."""
         self.opt.auto_gc = False
-        self.cmd._SetPreciousObjectsState = mock.Mock()
-        self.cmd._GCProjects([self.project], self.opt, None)
-        self.cmd._SetPreciousObjectsState.assert_called_once_with(
-            self.project, self.opt
-        )
+        with mock.patch.object(
+            sync.Sync, "_SetPreciousObjectsState"
+        ) as mock_set_state:
+            self.cmd._GCProjects([self.project], self.opt, None)
+            mock_set_state.assert_called_once_with(self.project, self.opt)
         self.assertFalse(self.project.bare_git.gc.called)
 
     @mock.patch("subcmds.sync.Progress")
     def test_GCProjects_sequential(self, mock_progress):
         """Test sequential GC (jobs < 2)."""
-        self.cmd._SetPreciousObjectsState = mock.Mock()
-        self.cmd._GCProjects([self.project], self.opt, None)
-        self.project.config.SetString.assert_called_once_with(
-            "gc.autoDetach", "false"
+        with mock.patch.object(sync.Sync, "_SetPreciousObjectsState"):
+            self.cmd._GCProjects([self.project], self.opt, None)
+        self.project.bare_git.gc.assert_called_once_with(
+            "--auto", config={"gc.autoDetach": "false"}
         )
-        self.project.bare_git.gc.assert_called_once_with("--auto")
+        # Verify that gc.autoDetach was not permanently set in config.
+        for call in self.project.config.SetString.call_args_list:
+            self.assertNotEqual(call.args[0], "gc.autoDetach")
 
     @mock.patch("subcmds.sync.Progress")
     def test_GCProjects_parallel(self, mock_progress):
         """Test parallel GC (jobs >= 2)."""
         self.opt.jobs = 2
-        self.cmd._SetPreciousObjectsState = mock.Mock()
-
-        with mock.patch("subcmds.sync._threading.Thread") as mock_thread:
-            mock_t = mock.MagicMock()
-            mock_thread.return_value = mock_t
-            err_event = mock.Mock()
-            err_event.is_set.return_value = False
-            self.cmd._GCProjects([self.project], self.opt, err_event)
+        with mock.patch.object(sync.Sync, "_SetPreciousObjectsState"):
+            with mock.patch("subcmds.sync._threading.Thread") as mock_thread:
+                mock_t = mock.MagicMock()
+                mock_thread.return_value = mock_t
+                err_event = mock.Mock()
+                err_event.is_set.return_value = False
+                self.cmd._GCProjects([self.project], self.opt, err_event)
 
         self.assertTrue(mock_thread.called)