sync: Refactor to use _RunOneGC and fix config leakage 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>
diff --git a/subcmds/sync.py b/subcmds/sync.py index 85ead6c..bfbe193 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py
@@ -1244,14 +1244,15 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 a6be080..b3726d2 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py
@@ -552,35 +552,36 @@ 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)